-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
@@ -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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 [email protected]? # it's a place page! | |||
@tags = @node.tags | |||
@tags += [Tag.find_by(name: params[:id])] if Tag.find_by(name: params[:id]) | |||
|
There was a problem hiding this comment.
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 [email protected]? # 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) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace detected.
Code Climate has analyzed commit 8ae260f and detected 4 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
Codecov Report
@@ 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
|
tag1, tag2 = @node.normal_tags(:followers).includes(:tag).pluck(:name).first(2) | ||
|
||
# get recommendations | ||
@recommendations = Tag.get_recommendations(tag1, tag2) |
There was a problem hiding this comment.
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
?
plots2/app/controllers/home_controller.rb
Lines 39 to 59 in ca319fc
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@@ -55,7 +55,20 @@ | |||
</div> | |||
</div> | |||
</div> | |||
|
|||
<div> |
There was a problem hiding this comment.
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 [email protected]? # 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 [email protected]? %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<% if !@recommendations.nil? %> | |
<% @recommendations ||= Tag.get_recommendations("water-quality", "air-quality") %> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
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! |
OK, so I had to make some edits to the dashboard code to make it work, but ultimately I found that 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 |
@@ -294,4 +294,9 @@ def setup | |||
assert lat.location_tag? | |||
assert lon.location_tag? | |||
end | |||
|
|||
test 'Tag.get_recommendations' do |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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)
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
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 |
There was a problem hiding this comment.
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>'
There was a problem hiding this comment.
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!
This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here: |
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 |
Fixes #11331
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
rake test
@publiclab/reviewers
for help, in a comment below