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

Feature/json exporter #5354

Merged
merged 1 commit into from Dec 17, 2014
Merged

Conversation

gdpelican
Copy link
Contributor

A quick first pass at a JSON exporter. Would welcome some feedback around which fields are required, useful, etc.

For #5343

Note that this hasn't touched the XML export functionality at all.

@jaywink
Copy link
Contributor

jaywink commented Oct 19, 2014

Awesome, thanks for working on this extremely important feature!

Will check in more detail later.

As a general comment relating to exporters, I guess ideally we should aim to push them to Sidekiq for processing - and then email the user a link to the file (auto-expiry of file). But would this be a problem for pods on Heroku for example? Solved with S3 support?

@gdpelican
Copy link
Contributor Author

Yeah, thought about emailing them their data, but can't do it directly with the potential json size. Heroku would probably preclude storing it locally, but uploading could be a solution.

Maybe after the file's done processing we create an Export model, which uploads the file to S3 and auto-explodes in like 3 days, James Bond-style.

class Export < ActiveModel::Base
  belongs_to :user
  has_attached_file :exported_data
  after_create :schedule_deletion

  def schedule_deletion
    # schedule another sidekiq to destroy this model 
  end
end

??

@jhass
Copy link
Member

jhass commented Oct 19, 2014

In either case, we should consider running the result through gzip, that'll drastically reduce the filesize.

@jaywink
Copy link
Contributor

jaywink commented Nov 22, 2014

OK revisiting this, sorry for taking so long.

I think initially it's ok if we build 'n' serve the file as it is done now in runtime as long as we don't have large data included - eg comments and posts. When those arrive, we need to push the processing into sidekiq.

Agreed?

Also, the JSON isn't actual JSON in my env at least, it's like:

