Add API for filtering pd_workshops #13663
Conversation
aoby
changed the title
aoby
force-pushed
the
pd-workshop-filters-api
branch
from
5198ee5
to
377e1c6
Compare
| workshops = workshops.start_on_or_after(ensure_date(params[:start])) if params[:start] | ||
| workshops = workshops.start_on_or_before(ensure_date(params[:end])) if params[:end] | ||
| workshops = workshops.where(course: params[:course]) if params[:course] | ||
| workshops = workshops.where(organizer: params[:organizer]) if params[:organizer] |
There was a problem hiding this comment.
should this be organizer_id?
There was a problem hiding this comment.
Yeah, we might as well be explicit. Will update
| # returns [String] the original value | ||
| def ensure_date(date_str) | ||
| # will raise ArgumentError if it's not a valid date string | ||
| DateTime.parse(date_str) |
There was a problem hiding this comment.
If you wanted to online this
DateTime.parse(date_str) && date_str
| @@ -192,7 +192,8 @@ def self.in_state(state) | |||
| when STATE_ENDED | |||
| where.not(started_at: nil).where.not(ended_at: nil) | |||
| else | |||
| raise "Unrecognized state: #{state}" | |||
| raise "Unrecognized state: #{state}" if error_on_bad_state | |||
| none | |||
There was a problem hiding this comment.
If all workshops should be in a good state I'd suggest logging something that should be picked up by HoneyBadger. It might be enough to just log an ERROR
There was a problem hiding this comment.
This is a query argument, not the workshop state. The workshop itself is always in a valid state
There was a problem hiding this comment.
We don't need to know if someone passes a bogus argument to the API
| @@ -103,6 +103,71 @@ class Api::V1::Pd::WorkshopsControllerTest < ::ActionController::TestCase | |||
| assert_equal workshop_in_progress.id, response[0]['id'] | |||
| end | |||
|
|
|||
| # Action: filter | |||
| test 'admins can filter' do | |||
There was a problem hiding this comment.
Teachers and students cannot filter
There was a problem hiding this comment.
It's included in the all_forbidden (see line 526 below). Do you want an explicit test case for this too?
| FakeController.any_instance.stubs(params: @params) | ||
|
|
||
| @user = mock | ||
| @user.expects(admin?: true) | ||
| @user.stubs(admin?: true) |
There was a problem hiding this comment.
You can always just update this attribute instead of stub it. Doesn't really matter
There was a problem hiding this comment.
@user is a mock (see previous line), not a real user.
| test 'filter_workshops with state' do | ||
| expects(:in_state).with('Not Started', error_on_bad_state: false) | ||
| params state: 'Not Started' | ||
| @controller.filter_workshops @workshop_query |
There was a problem hiding this comment.
Assert that you get stuff back?
There was a problem hiding this comment.
Everything is stubbed here so nothing useful is returned. These test cases are verifying that the expected queries are made via. the expects
Or do you mean for the default test on line 74? In that case, with no expectation, any filter will fail the test. I can add a comment explaining this, and/or do some more explicit validation.
There was a problem hiding this comment.
I added comments, and rearranged the test expectations. PTAL
…ate_order. Add CSV download support to workshop filters controller.
|
|
||
| # Orders by the workshop state, in order: Not Started, In Progress, Ended | ||
| # @param :desc [Boolean] optional - when true, sort descending | ||
| def self.order_by_state(desc: false) |
There was a problem hiding this comment.
I'm a little twitchy at all of this raw SQL - I feel like there is a way to write this in ActiveRecord
There was a problem hiding this comment.
Retracting - you can't get around CASE
There was a problem hiding this comment.
I also don't feel great that this logic is duplicated in SQL and in the model, but I couldn't come up with a better solution short of saving the state in the DB and I didn't like that either. This logic is pretty small and unlikely to change so I think it's ok.
| @@ -0,0 +1,33 @@ | |||
| class Api::V1::Pd::WorkshopDownloadSerializer < ActiveModel::Serializer | |||
| attributes :id, :status, :dates, :organizer_name, :organizer_email, :location_address, | |||
| @@ -68,10 +66,12 @@ class FakeController < ::ApplicationController | |||
| expects(:not).with(course: Pd::Workshop::COURSE_CSF) | |||
|
|
|||
| params course: '-csf' | |||
| @controller.load_filtered_ended_workshops | |||
| load_filtered_ended_workshops | |||
| end | |||
|
|
|||
| test 'filter_workshops default' do | |||
There was a problem hiding this comment.
In general you're verifying that the filters you expect to be applied are actually being applied, but not asserting on the final output. Expected?
No description provided.