Fixed the pagination problem #4042
Conversation
Generated by 🚫 Danger |
|
Hey there, thanks for making this pull request! It's a good solution to the issue, but there are some issues that we should fix in the codeclimate so that the checks pass. I'll request some of them in the code. |
|
The mechanics of sorting are completely rewritten and the sort operation now happens in the backend, so with every filter change the website is reloaded. |
|
Ok, I'll try to solve them. |
|
I believe if the Travis Ci and Danger checks have passed, we may be able to ignore the CodeClimate issues.. |
|
Now that the codeclimate tests pass I think there is no problem with the PR. |
|
@okonek I am having issues passing Travis CI tests and I saw in their pull requests bank that you had a similar error to mine which you managed to fix:
I was wondering if you could tell me what was causing this error for you / how you fixed it please! |
|
Could you please link this PR, because I don't remember where did I have this issue? |
|
Ok, just added another commit to this PR, because I just realised a small issue, that is now resolved. Could someone please review my task on GCI? |
|
Wow this is a pretty involved PR! Thanks! Would anyone from @publiclab/reviewers or @publiclab/mentors be able to give it a close review? Thanks!!!! |
|
Yes, I worked pretty hard on that one. Please review. I'm waiting for two days. |
Gemfile.lock
Outdated
| @@ -60,7 +60,6 @@ GEM | |||
| scrypt (>= 1.2, < 4.0) | |||
| authlogic-oid (1.0.4) | |||
There was a problem hiding this comment.
Please remove Gemfile.lock from your PR, as we update it only when required. Thanks!
|
Hi, @okonek - apologies, it takes a bit to review a longer PR, and it looks like many mentors are tied up in exams at the moment. We appreciate your patience. @gauravano perhaps we should push this to unstable to test it out live, as well? |
app/assets/javascripts/dashboard.js
Outdated
| @@ -1,15 +1,19 @@ | |||
| /* eslint-disable complexity */ | |||
| /* eslint-disable wrap-iife */ | |||
There was a problem hiding this comment.
I don't think these lines are needed.
|
Agreed @jywarren, I am pushing it on |
|
Should be testable here: https://unstable.publiclab.org/dashboard |
|
Oh, odd... @icarito did something more change in our subdomains? I can't seem to load unstable as it goes directly to Jenkins... |
|
Let's see how it works after the build finishes . |
| } | ||
| var baseurl = window.location.href; | ||
| url = new URL(baseurl); |
There was a problem hiding this comment.
Hi, reading through this carefully, it's very well crafted, and thank you!
One thing that was true about the previous version was that the types shown did not have to be specified in the URL. I'm thinking about the typical use case of someone coming to /dashboard -- does this code refresh the page in order to send that information to the controller? I am not sure that's the perfect solution here but I'm trying to understand how you've approached the problem.
If I'm misunderstanding, could you add some comments to explain the sequence, as the comments had been in the original version? I know it's some extra work, but this is a complex interaction here and we definitely want to get it right and make sure it's readable by future developers too!
Thanks so much for your work on this. We'll hopefully get the 'unstable' branch material working again so it'll be easier to review, too.
|
Unstable is running as build finished. |
|
oh oh oh WOW it's a new feature! Showing Jenkins until it's done!!! Wow!!! |
|
I think I got your approach. Currently, we take all the notes, wikis, comments and then go to dashboard, and pagination show count totaling the sum. When user try to show notes only, or wikis only, other nodes get invisible 👓 but count remains same. For instance, suppose there are 1000 total notes displaying over 100 pages. And, there are 10 events in them, a event can be anywhere 8th page, 9th page.... Now, if we wish to show only events, other nodes(notes, wikis, questions get invisible) and only events show but on their same positions i.e., on 8th page, 9th page, etc and other pages appear empty with same count of pages because in reality other nodes are there only. So, you are reloading page in this PR and loading the content which is checked. Am I right? |
|
What if we started showing the type only once someone begins clicking the
pagination... So normally at /dashboard it would show pagination for all,
but once you start going back to older pages like page=2, it begins to use
your filtering to show more accurate pagination by also appending
types=comments,questions, etc?
It also isn't perfect but it's a good compromise and also a good adaptation
of the code that's now been written?
…On Tue, Nov 27, 2018, 6:44 PM Gaurav Sachdeva ***@***.*** wrote:
I think I got your approach. Currently, we take all the notes, wikis,
comments and then go to dashboard, and pagination show count totaling the
sum. When user try to show notes only, or wikis only, other nodes get
invisible 👓 but count remains same. For instance, suppose there are 1000
total notes displaying over 100 pages. And, there are 10 events in them, a
event can be anywhere 8th page, 9th page.... Now, if we wish to show only
events, other nodes(notes, wikis, questions get invisible) and only events
show but on their same positions i.e., on 8th page, 9th page, etc and other
pages appear empty with same count of pages because in reality other nodes
are there only.
So, you are reloading page in this PR and loading the content which is
checked. Am I right?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4042 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJyHaD4MlRkIImXPUAxqIQQbYRsLrks5uzc4jgaJpZM4YyBuC>
.
|
|
I am testing the new dashboard on unstable and I feel there are some issues:
@jywarren do you think we should change layout of dashboard something like tabs on profile page - https://publiclab.org/profile/warren . Do you think it can be discussed in Openhour? |
|
@jywarren And? I know you are busy, but I wait for very long. |
|
Hi, @okonek - thanks for your patience. The main reason I prefer the solution you've finished (as mentioned in this comment, and adding the The only other solution I can think of is to store the settings in the session, or in the database, so that the server side can "know" to display the right types, and we wouldn't have to rely on JavaScript. But the JavaScript solution has been in place for a long time and people haven't had a major problem with it, so I think we might as well stick with what works rather than introduce a page refresh that happens a lot. Thanks for working on this one, I think we can wrap it up now! |
|
Yes, that's fine with me! Although I've love to see the latest code Jan
added. Thanks!
…On Fri, Nov 30, 2018 at 5:24 PM Gaurav Sachdeva ***@***.***> wrote:
I think the GCI task can be approved based on the proposed solution. What
do you think @jywarren <https://github.com/jywarren> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4042 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ1ctaAPGGdnc-rL8s2_BL_KLCWZrks5u0a_0gaJpZM4YyBuC>
.
|
|
@jywarren I think I resolved the issue perfectly now. With small changes page now fetches filtered data using AJAX, so there is no reloading. I also add a gif of how it works now. If there are no code errors, I think it is mergable. |
|
I also resolved the issue of filtering questions. |
| $('.activity .col-md-6').css('display', ''); | ||
| getLocalStorageActivity(); | ||
|
|
||
| }); | ||
|
|
||
|
|
||
| $('.activity-dropdown .dropdown-toggle').click(function(e) { |
There was a problem hiding this comment.
OK, this is now empty - shall we remove it?
There was a problem hiding this comment.
What do you mean? What should be removed?
app/controllers/home_controller.rb
Outdated
| basenotes = basenotes.where('nid != (?)', blog.nid) if blog | ||
| notes = basenotes | ||
|
|
||
| questions = Tag.find_nodes_by_type('question:question', 'note', 999_999_999_999_999) |
There was a problem hiding this comment.
Here you should be able to pass nil and the limit will be lifted. But unfortunately, this will query the database for all question nodes, and likewise below all event nodes and above all notes -- on the production server this will be thousands of records, so we need to optimize for it to work. The .page() method is quite sophisticated, and uses limit() and offset() to select just the right records to display on the given page. But the problem we're encountering here is that we are trying to subselect across different tables. I don't believe the will_paginate gem can do this.
What we could do here is to start by fetching the paginated notes, by leaving in .page() for that query. For events and questions, which are also nodes, we could try to filter them in/out if they are not selected... this could be difficult but we could re-use some of the code in Tag.find_nodes_by_type, maybe...
Then, we could use the timestamps from the nodes we fetched to select the comments which were made during the same period. These could be added in, and while the calculation of total # of pages would be correct, the number of items per page would vary since we would be calculating # of pages based only on nodes.
I'm trying to think of how this could be done more simply but it was already a pretty complex thing to display in the first place... the critical part now is to not return the entire table's worth of each type on each page load. This works on a very small dataset but would almost certainly crash the server when 1000s of records are returned each time in production.
There was a problem hiding this comment.
https://stackoverflow.com/a/7959348 suggests synchronizing the offset, but then over time the comments and nodes would drift because they don't occur at the same rate over time.
There was a problem hiding this comment.
OK, here's an idea. I don't know if it's good, but just to think through it...
Let's simplify - let's put wiki edits permanently in the sidebar and display them below in mobile view, so we just don't worry about wikis as part of this. Then we're left with Notes, Events, Questions (all types of Node) and Comments.
The first three we could probably filter if we wanted based on what tags they have, using joins. But Comments messes us up, because it's a different table (see my above comment). What if we, instead of showing pure comments, showed only Nodes in Activity, but we joined comments where they exist (let's forget Answer comments for now) and we sorted by comment.timestamp and then node.created: https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-order -- would that work? And then in the views, we detect if there is a comment attached and we display the X commented on Y Z time ago instead of the usual note template?
This isn't air-tight, but seems plausible. I'm sorry I missed the change in pagination from the very first commit. I'm going to think on this more and happy to hear your thoughts too.
There was a problem hiding this comment.
In the above scenario, the right type of join will return multiple duplicate Node records, but one for each comment, and ordered by comment timestamp. I think that's fine, because each would really be a "stand in" for the associated comment record. But alternatively we could try to use a join type which would return only unique/discrete Node records, but choose the most recent associated comment record. Then we could show the Node template on the dashboard, and kind of "attach" the most recent comment to the bottom of it, so as to show why it's appearing in the timeline at that point. Just thinking through possibilities here.
There was a problem hiding this comment.
Finally, because this is already quite complex, we could stop offering to filter by Event and Question, simply grouping all Nodes together. That would simplify things a good bit, leaving only All, "Posts" (we'll call them?), and Comments. Trying to think of how to make this a reasonable path forward.
There was a problem hiding this comment.
You know what? I have a simpler idea. I know it's not that great but it's better than fetching thousands. We could just fetch the first 37 from all of the tables, because it's the nodes limit for one page. You know what I mean? It's a temporary solution, but it's not that bad and it could be solved in another PR.
There was a problem hiding this comment.
I think this is doable, and I agree with the principle of doing something now and following up in later PRs. But this would start to go out of time sync pretty fast because there are more comments per time period than nodes. Could we try the simpler solution but using timestamps instead of record count?
There was a problem hiding this comment.
So you'd narrow your query with a where() and I think you can even pass a range to where() in activerecord... then it'd be like .where(timestamp: basenotes.first.created..basenotes.last.created) i think?
|
@publiclab/reviewers @publiclab/mentors we ran into a REALLY tough issue in @okonek's PR here... a true puzzle trying to develop some solutions for. @okonek has done some tremendous work on the problem but it's revealed an even larger challenge for database query optimization on the dashboard. If anyone likes that kind of problem, we'd be grateful for some ideas or input; see my comment at: https://github.com/publiclab/plots2/pull/4042/files#r238070635 |
|
@jywarren Could you accept limiting every query to 37 nodes? The maximum node count per page? Please, I'd like to be done with this issue. |
|
Check out my comment above -- limiting by timestamp instead -- i think that is pretty good. If you are really eager to move on to another issue, I totally understand. This one was extremely challenging, to tell the truth, especially with the added issue of database optimization. Your solution is super nice - the Ajax is really wonderful. It's just an issue which, if we could go back, perhaps should have been solved in smaller parts and architected more thoroughly. |
|
@jywarren Ok, I don't want to leave this issue with a bad solution. Could you better describe your idea of limiting by timestamps? Thanks |
|
And could you tell me why limiting by 37 is bad? I mean, I can't find a case where it wouldn' work. Thanks |
|
Yes I will but it'll have to be tonight as I have some work now. But it's
just like limiting to 37 but using a time boundary instead of a count. I
can offer some more code hints tonight however. Thanks, and especially
since we did not describe this particular challenge in the original issue,
don't worry, you'll definitely be getting credit for this PR!
Thanks again,
J
…On Sat, Dec 1, 2018, 1:26 PM Jan Okoński ***@***.*** wrote:
@jywarren <https://github.com/jywarren> Ok, I don't want to leave this
issue with a bad solution. Could you better describe your idea of limiting
by timestamps? Thanks
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4042 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ1pEX_6JITC34snYmULgoIjZ0lpOks5u0smngaJpZM4YyBuC>
.
|
|
I'm just thinking loud, but I think that neither my solution or yours would work, because this limiting would break the pagination if we would like to parse an old site like 4, 5 etc |
|
I really don't know what to do. I'm stuck with this issue for very long |
|
@jywarren Any tip? |
|
Don't worry. I'll spend some time on this tonight.
…On Sat, Dec 1, 2018, 2:22 PM Jan Okoński ***@***.*** wrote:
@jywarren <https://github.com/jywarren> Any tip?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4042 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ2uSTmWLwJET-q15RsAXqTUem6e2ks5u0tbfgaJpZM4YyBuC>
.
|
|
Hi @okonek -- I've spent a long time thinking about this one, and I am afraid the best way forward for now is to leave it as it is, and close this PR. I really can't think of a way to separately paginate questions, wikis, comments, answer comments, events, and notes, and any combination. I want to apologize that the original issue made it seem that this was an easier problem than it really was, and I should have noticed that and suggested we break it into smaller parts, or solve it a different way. There's a lot of dense delicate code here, and it's not really great code either. I would like to take steps to simplify and optimize this code. But I think there may be related issues we could work on that would be ways to do this in smaller pieces. Let me suggest a few that may be interesting to you as you now understand this code very well.
I've also created four new refactoring issues which may be of interest. You've demonstrated a deep grasp of Rails code through this very challenging issue, and I want to offer you some tasks which may help you to build some real-world skills in refactoring, reorganizing, and managing larger code bases. If one of the top four here interests you, we'd love to have your help with one! I want to say that you've done some really impressive work here. Don't be discouraged that we weren't able to figure it out. Sometimes unexpected challenges come up as we break down a problem, and it challenges us to try something very different. We become better coders when this happens. Maybe we'll suddenly have an idea for how to do this set of functions better. Let's keep it in mind as we move forward. There is a dashboard redesign project coming soon as well, and it could be an opportunity to try something different. @okonek -- i'm going to give you credit for a viable solution to the original problem of pagination, even though new problems came up in the pursuit of this. As they weren't part of the original task, we'll consider that outside the scope of the challenge, although you did a great job thinking through it. Thanks for everything! If it's all right with you, we can close this PR, but the discussions here will be a good guide if and when we come back to this problem. |
|
Thank you very much for a tremendous help on that one. I can't say I was working on this alone. This problem is a lot bigger than anyone could thought. Anyways thank you for all your help and I am sure I've learned a lot from this issue. |
|
@okonek as you have worked on this challenge for quite a long time. We want to give you credit for this challenge. You can claim the task, I will approve it. |
|
@jywarren has already given you points. |
Fixes #2258 (<=== Add issue number here)
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
rake test@publiclab/reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto 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!