{:name=>"jaywink@localhost:3000", :email=>"mail@jasonrobinson.me", :serialized_private_key=>"-----BEGIN RSA PRIVATE KEY-----\nMIIJKQIBAAKCAgEAtMQTCTnPaUCzoNSzDFlZSoRXHEOkHjcBoXybUs7K15Ny9Bbz\nnbvrQmhG7LUxJ29On8jkvnHe7JWMfLztIHMjMe2sqcruo003KcahT2i9I3Zd6nSm\n
.....

And so on.

Otherwise, once the format and what attributes to include have been tweaked - would love to get this merged!

@jaywink
Copy link
Contributor

jaywink commented Nov 22, 2014

Also one other thing. I think we should include the diaspora* version that exported the file from day one. The tricky thing is how to get this, since any develop version pods would just report head.

@jhass any ideas? Kinda difficult how we replace the version in defaults.version.number currently.. Having the real version number there always would be nice and then having defaults.version.head True/False would be something that would work better - but I guess that would change quite many things..

@@ -45,6 +47,8 @@ class User < ActiveRecord::Base
has_many :invitations_from_me, :class_name => 'Invitation', :foreign_key => :sender_id
has_many :invitations_to_me, :class_name => 'Invitation', :foreign_key => :recipient_id
has_many :aspects, -> { order('order_id ASC') }
has_many :posts, through: :person
Copy link
Contributor

Choose a reason for hiding this comment

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

If we outscope posts and comments from this pull, these two should be removed to be added later.

@jhass
Copy link
Member

jhass commented Nov 22, 2014

Maybe we should just give the export format its own version. Including the current release won't help much for the dev version, since you still don't know if it was 0.5.0.0dev prior the breaking change or 0.5.0.0dev after the breaking change.

@gdpelican
Copy link
Contributor Author

Alright, finally got some time to sit down with this for a little bit. Should be ready for another look @jaywink

def strategy(format)
@strategy ||= case format
when :json then Exporters::JSON
when :xml then Exporters::XML
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just drop everything related to the old XML user exporter? A community decision already accepted XML as the new format, the old one doesn't really work anyway.

Objections anyone?

Choose a reason for hiding this comment

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

A community decision already accepted XML as the new format

Shurely JSON, not XML?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah :D

On 7 December 2014 16:19:03 EET, goob notifications@github.com wrote:

@@ -5,12 +5,24 @@
module Diaspora

class Exporter

  • def initialize(strategy)
  •  self.class.send(:include, strategy)
    
  • def initialize(format = :json)
  •  self.class.include strategy(format) if
    
    strategy(format).present?
  • end
  • def strategy(format)
  •  @strategy ||= case format
    
  •    when :json then Exporters::JSON
    
  •    when :xml  then Exporters::XML
    

A community decision already accepted XML as the new format

Shurely JSON, not XML?


Reply to this email directly or view it on GitHub:
https://github.com/diaspora/diaspora/pull/5354/files#r21424879

Br,
Jason Robinson
https://jasonrobinson.me

@jaywink
Copy link
Contributor

jaywink commented Dec 7, 2014

Thanks @gdpelican I'll check the export asap on my dev environment, hopefully tonight. I think this is pretty close, personally I think we should drop the XML stuff already. If no one objects within a few days, feel free to do it IMHO :)

@jaywink
Copy link
Contributor

jaywink commented Dec 7, 2014

Quick test in dev env, looks ok to me. Note it's not prettyfied of course normally, I prettified it.

One thing that needs fixing is that contacts don't show which aspect they have been added to. The one contact here below is in the "Friends" aspect, but the json doesn't show that in any way.

{
    "name": "foobar@localhost:3000",
    "email": "foobar@example.com",
    "language": "en",
    "username": "foobar",
    "disable_mail": false,
    "show_community_spotlight_in_stream": true,
    "auto_follow_back": true,
    "auto_follow_back_aspect": "Acquaintances",
    "profile": {
        "first_name": "",
        "last_name": "",
        "gender": "",
        "bio": "### Markdown header\r\n\r\n> quote\r\n\r\n   code para\r\n\r\nFoo",
        "location": "Hki",
        "image_url": "/assets/user/default.png",
        "diaspora_handle": "foobar@localhost:3000",
        "searchable": true,
        "nsfw": false
    },
    "aspects": [{
        "name": "Family",
        "contacts_visible": true,
        "chat_enabled": false
    }, {
        "name": "Friends",
        "contacts_visible": true,
        "chat_enabled": false
    }, {
        "name": "Work",
        "contacts_visible": true,
        "chat_enabled": false
    }, {
        "name": "Acquaintances",
        "contacts_visible": true,
        "chat_enabled": false
    }],
    "contacts": [{
        "sharing": false,
        "receiving": true,
        "person_guid": "bed18bf02ee001323b1010c37b6c3c6f",
        "person_name": "jaywink@localhost:3000",
        "person_first_name": "jaywink",
        "person_diaspora_handle": "jaywink@localhost:3000"
    }]
}

@jaywink
Copy link
Contributor

jaywink commented Dec 7, 2014

Also, date of birth is missing from the profile fields :)

@jaywink
Copy link
Contributor

jaywink commented Dec 7, 2014

Also, something odd related to the exporter, but that is probably the fault of the profile system and not the exporter itself. Just after creating the profile it looks like this:

    "first_name": null,
    "last_name": null,

But after saving it once, it looks like this:

    "first_name": "",
    "last_name": "",

So we probably have a bug in profile creation, setting fields to null, since the save does things differently.

@gdpelican gdpelican force-pushed the feature/json-exporter branch 3 times, most recently from f0035fe to e976fae Compare December 8, 2014 09:02
@gdpelican
Copy link
Contributor Author

@jaywink

Cool, so I

  • Added version 1.0 to the root of the exported json, pushing everything else into 'user'
  • Added birthday to the profile serializer
  • Added aspects to the contact serializer
  • Nuked the XML serializer entirely

