Plots2: Results are not sorted according to views

[radheyshyamjangid]

Please describe the problem (or idea)

results are not sorted according to views in wikis

What happened just before the problem occurred? Or what problem could this idea solve?

What did you expect to see that you didn't?

the results should be sorted according to page views.

Please show us where to look

https://publiclab.org/search/wikis/science+community?order=views

What's your PublicLab.org username?

jradheyshyam96

This can help us diagnose the issue:

Browser, version, and operating system

Windows 10 chrome

Many bugs are related to these -- please help us track it down and reproduce what you're seeing!

---

Thank you!

Your help makes Public Lab better! We deeply appreciate your helping refine and improve this site.

To learn how to write really great issues, which increases the chances they'll be resolved, see:

https://publiclab.org/wiki/developers#Contributing+for+non-coders

[welcome[bot]]

Thanks for opening your first issue here! Please follow the issue template to help us help you 👍🎉😄
If you have screenshots to share demonstrating the issue, that's really helpful! 📸

[avsingh999]

@radheyshyamjangid great catch

[igniteeng000]

https://publiclab.org/wiki?sort=page_views

[igniteeng000]

result are not sorted according to page views also.

[avsingh999]

@igniteeng000 I think he saying same as

[igniteeng000]

@avsingh999 @radheyshyamjangid please look at the links they both are different.

[radheyshyamjangid]

@igniteeng000 yup u are right, it is not working as should need to be. Contents are not sorting after clicking.

[radheyshyamjangid]

@avsingh999 nope it is different

[SidharthBansal]

great catch @radheyshyamjangid.
May I know who is working on this out of @igniteeng000 @radheyshyamjangid @avsingh999 ??

[SidharthBansal]

https://github.com/publiclab/plots2/pull/2002 might help.
I resolved a similar bug few months back

[igniteeng000]

I am not working on this. @SidharthBansal Please label this issue with appropriate tags.

[14Richa]

@SidharthBansal Can you assign this bug to me? I would like to try my hands on this!

[SidharthBansal]

Go ahead. No one has claimed to solve this issue before you. So you can solve this.

[SidharthBansal]

Hi, just checking if you've gotten stuck on this at all, or if I could help in any way? Thanks!

[14Richa]

Hey, thanks for checking. Weekdays become a difficult to be active here. I
will push something over the weekend. Thanks

On Wed, 3 Oct 2018 at 3:05 PM, Sidharth Bansal [e-mail]
wrote:

Hi, just checking if you've gotten stuck on this at all, or if I could
help in any way? Thanks!

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/plots2/issues/3531#issuecomment-426572170,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AnXvlIyyEFLZ6wwDhVx-Q_MfIbgSF5Njks5uhITcgaJpZM4W_pM2
.

[14Richa]

Hey Sidharth,
In my local session I think this works fine. Can you help me find what am I missing or is this issue resolved? Attached gif:
Thanks

[SidharthBansal]

Its little hard to reproduce. I remember I sent a commit for this an year
ago. You can seek help from @publiclab/mentors. Let's see if anyone can
reproduce the error on local

On Tue, Oct 9, 2018, 11:22 PM 14Richa [e-mail] wrote:

Hey Sidharth,
In my local session I think this works fine. Can you help me find what am
I missing or is this issue resolved? Attached gif:
Thanks

[image: sortbyviews]
https://user-images.githubusercontent.com/41283476/46688072-1054a980-cc1a-11e8-84a7-b50c71566ab8.gif

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/plots2/issues/3531#issuecomment-428287545,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AUACQ3AI68G2XL4R2LMqtt-T9WLsUTggks5ujOJdgaJpZM4W_pM2
.

[alonpeer]

I believe this is happening because the actual number of views is the sum of node.views and node.legacy_views, while the ORDER BY part of the queries only takes node.views into account. I cannot verify my guess, since I don't have access to the production DB, and I'm only partially familiar with the system.

I see two possible fixes:

1. Replace .order('views DESC') with `.order('(views + legacy_views) DESC').
   Pro: operation in the DB layer instead of app.
   Con: this seems to be an operation that is considered deprecated in future Rails versions:

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) 
called with non-attribute argument(s): "(node.views + node.legacy_views) DESC". Non-attribute 
arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, 
such as request parameters or model attributes. Known-safe values can be passed by wrapping them 
in Arel.sql().

1. Replace .order('views DESC') with .sort_by(&:totalviews).reverse.
   Pro: it works and uses the same views count as the one used for display.
   Con: it shifts the sorting to the app layer, which means the app must pull all nodes in order to sort them in memory. I don't know how big the set of nodes might be, but it should be fine if it's just a few hundreds.

2. Make a DB migration where legacy_views values are added to the views column, then deprecating that column completely.
   Pro: it simplifies the app and helps avoid confusion. Also, solves the problems with the first two solutions.
   Con: might involve much more work.

In order to help us make a choice here, it would be good to get some data about what's the largest number of wiki pages per tag that currently exists in the DB.

My personal opinion is that the third option is the best one, and I'm happy to help with planning and execution.

[jywarren]

Hi, i also think 3 is the most ideal, but i think i remember we hesitated
because the views column is re-calculated and it could be overwritten. But
if we can be extra sure this won't happen, then I'm fine with the idea of
merging it in. Thank you so much!

On Sun, Mar 17, 2019 at 8:08 PM alonpeer [e-mail] wrote:

I believe this is happening because the actual number of views is the sum
of node.views and node.legacy_views, while the ORDER BY part of the
queries only takes node.views into account. I cannot verify my guess,
since I don't have access to the production DB, and I'm only partially
familiar with the system.

I see two possible fixes:

1. Replace .order('views DESC') with `.order('(views + legacy_views)
   DESC').
   Pro: operation in the DB layer instead of app.
   Con: this seems to be an operation that is considered deprecated in
   future Rails versions:

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL)
called with non-attribute argument(s): "(node.views + node.legacy_views) DESC". Non-attribute
arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values,
such as request parameters or model attributes. Known-safe values can be passed by wrapping them
in Arel.sql().

1.

Replace .order('views DESC') with .sort_by(&:totalviews).reverse.
Pro: it works and uses the same views count as the one used for
display.
Con: it shifts the sorting to the app layer, which means the app must
pull all nodes in order to sort them in memory. I don't know how big the
set of nodes might be, but it should be fine if it's just a few hundreds.
2.

Make a DB migration where legacy_views values are added to the views
column, then deprecating that column completely.
Pro: it simplifies the app and helps avoid confusion. Also, solves the
problems with the first two solutions.
Con: might involve much more work.

In order to help us make a choice here, it would be good to get some data
about what's the largest number of wiki pages per tag that currently exists
in the DB.

My personal opinion is that the third option is the best one, and I'm
happy to help with planning and execution.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/plots2/issues/3531#issuecomment-473729754,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABfJxUDRFBixN4fL3SflkqUJlVE6JvWks5vXtjHgaJpZM4W_pM2
.

[alonpeer]

@jywarren great!
What would be the best way of verifying that views does not get recalculated?
And what would be the best way for me to pair with someone on this?

[jywarren]

Hi! As to verifying, i think probably I would look carefully through the
code for anywhere that writes to the views column. I don't know another way
to do it, but there should only be a few spots where this is done, maybe
even just one.

As to pairing, I'd suggest asking in our chatroom at
https://publiclab.org/chat (you can use Gitter as a default, it's quite
easy) and perhaps someone there would be able to? Thank you!

On Mon, Mar 18, 2019 at 12:19 PM alonpeer [e-mail] wrote:

@jywarren https://github.com/jywarren great!
What would be the best way of verifying that views does not get
recalculated?
And what would be the best way for me to pair with someone on this?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/plots2/issues/3531#issuecomment-473985970,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABfJy8RjWVwLkOKRrRvdnBAlexOP9cyks5vX7xjgaJpZM4W_pM2
.

[alonpeer]

Ahh, this ticket from 2017 seems to be very much related: https://github.com/publiclab/plots2/issues/1196

[alonpeer]

The column node.views is being used as the counter_cache for the impressions counter gem "impressionist". This means that it always gets re-evaluated.
When there's a new impression on a node page, the gem will INSERT INTO impressions a new row, and will also UPDATE node SET views = views + 1 WHERE nid = ?.
I think what we can do, in order to deprecated legacy_views, is to run a migration with UPDATE node SET views = views + legacy_views, and then just make use of node.views anywhere we need that data (for display or sorting).

[alonpeer]

Related PR: https://github.com/publiclab/plots2/pull/5248