Restore using node.views for unique impressions count

[alonpeer]

Fixes #1196

This should improve performance of various page loads, as it avoids
running COUNT() queries on the impressions table, and instead uses the
views value, which the impressions gem keeps up-to-date.

Notice that I update the tests to .reload the node before reading values,
because ActiveRecord caches those, which is why tests fail without it.

---

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

• PR is descriptively titled 📑 and links the original issue above 🔗
• tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
• code is in uniquely-named feature branch and has no merge conflicts 📁
• screenshots/GIFs are attached 📎 in case of UI updation
• ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

[alonpeer]

@publiclab/reviewers please review. @jywarren I saw you were involved in the early adoption of the impressions gem, I'm curious what your take on this is.

[plotsbot]

	1 Warning
⚠️	New migrations added. Please update schema.rb.example by overwriting it with a copy of the up-to-date db/schema.rb. Also, be aware to preserve the MySQL-specific conditions for full-text indices.

	1 Message
📖	@alonpeer Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

[alonpeer]

I actually just realized there's a caveat with this fix.
Right now, node.views is always increased for any impression. After this change, it'll be increased only for unique IP addresses. This means that node.totalviews before this change is accurate (only unique views are returned), while after this change it'll increase a lot, due to duplicates per IP address.

In order to fix this, along with applying this change, a migration is needed: the node.views values for all rows should be updated with the count of uniques according to the data in the impressions table.

I'm not sure exactly what the procedure is in PublicLab for these kind of changes, but I'm happy to pair with someone to get this done.

Here's what can be run from rails console in order to update all nodes:

Node.ids.each do |id| 
  node = Node.find(id)  
  node.update_columns(views: node.impressionist_count(filter: :ip_address))  
end

[jywarren]

Hi @alonpeer ! Thank you so much; we could roll a migration into this PR if you think that makes sense? I think we are not /super/ worried about unique vs. non-unique, as this metric is just for getting a general idea anyways -- for a while, bots were triggering it. So, as long as we make a reasonable call, i think we can be OK with it. Thanks so much!

[alonpeer]

@jywarren how many rows are there in node in production? I'm happy to add a migration here, but would like to avoid crashing the deploy process, or OOM it, which is why I suggested migrating this manually.
But if the number of rows is a few thousands, or 10k, I think that'll be just fine.

[alonpeer]

I actually see other migrations written recently, which seem to do similar amount of work, so I assume that it's just fine 😃

[jywarren]

Maybe run it only for nodes with status=1? But yeah, good call! Thanks!!

…

On Mon, Mar 25, 2019 at 6:13 PM alonpeer ***@***.***> wrote: I actually see other migrations written recently, which seem to do similar amount of work, so I assume that it's just fine 😃 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5248 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJyzoi4qDqcITXJCxpWj85hYqKijPks5vaUnTgaJpZM4cFrzY> .

[alonpeer]

Hmm, can't nodes with status=0 get published again?

[alonpeer]

Anyway, migration file added.

[alonpeer]

Removed the [DO NOT MERGE] instruction 😄

[grvsachdeva]

Hey @alonpeer, I think you need to regenerate timestamp by running migration again as someone else has changed the file before you.

[jywarren]

You can also manually forward-date it and it should work :-) Thank you!

…

On Sun, Mar 31, 2019 at 4:39 PM Gaurav Sachdeva ***@***.***> wrote: Hey @alonpeer <https://github.com/alonpeer>, I think you need to regenerate timestamp by running migration again as someone else has changed the file before you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5248 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ3LCgi9eyLhyxwxszPB-pCDqWGytks5vcRzfgaJpZM4cFrzY> .