Didn't get a chance to look at the null / "" profile name beyond having a chuckle at the checkbox tags wtf comment in the profiles_controller. (checkbox tags are, indeed, wtf.)
Strikes me as pretty low on the scale of broken?

@Flaburgan
Copy link
Member

Yeah I don't think you should worry about that in this PR.

@@ -37,6 +37,8 @@ class User < ActiveRecord::Base
serialize :hidden_shareables, Hash

has_one :person, :foreign_key => :owner_id
has_one :profile, through: :person
Copy link
Contributor

Choose a reason for hiding this comment

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

Quite a few tests are failing. I'm wondering if this is the cause? For example: AccountDeleter has all user association keys accounted for

Diff:
   @@ -13,7 +13,6 @@
     :invited_by,
     :notifications,
     :person,
   - :profile,
     :reports,
     :services,
     :tag_followings,

 # ./spec/lib/account_deleter_spec.rb:175:in `block (2 levels) in <top (required)>'

So the tests need a little check through, whether things are really "breaking" or just tests need patching.
Also, if you could clean the commit message and add a Changelog entry - I think this would be ready for merging.

@gdpelican
Copy link
Contributor Author

Pushed some test fixes, it seems like ActiveModelSerializer changes the default behaviour of render :json => @model a little bit; I was able to fix the failing tests by adding root: false to them.

(difference being {id: 1, name: 'name}with root: false, and {user: {id: 1, name: 'name'}}' with the defaulted root: true

Wonder if we should just turn off using the root element across the board for AMS, or if the tests passing is confidence enough that all the important json is still coming through correctly?

@jaywink

@gdpelican
Copy link
Contributor Author

Seems like there's still some frontend tests to fix up; I'll fix em up when I get the chance.

@gdpelican
Copy link
Contributor Author

Cool, looks like that build's passing now.

Squashed 'er up and put in a message in the changelog as well.

@Flaburgan
Copy link
Member

Hey, merge is close to you... :D

## Json exporter functionality (Iteration 1)

Allows users to export their data in JSON format from their user settings page

Copy link
Member

Choose a reason for hiding this comment

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

It's an important feature, but the big headings are for things the podmin needs to read and know for update ;)

@jhass
Copy link
Member

jhass commented Dec 11, 2014

Just some minor remarks, then this should be good :)

@gdpelican gdpelican force-pushed the feature/json-exporter branch 2 times, most recently from d417fb0 to 4ba9a64 Compare December 15, 2014 12:07
@profile_deletion.perform!
expect(@profile.reload.first_name).to be_blank
expect(@profile.reload.last_name).to be_blank
expect(@profile.reload.searchable).to be_falsey
Copy link
Member

Choose a reason for hiding this comment

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

Actually, slight optimization nitpick here, reload returns self and updates the receiver, so just do it one time above the expectations.

@gdpelican
Copy link
Contributor Author

Ready for final sign-off I think @jaywink @jhass

@jhass jhass added this to the next-major milestone Dec 17, 2014
@jhass
Copy link
Member

jhass commented Dec 17, 2014

Alright, thank you!

@jhass jhass closed this Dec 17, 2014
@jhass jhass reopened this Dec 17, 2014
jhass added a commit that referenced this pull request Dec 17, 2014
@jhass jhass merged commit 6806b2d into diaspora:develop Dec 17, 2014
@goobertron
Copy link

Thanks for your work on this, @gdpelican (and @jhass too).

@Flaburgan
Copy link
Member

Nice! The first step for #5343 :D

Thank you @gdpelican @jhass and @jaywink

@Flaburgan
Copy link
Member

Just pulled this on d-fr, I tested it with my account (more than 1000 contacts). I put it in http://jsonlint.com/ and it says "valid" :)

@svbergerem
Copy link
Member

Awesome! Nice work @gdpelican!

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

Successfully merging this pull request may close these issues.

None yet

6 participants