Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #11331: Created random generator to fetch nodes randomly as cards #11332

Closed
wants to merge 3 commits into from

Conversation

KarishmaVanwari
Copy link
Contributor

Fixes #11331

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

@gitpod-io
Copy link

gitpod-io bot commented Aug 5, 2022

@@ -478,4 +478,34 @@ def span(start, fin)
def range(fin, week)
(fin.to_i - week.weeks.to_i).to_s..(fin.to_i - (week - 1).weeks.to_i).to_s
end

def self.get_recommendations(tag1, tag2)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private (on line 464) does not make singleton methods private. Use private_class_method or private inside a class << self block instead.

Node.where("nid IN (?)", random_content_nids)
end

def self.find_recommended_nodes(tagnames, limit = 10)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private (on line 464) does not make singleton methods private. Use private_class_method or private inside a class << self block instead.

@@ -60,6 +60,11 @@ def show
if !@node.nil? # it's a place page!
@tags = @node.tags
@tags += [Tag.find_by(name: params[:id])] if Tag.find_by(name: params[:id])

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace detected.

@@ -60,6 +60,11 @@ def show
if !@node.nil? # it's a place page!
@tags = @node.tags
@tags += [Tag.find_by(name: params[:id])] if Tag.find_by(name: params[:id])

tag1, tag2 = @node.normal_tags(:followers).includes(:tag).pluck(:name).first(2)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace detected.

@codeclimate
Copy link

codeclimate bot commented Aug 5, 2022

Code Climate has analyzed commit 8ae260f and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category Count
Style 4

View more on Code Climate.

@github-actions
Copy link

github-actions bot commented Aug 5, 2022

This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here:
https://github.com/publiclab/plots2/suites/7689226168/artifacts/321116664

@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #11332 (73ab354) into main (8b97a11) will increase coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head 73ab354 differs from pull request most recent head 1850bf3. Consider uploading reports for the commit 1850bf3 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11332      +/-   ##
==========================================
+ Coverage   82.46%   82.56%   +0.09%     
==========================================
  Files          98       96       -2     
  Lines        5995     5960      -35     
==========================================
- Hits         4944     4921      -23     
+ Misses       1051     1039      -12     
Impacted Files Coverage Δ
app/controllers/wiki_controller.rb 82.97% <100.00%> (+0.12%) ⬆️
app/models/tag.rb 93.60% <100.00%> (+0.30%) ⬆️
app/controllers/home_controller.rb 95.77% <0.00%> (-0.18%) ⬇️
app/models/user.rb 86.49% <0.00%> (-0.15%) ⬇️
app/models/node.rb 91.18% <0.00%> (-0.09%) ⬇️
app/models/comment.rb 77.70% <0.00%> (-0.08%) ⬇️
app/models/answer.rb
app/models/drupal_content_field_image_gallery.rb
app/controllers/comment_controller.rb 83.07% <0.00%> (+0.26%) ⬆️
app/controllers/map_controller.rb 50.33% <0.00%> (+0.33%) ⬆️
... and 1 more

tag1, tag2 = @node.normal_tags(:followers).includes(:tag).pluck(:name).first(2)

