Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add basic text file imports #172

Merged
merged 22 commits into from Jan 14, 2020
Merged

add basic text file imports #172

merged 22 commits into from Jan 14, 2020

Conversation

@robjloranger
Copy link
Member

robjloranger commented Aug 16, 2019

this adds basic support for importing files as blog posts.

.txt and .md are supported at this time and the
collection is selectable, defaulting to draft.

if a collection is specified the post is federated.

T609


  • I have signed the CLA
this adds basic support for importing files as blog posts.

.txt and .md are supported at this time and the
collection is selectable, defaulting to draft.

if a collection is specified the post is federated.
@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented Aug 16, 2019

I got a big sidetracked down the rabbit hole of #96, once I figured out what was getting in my way I remembered seeing something about the NOW db function and found the PR.

so there may be things I missed migrating to a fresh branch, but quick manual tests were successful.

different template action for partial or complete import success
@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented Aug 17, 2019

It's looking pretty good. I would prefer if the errors could be in a similar style box as the success alert, it would look a bit more polished.

Other than that I might change the form to allow all plain text files, not just .txt and .md. Then verify they are plain text again on the server. Not everyone will use files extensions.

Copy link
Member

thebaer left a comment

Thanks, this is looking good so far. I haven't tested yet, but here are some thoughts from reading through everything.

account_import.go Outdated Show resolved Hide resolved
routes.go Outdated Show resolved Hide resolved
@thebaer

This comment has been minimized.

Copy link
Member

thebaer commented Aug 19, 2019

Also, good idea on allowing all plain text files, and not just those with a file extension.

@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented Aug 19, 2019

Awesome, I'll fix these up shortly.

I think I tried using CreatePost at first but ran into trouble with something, which may have been my doing.

handler for post request to import is now under /api/me/import
form target updated

also allow all plaintext files in form
this changes the import handler to use CreatePost instead of
CreateOwnedPost which required generation of non expiring access tokens
@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented Aug 19, 2019

@thebaer all set, tested on my end and still working

in favor of library side generation to support zip files
this updates to parse the time from the imported file, using v0.1.1 of
the wfimport library
@robjloranger robjloranger mentioned this pull request Aug 21, 2019
1 of 1 task complete
robjloranger and others added 8 commits Aug 26, 2019
now checking for and returning invalid content type errors
- Changes Import link location in dropdown menu
- Makes design consistent with Invite People page (and extracts some
  common CSS into core.less)
- Selects the user's first blog by default in the dropdown
- Changes the copy a bit

Ref T609
- Only retrieve a collection from database if an alias is submitted
- Only call GetCollection() once (previously, it was inside the loop)
- Return error if user doesn't own the collection

Ref T609
This moves file operations inside the `for` loop into an anonymous func,
so the `defer` calls don't wait until the end of the handler call to
actually execute.

Ref T609
@thebaer

This comment has been minimized.

Copy link
Member

thebaer commented Jan 9, 2020

Thanks for getting those things! After testing, I made a few changes.

I tweaked the design and copy on the Import page to be consistent with the rest of the site.

I also ran into problems when importing Draft posts -- whether from bad local data or something else, the GetCollection("") call actually returned a collection my account didn't own, and the posts were imported to it anyway. So I fixed the logic there to protect against this and to be more consistent with how we do these draft / collection post checks elsewhere in the app. And I updated the logic to make sure you can't import posts to collections you don't own.

I also reduced the number of database calls (previously, this was repeatedly making the same GetCollection() call in the for loop). And I fixed issues with putting defer calls for file operations in a loop.

Next I'll probably tweak the stuff around temporary files a bit.

Copy link
Member

thebaer left a comment

Last thing to wrap this up: let's use the actual file creation date as the Created time for new posts generated upon import. With that in place, we'll get this merged!

account_import.go Show resolved Hide resolved
@robjloranger robjloranger self-assigned this Jan 9, 2020
@robjloranger

This comment has been minimized.

Copy link
Member Author

robjloranger commented Jan 14, 2020

I had to use the last modified time as the File API does not provide created

@thebaer

This comment has been minimized.

Copy link
Member

thebaer commented Jan 14, 2020

Cool, that's not a problem (and maybe better, anyway). Testing now.

File API gives timestamp in milliseconds, not seconds, so this converts
it correctly.

Ref T609
@thebaer thebaer force-pushed the import-text branch from ab2e64a to 2b06699 Jan 14, 2020
thebaer added 2 commits Jan 14, 2020
File API gives timestamp in milliseconds, not seconds, so this converts
it on the client-side and sends it the correct time to the server.

Ref T609
Copy link
Member

thebaer left a comment

This wasn't working, as the File API gives the timestamp in milliseconds, not seconds. Went ahead and fixed that, and all looks good now.

Thanks, @robjloranger! Merging now.

@thebaer thebaer merged commit 75e2b60 into develop Jan 14, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@thebaer thebaer deleted the import-text branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.