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

Export articles/posts #576

Merged
merged 19 commits into from Nov 21, 2018
Merged

Export articles/posts #576

merged 19 commits into from Nov 21, 2018

Conversation

rhymes
Copy link
Contributor

@rhymes rhymes commented Sep 1, 2018

This is an attempt to implement an "export posts" feature on the website.

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

This PR is to allow people to export their own posts.

The feature works likes this:

  • the user requests an export, the flag articles_export_requested becomes true
  • the server starts the export inside an async job and then sends an email with the attached export as a zip file
  • the user receives such email with a zip containing a json of all their articles
  • the server sets articles_export_requested to false so that another request can be issued in the future
  • the server also registers the last time this request was made (which could help doing analytics on how many users have requested exports at least once)
  • the UI will tell the user if they are waiting for the email and bar them from issuing another request in the meantime

Being a WIP there are a few details to work out and I would like @benhalpern and @jessleenyc feedbacks on these question if possible:

  • the json export only exports a subset of the Article model: I'm not 100% sure the subset is correct because I'm not familiar with all the attributes. Maybe I should have included something I've excluded and viceversa
  • should the export also attach to the zip a folder with all the articles as markdown files and not just the json dump?
  • is the wording on the website and in the emails correct?

Related Tickets & Documents

Closes #219

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

export-posts

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

@benhalpern
Copy link
Contributor

  • We'll take some time to think over exactly which attributes make sense before this gets merged. Shouldn't affect your design decisions in the meantime, but this should be a whitelist rather than a blacklist. For clarity and security I think we should design around what we do send as opposed to what we don't.
  • I think the json dump, which includes relevant markdown, works as a starting point. We could add other functionality later.
  • Copy looks good to me, but similarly we can give this a final sweep once this otherwise ready. @jessleenyc @pkfrank if you have any thoughts on the copy or any other features here, give it some thought and we'll touch base before it goes live.

Overall this is pleasantly scalable. Had we built this in-house we would have likely rushed to do a simpler synchronous export which would have to be re-built once peoples' data started piling up.

One comment: I expect people will want to export their comments or other data in the future, so any logic which could be generalized as "export logic" as opposed to "article export" is probably worth considering at this point. Probably not worth building the other exports just yet. Articles are their own special category as they are the most portable concept of the site.

@rhymes
Copy link
Contributor Author

rhymes commented Sep 2, 2018

@benhalpern agreed. I didn't plan to have it merged it as it is, I'm happy we started a conversation. You're right, it definitely needs to be more generic, the service should be about "exporting user content" (regardless of the fact that the first version only exports articles), same for the two attributes introduce. Something more like export_requested instead of articles_export_requested.

Let's discuss it in the next few days, thanks for the review!

@rhymes
Copy link
Contributor Author

rhymes commented Oct 26, 2018

@benhalpern I gave this another go. The changes so far:

  • I switched from a blacklist to a whitelist
  • Made the code more generic by splitting the "articles" steps into their own class used by an Exporter
  • Changed the copy to make it more generic

I'm not sure about the naming of the class Exporter::Exporter but I'm open to suggestions on that and everything else.

@maestromac
Copy link
Member

@rhymes what about Exporter::Service?

@rhymes
Copy link
Contributor Author

rhymes commented Oct 31, 2018

@rhymes what about Exporter::Service?

fine by me! I'll change it right away

@jessleenyc
Copy link
Contributor

PS - the copy looks good to me! We can change the user facing copy to be more general once we get other types of exports in as an option.

Copy link
Contributor

@benhalpern benhalpern left a comment

Choose a reason for hiding this comment

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

Hey @rhymes, so sorry we haven't gotten to this yet. This all looks good to me. @maestromac I didn't wind up having time for a full-full-full human review, but I feel pretty good about all this. If you want to give this a review, we can merge at some point tomorrow.

@rhymes can you confirm you'd like to merge this if we give it the okay?

@rhymes rhymes changed the title [WIP]: Export articles/posts Export articles/posts Nov 13, 2018
@pr-triage pr-triage bot added the PR: reviewed-approved bot applied label for PR's where reviewer approves changes label Nov 13, 2018
@rhymes
Copy link
Contributor Author

rhymes commented Nov 13, 2018

@benhalpern @maestromac The branch is ready. My main concern was about the list of fields in the export and whatever might have been missed by my pair of eyes. Not being a critical core feature we can still adjust it on the go by tweaking the copy as @jessleenyc mentioned and then decide how to export the rest of the content (comments, hearts, unicorns, saved articles and so on)

@benhalpern
Copy link
Contributor

benhalpern commented Nov 14, 2018

@rhymes Nothing is sensitive in the export, but it probably makes sense to not include fields we don't really use.

  • body_html <- sort of legacy at this point. This is what we captured via WYSIWYG before switching to markdown very early on.
  • lat, long <- not implemented in general
  • updated_at <- not necessarily triggered by user. Could be confusing.

Those are the only ones I'd remove (and keeping them wouldn't be the end of the world)

I'll leave this up to @maestromac @jessleenyc and @Zhao-Andy to give this all final approval. Let me know when you've settled this all and I'll merge it.

@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Nov 14, 2018
@rhymes
Copy link
Contributor Author

rhymes commented Nov 14, 2018

@benhalpern done, thanks for review!

Copy link
Member

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

Aside from a few tiny details, awesome work @rhymes ! I've tested it out locally and it's works well!

spec/requests/user_settings_spec.rb Outdated Show resolved Hide resolved
app/services/exporter/service.rb Show resolved Hide resolved
@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Nov 19, 2018
@benhalpern
Copy link
Contributor

@maestromac feel free to take whatever action is needed and then merge this 🙂

@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Nov 21, 2018
@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Nov 21, 2018
@benhalpern benhalpern merged commit 915e2d7 into forem:master Nov 21, 2018
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Nov 21, 2018
@rhymes
Copy link
Contributor Author

rhymes commented Nov 21, 2018

@maestromac @benhalpern @jessleenyc thanks for the help in getting this past the finish line :-)

Really appreciate it!

@rhymes rhymes deleted the feature/export-posts branch November 21, 2018 16:32
@benhalpern
Copy link
Contributor

Hey @rhymes care to make a changelog post?

Thanks for your patience in getting this shipped, super excited for our future of data portability.

@rhymes
Copy link
Contributor Author

rhymes commented Nov 21, 2018

Sure! I'll write it now.

Thanks

@c33s
Copy link

c33s commented Nov 22, 2018

sorry for my late comment, haven't noticed that your referenced #219 in this PR.

thanks for implementing my feature request. awesome that the feature is now in the codebase.

and then sends an email with the attached export as a zip file

please make it also possible to download the generated file/files via the profile/setting/dashboard page (and an option to disable getting it sent by email). from security point of view, email is unsafe. all posts get transfered unencrypted (also unpublished ones).
a https encrypted download option is much better and safer. alternativly access via ssh+git would be an option see #220

@rhymes
Copy link
Contributor Author

rhymes commented Nov 22, 2018

@c33s altough I understand the value of a "download" link, the website and the content are already public. Which sensitive data are you referring to?

@c33s
Copy link

c33s commented Nov 22, 2018

@rhymes unpublished articles. which can contain uncleaned code including secrets.

@rhymes
Copy link
Contributor Author

rhymes commented Nov 22, 2018

@c33s nice catch, can you open a separate issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants