Likes page optimization #87

[sirMackk]

As per #87 - adds pagination to the @notes on a user's profile likes page to make it load faster. Additionally, it cuts some useless stuff out of the seeds.rb file that I became aware of during testing things out. Since this adds an initializer, it the production app might need a restart after applying this PR.

[jywarren]

Awesome, sirMackk - we'll get to this asap; i hope to have time to run tests on it today or tomorrow. Thanks.

BTW, does your use of Array.paginate still return all liked notes, but then serve them up in batches of 20? We could conceivably add a paginate function to https://github.com/publiclab/plots2/blob/master/app/models/drupal_users.rb#L49, or alternatively just manually use that line with a .paginate call instead of relying on the model definition.

[sirMackk]

No hurry! I'll have to investigate the Array.paginate method since it's using will_paginate. I'll get back to you as soon as I check it out.

[sirMackk]

I investigated the Array.paginate method and it indeed does return all liked notes and serves them in batches of 10. I think it would be possible to alter the liked method on user to return a relation instead of array, which would allow the paginate method to limit the number of liked notes returned - it would be easier on the server.

Thanks for pointing that out! I'll come back with a solution soon, probably after the weekend since I'm doing the whole moving thing and I'll be out of range of any wifi.

[btbonval]

We should push pagination down to the database and not paginate after
retrieving all results. Unsure what Array.paginate does, but it sounds like
@sirMackk suggests it operates at a level higher than the db?

On Fri, May 30, 2014 at 1:03 AM, sirMackk notifications@github.com wrote:

I investigated the Array.paginate method and it indeed does return all
liked notes and serves them in batches of 10. I think it would be possible
to alter the liked method on user to return a relation instead of array,
which would allow the paginate method to limit the number of liked notes
returned - it would be easier on the server.

Thanks for pointing that out! I'll come back with a solution soon,
probably after the weekend since I'm doing the whole moving thing and I'll
be out of range of any wifi.

—
Reply to this email directly or view it on GitHub
#88 (comment).

[jywarren]

Yes, the better .paginate methods (in the will_paginate plugin) like the
replacement for the ActiveRecord .find method do database pagination.
Array.paginate is just there in case all else fails, I think.

On Fri, May 30, 2014 at 11:18 AM, Bryan Bonvallet notifications@github.com
wrote:

We should push pagination down to the database and not paginate after
retrieving all results. Unsure what Array.paginate does, but it sounds
like
@sirMackk suggests it operates at a level higher than the db?

On Fri, May 30, 2014 at 1:03 AM, sirMackk notifications@github.com
wrote:

I investigated the Array.paginate method and it indeed does return all
liked notes and serves them in batches of 10. I think it would be
possible
to alter the liked method on user to return a relation instead of array,
which would allow the paginate method to limit the number of liked notes
returned - it would be easier on the server.

Thanks for pointing that out! I'll come back with a solution soon,
probably after the weekend since I'm doing the whole moving thing and
I'll
be out of range of any wifi.

—
Reply to this email directly or view it on GitHub
#88 (comment).

—
Reply to this email directly or view it on GitHub
#88 (comment).

[btbonval]

Cool. In SQL pagination is performed with LIMIT and OFFSET, but no one
should ever have to program SQL directly anymore. Learning the millions of
ORMs on top of SQL is pretty tricky, so I still don't know all the possible
ActiveRecord/Rails equivalents for doing the same thing.

On Fri, May 30, 2014 at 8:22 AM, Jeffrey Warren notifications@github.com
wrote:

Yes, the better .paginate methods (in the will_paginate plugin) like the
replacement for the ActiveRecord .find method do database pagination.
Array.paginate is just there in case all else fails, I think.

On Fri, May 30, 2014 at 11:18 AM, Bryan Bonvallet <
notifications@github.com>
wrote:

We should push pagination down to the database and not paginate after
retrieving all results. Unsure what Array.paginate does, but it sounds
like
@sirMackk suggests it operates at a level higher than the db?

On Fri, May 30, 2014 at 1:03 AM, sirMackk notifications@github.com
wrote:

I investigated the Array.paginate method and it indeed does return all
liked notes and serves them in batches of 10. I think it would be
possible
to alter the liked method on user to return a relation instead of
array,
which would allow the paginate method to limit the number of liked
notes
returned - it would be easier on the server.

Thanks for pointing that out! I'll come back with a solution soon,
probably after the weekend since I'm doing the whole moving thing and
I'll
be out of range of any wifi.

—
Reply to this email directly or view it on GitHub
#88 (comment).

—
Reply to this email directly or view it on GitHub
#88 (comment).

—
Reply to this email directly or view it on GitHub
#88 (comment).

[sirMackk]

Fixed this to use LIMIT and OFFSET so that the will_paginate gem uses the DB for more efficient queries, as per the suggestions.

@btbonval You're totally right on this. All I had to do was to make sure a method was returning a relation instead of an array. Relations were one of the gotchas I ran into when I was learning rails - they lazy load the data, so it's possible to tack on extra params down the road (.limit, .paginate, .order, etc.) and the whole query only gets executed when we really need it to.

I'll get on to fixing #73 to merge cleanly.

[btbonval]

Hot dog!

On Tue, Jun 3, 2014 at 1:52 PM, sirMackk notifications@github.com wrote:

