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

ghost
Copy link

@ghost ghost 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.
@ghost
Copy link
Author

ghost 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
@ghost
Copy link
Author

ghost 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 thebaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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.

@ghost
Copy link
Author

ghost 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.

Rob Loranger added 2 commits August 19, 2019 09:05
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
@ghost
Copy link
Author

ghost commented Aug 19, 2019

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

Rob Loranger added 4 commits August 21, 2019 10:07
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
@ghost ghost mentioned this pull request Aug 21, 2019
1 task
Rob Loranger and others added 8 commits August 26, 2019 14:53
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
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 thebaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@ghost ghost self-assigned this Jan 9, 2020
@ghost
Copy link
Author

ghost commented Jan 14, 2020

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

@thebaer
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
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 thebaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@thebaer thebaer deleted the import-text branch January 14, 2020 17:34
@heyakyra
Copy link
Contributor

What information is supported with imports (authors, tags, etc.)? Is there official documentation on this somewhere?

@thebaer
Copy link
Member

thebaer commented Aug 20, 2020

@heyakyra, built-in text file imports only support:

  • title
  • body
  • publish date

For more comprehensive imports, the API is probably the best way to migrate (documentation here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants