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
Feature/plans api filtering #2293
Feature/plans api filtering #2293
Conversation
…pi, add timestamps, completion to statistics-plans-api
Addresses #2217 |
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 have a few comments for the controller. Feel free to merge yourself after addressing them
|
||
# Filter on list of users | ||
user_ids = extract_param_list(params, "user") | ||
@plans = @plans.where(roles: {user_id: user_ids}) if user_ids.present? |
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.
do we want to include all roles here (e.g. commenter and reviewer)?
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.
Reviewer is no longer a role, But we do probably want to keep this to editors/administrators/creators.
I have added a helper-function to the role model to allow us to extract the numbers which correspond to a bit_value
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.
ah right. forgot we had removed the reviewer role. thanks for adding the helper method
@plans = @plans.where(updated_at: dates_to_range(params,"updated_after","updated_before")) | ||
end | ||
# filter on funder (dmptemplate_id) | ||
template_ids = extract_param_list(params, "template") |
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.
might be more efficient to process template and plan id filtering before the dates
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 think it should be lazy-evaluating, so activerecord should apply all stages of filtering at the same time.
# takes in the params hash and converts to a date-range | ||
def dates_to_range(hash,start,stop) | ||
today = Date.today | ||
start_date = Date.parse(hash.fetch(start, today.prev_month.to_date.to_s)) |
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.
Would be useful to trap any exceptions thrown by the Date.parse
here and either send the error back to the caller or default to a specific date like one month ago or 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.
We should add that in and make it apply both here and in the statistics_controller, as I've copied this method over from there.
@@ -25,7 +25,7 @@ def index | |||
.where(org_id: @user.org_id, published: true) | |||
.where.not(customization_of: nil) | |||
|
|||
Template.published.order(:org_id, :version).each do |temp| | |||
published_templates.order(:org_id, :version).each do |temp| |
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.
oh wow. great catch.
…nto feature/plans_api_filtering
app/models/role.rb
Outdated
# | ||
# Returns [Integer] | ||
def self.bit_values(access) | ||
Role.send(access.to_s + "_condition").split(' in ').last[1...-2].split(',').map(&:to_i) |
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.
Try Role.send(:chained_flags_values, 'access', access)
instead
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 making the changes @xsrust
Small bugfix for templates API (was throwing an error previously)
Added filtering on