Implement questions answered vs questions asked #4256
Conversation
ghost
assigned 
Generated by 🚫 Danger |
|
Hi! Just a hint - we can ignore the CodeClimate things, but sometimes they're really helpful suggestions. You can read them in detail and how to resolve them here: https://codeclimate.com/github/publiclab/plots2/pull/4256 |
| public | ||
|
|
||
| def index | ||
| if params[:period] |
There was a problem hiding this comment.
This might be a good one to cache daily! I think esp. with collecting a year's worth of content each page load.
cesswairimu
force-pushed
the
stats-on-questions
branch
7 times, most recently
from
946d3f0 to
0bdc26a
Compare
|
|
cesswairimu
force-pushed
the
stats-on-questions
branch
3 times, most recently
from
dc417cf to
2d477d9
Compare
|
Hey @gauravano @SidharthBansal @jywarren Kindly take a look and any suggestions on how I can best cache the stats here would be great. Thanks. |
|
As @jywarren mentioned we could cache for a day #4256 (comment) we also cache home page data so you can see home_controller for reference. Let me know if you need help. Thanks! |
|
Hey @jywarren I did some refactoring and removed the whole bunch of instance variables. Does the caching I did on the questions helper method make sense? Thanks |
app/helpers/questions_helper.rb
Outdated
| SORTING_OPTIONS | ||
| end | ||
|
|
||
| def cache_quiz_stats |
There was a problem hiding this comment.
This works, but then people coding do have to remember to use the "cached version". Another way would be to build this inside the filtering() method itself, so all calls to that are filtered. If people really want the non-filtered version, they can manually flush it with... i forget but something like Rails.cache.flush(...). Make sense? This is exciting!
There was a problem hiding this comment.
Yes it does thanks, on it
cesswairimu
force-pushed
the
stats-on-questions
branch
from
2d477d9 to
be99757
Compare
cesswairimu
changed the title
|
@jywarren this is ready @publiclab/reviewers this is ready kindly review |
cesswairimu
force-pushed
the
stats-on-questions
branch
from
be99757 to
715ccd4
Compare
app/helpers/questions_helper.rb
Outdated
| module QuestionsHelper | ||
| SORTING_OPTIONS = %w(All Week Month Year).freeze | ||
|
|
||
| def filtering(period) |
There was a problem hiding this comment.
This looks good but perhaps it could have a more descriptive name -- something describing how it's related to stats, or Q&A? Thanks!
There was a problem hiding this comment.
Also, I'd love to see a simple test for these methods. Where would that live?
There was a problem hiding this comment.
Let me try and figure out where the tests will go. Thanks
app/helpers/questions_helper.rb
Outdated
|
|
||
| if period == 'All' | ||
| Rails.cache.fetch("all_stats", expires_in: 1.days) do | ||
| @asked = Node.questions.to_a.size |
There was a problem hiding this comment.
I think if you use Node.questions.count it should do a more efficient query, just a SQL SIZE query instead of fetching all the full records and counting them. Maybe same on line below! 👍
There was a problem hiding this comment.
@jywarren tried .count was not working fine because its a hash returning results like so {82=>1, 85=>2, 86=>2, 87=>1}. Using .length instead. hope that's a better query?
app/views/questions/index.html.erb
Outdated
| </div> | ||
| <div class="col-md-4"> | ||
| <%= form_tag request.url, :method => 'get' do %> | ||
| <%= select_tag :period, options_for_select(options),onchange: "this.form.submit();", include_blank: "Filter stats", class: "form-control"%> |
There was a problem hiding this comment.
Ooh this is cool, could you share a screenshot? Thanks!
There was a problem hiding this comment.
@jywarren a screenshot is ☝️ above. Thanks for the feedback.
|
Regarding the question of where statistics code "home base" is - would it fit in a model, so it's testable via unit tests? But open to ideas and brainstorming. Maybe ask @gauravano or @SidharthBansal for input on this as well! |
cesswairimu
force-pushed
the
stats-on-questions
branch
from
07418b5 to
a7e0cd7
Compare
cesswairimu
force-pushed
the
stats-on-questions
branch
2 times, most recently
from
a8234a2 to
cbf49d1
Compare
cesswairimu
force-pushed
the
stats-on-questions
branch
from
cbf49d1 to
e9fd267
Compare
|
@jywarren I implemented the tests and changed the method name to a better name and improved the queries. Please take a look. |
|
|
Actually it's more broad - that is, should we have a central location where we keep much of the statistics code, as if in a model, or should we be OK with some appearing in controllers, some in helpers... i'm just trying to think critically about how we're doing overall code organization. We don't have to come up with a final solution right now! Thanks! |
|
This is great. Merging!!! |
* implement questions answered vs questions asked * Refactoring the questions stats * caching questions stats * test question helper
Fixes #4137
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!