Fixed this to use LIMIT and OFFSET so that the will_paginate gem uses the
DB for more efficient queries, as per the suggestions.

@btbonval https://github.com/btbonval You're totally right on this. All
I had to do was to make sure a method was returning a relation instead of
an array. Relations were one of the gotchas I ran into when I was learning
rails - they lazy load the data, so it's possible to tack on extra params
down the road (.limit, .paginate, .order, etc.) and the whole query only
gets executed when we really need it to.

I'll get on to fixing #73 #73
to merge cleanly.

—
Reply to this email directly or view it on GitHub
#88 (comment).

[jywarren]

So tests pass on my local machine but not on the test.publiclab.org vm -- it fails with an error:

ActiveRecord::StatementInvalid: Mysql2::Error: Table 'publiclab_test.users' doesn't exist: SHOW FIELDS FROM `users` 

http://test.publiclab.org/test.log

I'm not sure if this has to do with the seeds.rb file, or what... maybe sirMackk@1c3f502#diff-f7a25bc30a409fc77050703fc048ca8cL18

I think this is ready to push to master along with a bunch of other changes I did in the past week, but want to resolve this issue first. Sorry, can you take a look and tell me what you think? Thanks!

[sirMackk]

@jywarren Sorry, I just saw your message. I'll check it out as soon as possible - thanks for including the log file. I got rid of those entries from the seeds.rb file, since I noticed that User.create! was also creating a DrupalUser, which means the db had 3 users and 6 drupal_users. I'll get back to you soon!

[jywarren]

Hmm, i think this may have more to do with the migrations in publiclab/plots#35 -- i have to admit i'm not really clear on how the tests run... i've always assumed they use the migrations, but maybe they just directly construct the db from the schema.rb file? In any case, i checked and the test db (after these failed tests) only includes:

| comments |
| community_tags |
| content_field_bbox |
| content_field_image_gallery |
| content_field_main_image |
| content_field_map_editor |
| content_field_mappers |
| content_type_map |
| files |
| images |
| node |
| node_access |
| node_counter |
| node_revisions |
| node_selections |
| profile_fields |
| profile_values |
| rsessions |
| rusers

[jywarren]

Reading in http://guides.rubyonrails.org/testing.html#maintaining-the-test-database-schema now... but don't have too much more time today because of the NE Barnraising :-)

[sirMackk]

Yup, definitely missing the "users" table and a few others. How are you setting up the test database on the vm? I remember sometimes having with the test db (on various projects) not being updated and having to run rake db:migrate RAILS_ENV=test. I'll try out a few things today. Have fun at the Barnraising!

[jywarren]

Ah, ok, so the EEEEEEEE (8 Es) at the beginning indicates 8 errors. Nice. That jives with the listed # of tests which errored in the first section. But clearly the test database hasn't completely populated. Maybe it ran halfway and errored out in a past test run? Yes, it seems like all tables after rusers are missing. Gotta check log/test.log and maybe a mysql log if there is one?

[jywarren]

OK, i dropped and recreated the test database, and got the same errors. So somehow it's not getting past creating rusers when constructing the test db. I'll try runnning rake db:migrate RAILS_ENV=test

[jywarren]

OK, running that resulted in

uninitialized constant ConsolidateTags::DrupalNodeTag/home/warren/sites/test.publiclab.org/db/migrate/20140429190219_consolidate_tags.rb:32:in `block in up' 

That's:

drupalnodetags = DrupalNodeTag.count(:all)

So I guess it's freaking out that I deleted the /app/models/drupal_node_tag.rb file. My fault!

[sirMackk]

Ah, that's interesting. That explains why the db populated only halfway as you said earlier. I had a similar issue with UrlAlias as that the migration needed the model, which was supposed to be deleted, so I used ActiveRecord::Base.connection.execute to access the url_alias table without the model existing anymore. If you're busy with the Barnraising, I'd be more than happy to take a look at consolidate tags migration issue today?

[jywarren]

Ah, just saw your comment. I'm going to be busy soon, but I just committed an empty stubbed drupal_node_tag.rb as a workaround. If you have a better way to do it, that'd be great!

[jywarren]

couple mistakes in creating that stub, but rake db:migrate RAILS_ENV=test ran fine now.

[jywarren]

Hooray! Tests pass!

[sirMackk]

Clever! Thats great! I'll see what can be done with that stub then :)

[jywarren]

This code is now live on publiclab.org -- thanks sirMackk!

On Fri, Jun 6, 2014 at 11:39 AM, sirMackk notifications@github.com wrote:

Clever! Thats great! I'll see what can be done with that stub then :)

—
Reply to this email directly or view it on GitHub
#88 (comment).

[btbonval]

Woooo!

On Fri, Jun 6, 2014 at 8:45 AM, Jeffrey Warren notifications@github.com
wrote:

This code is now live on publiclab.org -- thanks sirMackk!

On Fri, Jun 6, 2014 at 11:39 AM, sirMackk notifications@github.com
wrote:

Clever! Thats great! I'll see what can be done with that stub then :)

—
Reply to this email directly or view it on GitHub
#88 (comment).

—
Reply to this email directly or view it on GitHub
#88 (comment).

[sirMackk]

Awesome!