[Outreachy] Add All Posts Route /notes #8800

RuthNjeri · 2020-12-04T14:55:04Z

I'm still trying to understand what 'All Posts' means.. this is a first attempt at trying to understand it.
I'll add a test once feedback is provided.

gitpod-io · 2020-12-04T14:55:08Z

RuthNjeri · 2020-12-04T14:57:50Z

The dashboard/all route is meant to display all posts. I have used the activity method to derive the details needed(Research note, questions, wiki, comments tab)

The only pending section is what will be displayed at the banner section...should it be a featured post?

sagarpreet-chadha

Looks great till now

app/controllers/home_controller.rb

app/views/dashboard/all_posts.html.erb

jywarren · 2020-12-06T15:57:18Z

Ah, my apologies @RuthNjeri - in this new design, we won't be mixing the types, so each of Notes, Questions, Wikis, and Comments will each simply have their own tab, and their own URL (so we won't load them all on a single page load, it'll be more efficient to just reload the page when the tab is clicked, just like at https://publiclab.org/tag/outreachy for example!).

Make sense? Thanks!!!

jywarren · 2020-12-06T16:04:18Z

So, perhaps we should think of this as a collection of pages that make up "All posts" -- All notes, All questions (would this lead to https://publiclab.org/questions so as not to be redundant?), All wikis (https://publiclab.org/wiki), All comments (this last one might actually link to or replace https://publiclab.org/comments?)

Let's make this PR just about notes, as there is no place where all notes are collected. We can think on the remaining types in the meantime! I'll ping @publiclab/community-reps about this. The main challenge if we link all other types to their corresponding pages, is that if our layout looks like tabs, people may expect those pages to appear like tabs, and be confused if they're led to pages that don't seem to be a sub-tab of the "All posts" page collection. Either this is just OK, and not a big deal, or maybe we would think of some way to indicate that the other types aren't tabs at all, but just links?

My take is that /questions /wiki /comments are well established and we should try to make the links not appear as tabs, which is a design challenge but not too tough, i think. Let's see what people think!

RuthNjeri · 2020-12-07T14:44:54Z

Ah, my apologies @RuthNjeri - in this new design, we won't be mixing the types, so each of Notes, Questions, Wikis, and Comments will each simply have their own tab, and their own URL (so we won't load them all on a single page load, it'll be more efficient to just reload the page when the tab is clicked, just like at https://publiclab.org/tag/outreachy for example!).

Make sense? Thanks!!!

This makes perfect sense!

RuthNjeri · 2020-12-07T15:07:34Z

So, perhaps we should think of this as a collection of pages that make up "All posts" -- All notes, All questions (would this lead to https://publiclab.org/questions so as not to be redundant?), All wikis (https://publiclab.org/wiki), All comments (this last one might actually link to or replace https://publiclab.org/comments?)

Let's make this PR just about notes, as there is no place where all notes are collected. We can think on the remaining types in the meantime! I'll ping @publiclab/community-reps about this. The main challenge if we link all other types to their corresponding pages, is that if our layout looks like tabs, people may expect those pages to appear like tabs, and be confused if they're led to pages that don't seem to be a sub-tab of the "All posts" page collection. Either this is just OK, and not a big deal, or maybe we would think of some way to indicate that the other types aren't tabs at all, but just links?

My take is that /questions /wiki /comments are well established and we should try to make the links not appear as tabs, which is a design challenge but not too tough, i think. Let's see what people think!

I'll create a query to collect all notes..

About the tabs issue, what if, instead of having a route and page called 'All Posts' we have a dropdown that has a link to each of the notes, questions, comments, and wiki.. that way, we won't have an entirely new page that looks similar to the topics page.

Since the 'All Posts' link will be on the Dashboard, we can make it a dropdown instead of a link.. unless it is necessary to have a main page for all the posts.

RuthNjeri · 2020-12-07T15:57:43Z

@jywarren I have just discovered that there's an action for all notes in the NotesController. The page is https://publiclab.org/notes

This might mean that we have the pages that display all: notes, questions, wikis, and comments. The only pending issue is the design of the page, I have shared my suggestion of using a dropdown on the Dashboard page, see above...

jywarren · 2020-12-07T17:07:58Z

Hi @RuthNjeri -

@jywarren I have just discovered that there's an action for all notes in the NotesController. The page is https://publiclab.org/notes

Whoa! OK yikes yes! Let's use that, and just refine the design to better match the /tag/____ style. And /notes is a good URL!

Since the 'All Posts' link will be on the Dashboard, we can make it a dropdown instead of a link..

I guess, to me, it makes sense for the /notes page to easily offer links to the other types, in case you go there and then want to "traverse" to other types. What if we instead of tabs, offered some "pill" links in the same order so it looks familiar in placement and order, but doesn't look like they function as tabs. The row of tabs shown here:

Could instead be a pill nav: https://getbootstrap.com/docs/4.4/components/navs/#pills

That way we can also keep the dashboard simpler with no dropdown. But, we can try this out and re-evaluate once we show it to people -- it would just take some styling changes on the Dashboard to make changes if people really want the dropdown, for example. How does this sound?

One thing is, I'm not sure if there are functional tests for the /notes route. We may want to add some simple ones.

Lastly, while we can get rid of all the sidebar content on /notes, we should consider retaining the "by likes" and "by views" options. Maybe those would go in the "sort by" dropdown we had been wanting to get rid of, since they already exist.

Oh- one more thing -- http://publiclab.org/notes is really slow. I suspect the queries and pagination generating that page are not well optimized. Does anything stand out to you about them? Maybe, we could try to refine them to be closer to the TagController show action, but without filtering for tag/topic? I guess it's these three routes:

plots2/app/controllers/notes_controller.rb

Lines 304 to 342 in cbb807b

    
           # notes with high # of likes 
        
           def liked 
        
             @title = I18n.t('notes_controller.highly_liked_research_notes') 
        
             @wikis = Node.limit(10) 
        
               .where(type: 'page', status: 1) 
        
               .order('nid DESC') 
        
             @notes = Node.research_notes 
        
               .where(status: 1) 
        
               .limit(20) 
        
               .order('cached_likes DESC') 
        
             @unpaginated = true 
        
             render template: 'notes/index' 
        
           end 
        
           def recent 
        
             @title = I18n.t('notes_controller.recent_research_notes') 
        
             @wikis = Node.limit(10) 
        
               .where(type: 'page', status: 1) 
        
               .order('nid DESC') 
        
             @notes = Node.where(type: 'note', status: 1, created: Time.now.to_i - 1.weeks.to_i..Time.now.to_i) 
        
                          .order('created DESC') 
        
             @unpaginated = true 
        
             render template: 'notes/index' 
        
           end 
        
           # notes with high # of views 
        
           def popular 
        
             @title = I18n.t('notes_controller.popular_research_notes') 
        
             @wikis = Node.limit(10) 
        
               .where(type: 'page', status: 1) 
        
               .order('nid DESC') 
        
             @notes = Node.research_notes 
        
               .limit(20) 
        
               .where(status: 1) 
        
               .order('views DESC') 
        
             @unpaginated = true 
        
             render template: 'notes/index' 
        
           end

RuthNjeri · 2020-12-07T17:15:55Z

Using the /notes URL and refining it seems like a great starting point. I think redesigning the page makes a lot of sense.. I'll break down the tasks needed for this in the Planning issue to keep track of the updates and push updates on this PR once I complete the first task...

I'll update you once I need a review....

jywarren · 2020-12-07T17:33:30Z

Gosh, actually looking at that code i don't see why it's so inefficient. it seems well limited by timestamp. Ah, i see! config/routes.rb shows it's actually the index action:

plots2/config/routes.rb

Line 139 in cbb807b

get 'notes' => 'notes#index'

So that's this:

plots2/app/controllers/notes_controller.rb

Lines 6 to 9 in cbb807b

    
           def index 
        
             @title = I18n.t('notes_controller.research_notes') 
        
             set_sidebar 
        
           end

Leading to this sub-method set_sidebar which is really old and probably no longer necessary:

plots2/app/controllers/application_controller.rb

Lines 26 to 67 in cbb807b

    
           def set_sidebar(type = :generic, data = :all, args = {}) 
        
             args[:note_count] ||= 8 
        
             if type == :tags # accepts data of array of tag names as strings 
        
               @notes ||= if params[:controller] == 'questions' 
        
                            Tag.find_nodes_by_type(data, 'note', args[:note_count]) 
        
                          else 
        
                            Tag.find_research_notes(data, args[:note_count]) 
        
                          end 
        
               @notes = @notes.where('node.nid != (?)', @node.nid) if @node 
        
               @wikis = Tag.find_pages(data, 10) 
        
               @videos = Tag.find_nodes_by_type_with_all_tags(%w(video) + data, 'note', 8) if args[:videos] && data.length > 1 
        
               @maps = Tag.find_nodes_by_type(data, 'map', 20) 
        
             else # type is generic 
        
               # remove "classroom" postings; also switch to an EXCEPT operator in sql, see https://github.com/publiclab/plots2/issues/375 
        
               hidden_nids = Node.where(type: :note, status: 1).select { |n| n.has_a_tag('hidden:response') }.collect(&:nid) 
        
               @notes = if params[:controller] == 'questions' 
        
                          Node.questions 
        
                            .joins(:revision) 
        
                        else 
        
                          Node.research_notes.joins(:revision).order('node.nid DESC').paginate(page: params[:page]) 
        
               end 
        
               @notes = @notes.where('node.nid != (?)', @node.nid) if @node 
        
               @notes = @notes.where('node_revisions.status = 1 AND node.nid NOT IN (?)', hidden_nids) unless hidden_nids.empty? 
        
               @notes = if logged_in_as(['admin', 'moderator']) 
        
                          @notes.where('(node.status = 1 OR node.status = 4)') 
        
                        elsif current_user 
        
                          @notes.where('(node.status = 1 OR (node.status = 4 AND node.uid = ?))', current_user.uid) 
        
                        else 
        
                          @notes.where('node.status = 1') 
        
                        end 
        
               @wikis = Node.order('changed DESC') 
        
                 .joins(:revision) 
        
                 .where('node_revisions.status = 1 AND node.status = 1 AND type = "page"') 
        
                 .limit(10) 
        
                 .group('node_revisions.nid') 
        
                 .order('node_revisions.timestamp DESC') 
        
             end 
        
           end

I think we should just skip set_sidebar and do it ourselves, since we only need that one query for recent nodes!

jywarren · 2020-12-07T17:34:02Z

Ah sorry was still writing into that last comment. But, sounds great, Ruth! Thank you!!!

ebarry · 2020-12-07T18:44:43Z

Really great detective work in this thread @RuthNjeri ! Thank you for finding that https://publiclab.org/notes exists, and planning an update!

I just consulted the @publiclab/community-reps staff, and there is agreement that can we work with pill navs to the pages where we already present content by type. There was excitement that wiki updates would be easier/more consistent to navigate to. Several people said that it was really the comments that they needed to browse chronologically.

RuthNjeri · 2020-12-08T12:57:11Z

Thanks for the clarification @ebarry 😄

About the need to browse through the comments chronologically, we have the comments route https://publiclab.org/comments where the comments are ordered according to the most recent, is that the chronological order that's needed?

Sagarpreet · 2020-12-08T17:09:39Z

is that the chronological order that's needed

Correct @RuthNjeri 👍

test/functional/notes_controller_test.rb

app/views/notes/index.html.erb

app/controllers/notes_controller.rb

app/views/notes/index.html.erb

RuthNjeri · 2020-12-09T16:28:20Z

Hi! This is ready for review. 😄

RuthNjeri · 2020-12-10T05:59:16Z

Hi! This is ready for review. 😄

Tagging @jywarren, @Sagarpreet, and @cesswairimu.. this is ready for review..

cesswairimu

Fantastic @RuthNjeri tada: 🎉

Great great job..love the loads of refactoring and cleanup you did here 🚀 🚀
Thanks

codeclimate · 2020-12-16T19:01:45Z

app/controllers/notes_controller.rb

+
+  def published_notes
+    hidden_nids = Node.hidden_response_node_ids
+


Trailing whitespace detected.

RuthNjeri · 2020-12-16T19:03:35Z

Force pushed due to recent rebase with the main branch

RuthNjeri · 2020-12-16T19:06:42Z

Hi @jywarren, it seems that the database setup is failing when the build is running...

I'm not sure why it's failing...

codecov · 2020-12-16T19:07:41Z

Codecov Report

❗ No coverage uploaded for pull request base (main@16a51d6). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #8800   +/-   ##
=======================================
  Coverage        ?   81.86%           
=======================================
  Files           ?      100           
  Lines           ?     5938           
  Branches        ?        0           
=======================================
  Hits            ?     4861           
  Misses          ?     1077           
  Partials        ?        0

jywarren · 2020-12-16T20:45:59Z

Strange, ok I'm actually seeing this integration test fail:

FAIL["test_should_choose_i18n_for_sidebar/_author_+_sidebar/_post_button", #<Minitest::Reporters::Suite:0x000055bd75e04178 @name="I18nTest">, 129.59562038299998]
 test_should_choose_i18n_for_sidebar/_author_+_sidebar/_post_button#I18nTest (129.60s)
        Expected at least 1 element matching "h4", found 0..
        Expected 0 to be >= 1.
        test/integration/I18n_test.rb:379:in `block (2 levels) in <class:I18nTest>'
        test/integration/I18n_test.rb:365:in `each'
        test/integration/I18n_test.rb:365:in `block in <class:I18nTest>'

=============|

I restarted to see if it's intermittent... Any ideas otherwise?

cesswairimu · 2020-12-17T05:22:51Z

app/views/notes/index.html.erb

-<% if params[:controller] == "tag" || params[:controller] == "search" %>
-  <%= render :partial => "sidebar/related" %>
-<% elsif params[:action] == "author" || params[:action] == "author_topic" %>
-  <%= render :partial => "sidebar/author" %>
-<% else %>
-  <%= render :partial => "sidebar/user" %>
-<% end %>


The removal of this section is the one causing the tests to fail. Also I don't think we want to remove this sidebar...it gives access to recent wiki edits ... @RuthNjeri you might also want to add back the set_sidebar under index in the controller. what do you think?

Thanks @cesswairimu 😄

The removal of that section was part of this discussion with @jywarren where we replace it with the sort dropdown for the notes...

I think a user will still be able to access the recent wiki edits via the wiki section that will be linked on the notes page, I am working on this here, therefore, access to the recent edits won't be completely lost, just changed to another page....

I think this can also be open to further discussion before the PR is merged..

Oh okay, my bad missed that, then I think we might need to rewrite that test or remove it

RuthNjeri · 2020-12-17T11:15:25Z

Strange, ok I'm actually seeing this integration test fail:

FAIL["test_should_choose_i18n_for_sidebar/_author_+_sidebar/_post_button", #<Minitest::Reporters::Suite:0x000055bd75e04178 @name="I18nTest">, 129.59562038299998]
 test_should_choose_i18n_for_sidebar/_author_+_sidebar/_post_button#I18nTest (129.60s)
        Expected at least 1 element matching "h4", found 0..
        Expected 0 to be >= 1.
        test/integration/I18n_test.rb:379:in `block (2 levels) in <class:I18nTest>'
        test/integration/I18n_test.rb:365:in `each'
        test/integration/I18n_test.rb:365:in `block in <class:I18nTest>'

=============|

I restarted to see if it's intermittent... Any ideas otherwise?

Hi @jywarren the test is related to the sidebar that I removed, so I've removed the test...though there's a discussion happening above whether the sidebar should be removed..

codeclimate · 2020-12-17T11:34:32Z

Code Climate has analyzed commit a27dddb and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	2

View more on Code Climate.

jywarren · 2020-12-17T18:35:53Z

No I think that's correct to remove the sidebar and the test, thank you! This looks awesome and it's so comforting to see the green All checks have passed ✅ message 😅

Merging now! Congratulations!!!! Thanks everyone!!!

jywarren · 2020-12-17T18:36:19Z

This should shortly be visible at stable.publiclab.org!!!

RuthNjeri · 2020-12-17T18:52:11Z

Nice! The notes URL is loading a bit faster now 😄

jywarren · 2020-12-17T20:14:26Z

Awesome to see!

…

On Thu, Dec 17, 2020, 1:52 PM Ruth ***@***.***> wrote: Nice! The notes URL is loading a bit faster now 😄 — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#8800 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J7ITCUIJMR76VG6EL3SVJHOVANCNFSM4UNRMNYQ> .

* all posts route * add to do item * notes controller * remove previous code and inline comments * remove inline comments * reuse published_notes for popular, recent, liked * implements review feedback * fix order and increase limit * fixed tests * revert pagy code in wiki * optimize query and add test for hidden response id * update styling * fix tests * add columns in the hidden_response select * fix failing test * revise node query * add a revision for the hidden note test node * remove trailing whitespace * remove sidebar test

RuthNjeri added this to the New Dashboard Implementation milestone Dec 4, 2020

sagarpreet-chadha reviewed Dec 5, 2020

View reviewed changes

app/controllers/home_controller.rb Outdated Show resolved Hide resolved

app/controllers/home_controller.rb Outdated Show resolved Hide resolved

app/views/dashboard/all_posts.html.erb Outdated Show resolved Hide resolved

sagarpreet-chadha mentioned this pull request Dec 6, 2020

[Planning] New Dashboard Implementation Project (Outreachy) #8513

Closed

60 tasks

RuthNjeri changed the title ~~Add All Posts Route~~ Add All Posts Route [/notes] Dec 9, 2020

RuthNjeri changed the title ~~Add All Posts Route [/notes]~~ Add All Posts Route /notes Dec 9, 2020