-
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 #7831. Fixed filter spam tags in app/controllers/stats_controller.rb #7851
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7851 +/- ##
=======================================
Coverage 82.05% 82.05%
=======================================
Files 97 97
Lines 5645 5645
=======================================
Hits 4632 4632
Misses 1013 1013
|
app/controllers/stats_controller.rb
Outdated
@@ -1,7 +1,7 @@ | |||
class StatsController < ApplicationController | |||
def subscriptions | |||
@tags = {} | |||
TagSelection.where(following: true).each do |tag| | |||
TagSelection.joins(:node_tags).where(following: true).each do |tag| |
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.
Thanks for working on this so quick @dongskyler 🎉 ...we want this to become TagSelection.where(following: true) .joins(:node_tags)
as explained in this comment #7831 (comment)
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’m happy to work on it shortly.
But to be clear, this calls for another branch and another PR, and it will void my previous PR. This was not what I modified in my previous branch.
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.
Not really...we are moving the 'join' after the 'where'..
in the previous PR we had TagSelection.joins(:node_tags).where(following: true)
and before that we did not have the join to node_tags...makes sense?
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 misunderstood what you instructed me to do.
I have updated the branch and both tests have passed.
Please review.
hey, I can't see the changes on the files changes maybe you deleted that commit..kindly check. Thanks |
I'm sorry I didn't fix the issue right away. |
No worries...thanks for woking on this @dongskyler |
Fixes #7831. (<=== 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/reviewers
for help, in a comment belowIf tests do fail, click on the red
X
to learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
@cesswairimu
Thanks!