# get recommendations
@recommendations = Tag.get_recommendations(tag1, tag2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @KarishmaVanwari, since you want the random cards to be displayed in the dashboard's sidebar shouldn't this be in home_controller?

def dashboard_v2
@title = I18n.t('dashboard._header.dashboard')
# The new dashboard displays the blog and topics list
if current_user
@blog = Tag.find_nodes_by_type('blog', 'note', 1).limit(1).first
# Tags without the blog tag and everything tag to avoid double display
exclude_tids = Tag.where(name: %w(blog everything)).pluck(:tid)
@pagy, @tag_subscriptions = pagy(
TagSelection
.select('tag_selections.tid, term_data.name')
.where(user_id: current_user.id, following: true)
.where.not(tid: exclude_tids)
.joins("INNER JOIN term_data ON tag_selections.tid = term_data.tid")
.order("term_data.activity_timestamp DESC")
)
@trending_tags = trending
render template: 'dashboard_v2/dashboard'
else
redirect_to '/research'
end
end

Please let me know if I'm wrong or missing something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we put it in a reusable named methodin application_controller so it can be used in multiple places?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried placing this piece of code on line 54 in home_conroller but it gives the following error -
NoMethodError in HomeController#dashboard_v2
undefined method normal_tags for nil:NilClass .

@TildaDares @jywarren

@jywarren

This comment was marked as outdated.

@KarishmaVanwari

This comment was marked as outdated.

@@ -55,7 +55,20 @@
</div>
</div>
</div>

<div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we move this into its own partial - to load with <%= render partial: "nodes/random" %> and the partial template would be _random.html.erb.

@@ -60,6 +60,11 @@ def show
if !@node.nil? # it's a place page!
@tags = @node.tags
@tags += [Tag.find_by(name: params[:id])] if Tag.find_by(name: params[:id])

tag1, tag2 = @node.normal_tags(:followers).includes(:tag).pluck(:name).first(2)
Copy link
Member

@jywarren jywarren Aug 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tag1, tag2 = @node.normal_tags(:followers).includes(:tag).pluck(:name).first(2)
puts ">>>>>>>>>>>>>>>>>>>>>>>>>>>"
puts @node.inspect
puts ">>>>>>>>>>>>>>>>>>>>>>>>>>>"
tag1, tag2 = @node.normal_tags(:followers).includes(:tag).pluck(:name).first(2)

@@ -55,7 +55,20 @@
</div>
</div>
</div>

<div>
<% if !@recommendations.nil? %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<% if !@recommendations.nil? %>
<% @recommendations ||= Tag.get_recommendations("water-quality", "air-quality") %>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jywarren, I tried this out, but it still doesn't seem to work as I am unable to find cards as expected on the dashboard.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may be because you don't have nodes with those tags in your local development environment? Can you try tagging a note (not a wiki, i think this is just returning notes, right?) with "water-quality" or "air-quality"? If that doesn't work I will try running this code to debug. No worries!! Did you want to accept this suggestion and commit it?

@jywarren

This comment was marked as off-topic.

@jywarren

This comment was marked as off-topic.

@KarishmaVanwari
Copy link
Contributor Author

Hi @jywarren, I've opened a PR #11380. Thanks!

@jywarren
Copy link
Member

jywarren commented Sep 2, 2022

My apologies for mixing these all up, but i hid the subtopics discussion from this thread to try to clear it up. Looking at the issues here now!

@jywarren
Copy link
Member

jywarren commented Sep 5, 2022

OK, so I had to make some edits to the dashboard code to make it work, but ultimately I found that Tag.get_recommendations("blog", "three") is not returning anything (i used "blog" because that's in the db seeds file so it exists).

So let's debug why that's not working. I think we may want to write a unit test for that code. That will let us test Tag.get_recommendations("blog", "three") in isolation and ensure it returns the right thing. Why don't I write the unit test and you can work on figuring out why it's not returning anything!

@@ -294,4 +294,9 @@ def setup
assert lat.location_tag?
assert lon.location_tag?
end

test 'Tag.get_recommendations' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, here's the unit test. Let's ensure it returns something! We could even test how many nodes it returns, or something else. But most importantly that it returns nodes at all!

Copy link
Member

@jywarren jywarren Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, see below where I made a further suggestion about the unit test: #11332 (comment) (it got collapsed because I accepted the change)

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here:
https://github.com/publiclab/plots2/suites/8142454649/artifacts/353100585

@jywarren
Copy link
Member

jywarren commented Sep 6, 2022

Generally unit tests test individual functions in isolation, not in the context of a big running application all at once. They're useful for finding narrow reasons things may have gone wrong, and that individual pieces function as expected.

By contrast, integration tests, or system tests, are "full-stack" tests which start up the whole application and help us see how things might go wrong when everything is trying to work in synchrony, all at once.

Both types of test -- the narrow ones and the broad ones -- are useful for catching errors. Sometimes both get tripped! But I hope that way of thinking of different kinds of tests makes sense!

test 'Tag.get_recommendations' do
nodes = Tag.get_recommendations("blog", "test")
assert_not_nil nodes
assert nodes.length > 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha! This assertion failed! So, looks like we have been getting an empty array back:

https://github.com/publiclab/plots2/runs/8217990567?check_suite_focus=true#step:5:85

TagTest#test_Tag.get_recommendations (23.30s)
        Expected false to be truthy.
        test/unit/tag_test.rb:301:in `block in <class:TagTest>'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, so now, we can try to fix that. Once we push a fix up as a commit to this PR, the test will start to pass again -- that's actually "Test Driven Development" -- we set up a specific enough test condition (or "assertion") that our test will pass once it's working properly. The test actually precedes the final solution!

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here:
https://github.com/publiclab/plots2/suites/8164595320/artifacts/354647543

@stale
Copy link

stale bot commented Sep 16, 2023

Hi 😄, this issue has been automatically marked as stale because it has not had recent activity. Don't worry you can continue to work on this and ask @publiclab/reviewers to add in-progress label 🎉 . Otherwise, it will be closed if no further activity occurs in 10 days -- but you can always re-open it if you like! 💯 Thank you for your contributions! 🙌 🎈.

@stale stale bot added the stale label Sep 16, 2023
@stale stale bot closed this Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a Random Generator to fetch nodes randomly as cards
3 participants