Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

AO3-5100 Rework Open Challenges pages #3192

Open
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

hatal175 commented Dec 1, 2017

Issue

https://otwarchive.atlassian.net/browse/AO3-5100

Purpose

This does most of is described in the bug report.

  1. It correctly sorts the 'all open challenge page' - not just soonest to close on top but also merges both types of challenges which previously was not done.
  2. In each individual challenge, it adds a partial filter sidebar (no sorting or choosing challenge type).

I would like to request an opinion on whether the changes are what the author had in mind for this feature. Also an opinion for the changes themselves, maybe with regards to the filtering and their efficiency would be appreciated.

Testing

I've added some basic tests but the bug report describes the feature.

Credit

Tal.
Please use he.

hatal175 added some commits Nov 25, 2017

@@ -44,25 +44,38 @@ def index
params[:sort_column] = "collections.created_at" if !valid_sort_column(params[:sort_column], 'collection')
params[:sort_direction] = 'DESC' if !valid_sort_direction(params[:sort_direction])
sort = params[:sort_column] + " " + params[:sort_direction]
- @collections = Collection.sorted_and_filtered(sort, params[:collection_filters], params[:page])
+ @collections = Collection.sorted_and_filtered(params[:collection_filters], Proc.new { |query| query.order(sort) }, params[:page])
@houndci-bot

houndci-bot Dec 1, 2017

Use proc instead of Proc.new.

- @challenge_collections = (Collection.signup_open("GiftExchange").limit(15) + Collection.signup_open("PromptMeme").limit(15))
+ @challenge_collections = (Collection.signup_open("GiftExchange").includes(:challenge).limit(15) +
+ Collection.signup_open("PromptMeme").includes(:challenge).limit(15)).
+ sort_by { |collection| collection.challenge.signups_close_at }
@houndci-bot

houndci-bot Dec 1, 2017

Align sort_by with (Collection.signup_open("GiftExchange").includes(:challenge).limit(15) + on line 55.

+ params[:collection_filters].delete("challenge_type")
+ params[:collection_filters].delete("closed")
+
+ @challenge_collections = Collection.sorted_and_filtered(params[:collection_filters], Proc.new { |query| query.signup_open(@challenge_type) }, params[:page])
@houndci-bot

houndci-bot Dec 1, 2017

Use proc instead of Proc.new.

# build up the query with scopes based on the options the user specifies
query = Collection.top_level
if !filters[:title].blank?
# we get the matching collections out of autocomplete and use their ids
ids = Collection.autocomplete_lookup(search_param: filters[:title],
- autocomplete_prefix: (filters[:closed].blank? ? "autocomplete_collection_all" : (filters[:closed] ? "autocomplete_collection_closed" : "autocomplete_collection_open"))
- ).map {|result| Collection.id_from_autocomplete(result)}
+ autocomplete_prefix: (filters[:closed].blank? ? "autocomplete_collection_all" : (filters[:closed] ? "autocomplete_collection_closed" : "autocomplete_collection_open"))
@houndci-bot

houndci-bot Dec 1, 2017

Ternary operators must not be nested. Prefer if or else constructs instead.

app/models/collection.rb
- autocomplete_prefix: (filters[:closed].blank? ? "autocomplete_collection_all" : (filters[:closed] ? "autocomplete_collection_closed" : "autocomplete_collection_open"))
- ).map {|result| Collection.id_from_autocomplete(result)}
+ autocomplete_prefix: (filters[:closed].blank? ? "autocomplete_collection_all" : (filters[:closed] ? "autocomplete_collection_closed" : "autocomplete_collection_open"))
+ ).map {|result| Collection.id_from_autocomplete(result)}
@houndci-bot

houndci-bot Dec 1, 2017

Align ) with (.
Closing method call brace must be on the same line as the last argument when opening brace is on the same line as the first argument.
Space between { and | missing.
Space missing inside }.

@@ -66,6 +66,11 @@
select("1", from: "gift_exchange_potential_match_settings_attributes_num_required_fandoms")
end
+When /^I set challenge close time to ([0-9]*) months?$/ do |months|
+ current_date = DateTime.current
@houndci-bot

houndci-bot Dec 1, 2017

Prefer Date or Time over DateTime.

@@ -66,6 +66,11 @@
select("1", from: "gift_exchange_potential_match_settings_attributes_num_required_fandoms")
end
+When /^I set challenge close time to ([0-9]*) months?$/ do |months|
+ current_date = DateTime.current
+ fill_in("Sign-up closes", with: "#{current_date.months_since(months.to_i)}")
@houndci-bot

houndci-bot Dec 1, 2017

Prefer to_s over string interpolation.

@sarken sarken changed the title from Ao3 5100 to AO3-5100 Rework Open Challenges pages Dec 1, 2017

hatal175 added some commits Dec 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment