Skip to content
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

@joeyAghion => Ability to sort on the index views #migration #164

Merged
merged 4 commits into from
Feb 20, 2018

Conversation

sweir27
Copy link
Contributor

@sweir27 sweir27 commented Feb 16, 2018

cc @nicoleyeo

This PR adds the ability to sort on the submissions, offers, and consignments index views.

Each sortable header is a link to a specified sort field/direction.

I also took the opportunity to do a little cleanup in the controllers to use decent_exposure instead of setting instance variables. I've really come to like this pattern! (cc @jonallured). Not everything is switched over, but I'm updating things as they are relevant.

offers-sort

Migration

Submission.pluck(:id).each do |sub_id|
  Submission.reset_counters(sub_id, :offers)
end; nil

@@ -3,11 +3,14 @@ class ConsignmentsController < ApplicationController
include GraphqlHelper

before_action :set_consignment, only: [:show, :edit, :update]
before_action :set_pagination_params, only: [:index]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll notice this in a bunch of controllers-- instead of setting the @page and @size variables in the base controller, we now use decent_exposure and they are lazy-loaded.

(Note that there's probably a way with kaminari to paginate without having to set these at all, but since we're using the kinetic UI it does require a page and size to be passed in still, so we must set them).

@@ -41,7 +41,7 @@ class Offer < ApplicationRecord
].freeze

belongs_to :partner_submission
belongs_to :submission
belongs_to :submission, counter_cache: true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will keep an offers_count on a submission.

@@ -3,6 +3,6 @@
<div class='overview-section-title--inline'>
Consignment
</div>
<%= render 'admin/consignments/consignment', consignment: consignment %>
<%= render 'admin/consignments/consignment', consignment: consignment, artist: artist %>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little refactor here-- the partials now require an individual artist to be passed in instead of inferring it from the @artist_details hash.

@@ -72,7 +74,9 @@
</div>
<div class='row col-sm-12'>
<div class='list-group'>
<%= render partial: 'admin/consignments/consignment', collection: consignments %>
<% consignments.each do |consignment| %>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we are now passing an artist per-item, I had to break this out.

On a side note, I actually like this a little better since it's easier to search for where partials are being used.

@sweir27 sweir27 changed the title @joeyAghion => Ability to sort on the index views @joeyAghion => Ability to sort on the index views #migration Feb 16, 2018
@jonallured
Copy link
Member

Yay!! Excited to see decent_exposure being used at Artsy!! 🎉

Reading through this PR, one thing jumps out at me - the length of the consignments exposure in ConsignmentsController. I wonder if there's a refactor here to extract an object. I see a phrase here, matching consignments, that I'm guessing could be good to pull out as a domain concept. I would love to pair on this, but my quick back-of-the-napkin idea is something more like this:

expose(:matching_consignments) do
  MatchingConsignments.find(params, @page, @size)
end

And then pull all the business logic from the existing exposure there. That would mean those nice controller specs you've written could be moved over there as well. Open to other ideas, my main point is simply that the controller is doing too much work here, knows too much. I think extracting an object that it can collaborate with would be a big win!

matching_consignments.joins(:accepted_offer).reorder("#{sort} #{direction}")
else
matching_consignments.reorder("#{sort} #{direction}")
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arbitrary indentation makes me crazy, even if rubocop likes it. Consider disabling the offense, or switching its mode to support the line-broken style:

x =
  if ...

@joeyAghion
Copy link
Contributor

I'm OK w/this when you want to merge...

@sweir27 sweir27 merged commit a398df4 into artsy:master Feb 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants