1756 add button to remove holds on an item #1835
Conversation
| @@ -196,4 +196,24 @@ def setup | |||
| reloaded_hold_ids = all(".item-holds-table tr[data-hold-id]").pluck("data-hold-id") | |||
| assert_equal expected_hold_ids, reloaded_hold_ids | |||
| end | |||
|
|
|||
| test "ending a specific hold on an item" do | |||
| # use factory bot to create a few started holds | |||
| @@ -1,8 +1,9 @@ | |||
| module Admin | |||
| module Items | |||
| class HoldsController < BaseController | |||
| before_action :set_item | |||
| @@ -11,6 +12,18 @@ def index | |||
| holds_scope.active | |||
| end | |||
| end | |||
|
|
|||
| def remove | |||
There was a problem hiding this comment.
I could also see this using the destroy controller action/HTTP DELETE method. I think it's a little better to use one of the default actions whenever possible, although we definitely have our share of custom ones as well in the app.
There was a problem hiding this comment.
That's a good thought! I think originally it was proper if we were going to destroy the hold, but since we decided to just timestamp ended_at I felt something else was appropriate. I considered using the PATCH/create combo but for some reason I didn't; can't remember.
Do you prefer something like this that is more generic?
def update
@hold = @item.holds.find(params[:hold_id])
@hold.update(hold_params)
redirect_to admin_item_holds_path(@item), flash: {success: "Hold updated."}, status: :see_other
end
Additionally - when I was working on this I remembered a team I worked on where we discussed creating new controllers for variations of main resources, for example we could have a single concern EndedHoldsController where we simply POST/create.
Where does your intuition point to?
There was a problem hiding this comment.
I don't feel too strongly to be honest. I'd like to say we have a strong pattern in the app for this kind of thing, but it's probably a bit ad-hoc since we've had a lot of different people/perspectives in the code over the years.
I guess whether the hold is deleted or not depends on the perspective: in the database we're not deleting it, but from the UI perspective it's gone! But if we're to say that we're going to keep the controllers in line with the the database-view, then I agree that this isn't really deleting the hold.
I actually like the new controller option. Now that you mention it, I did something similar to implement reordering of the holds to avoid overloading the update action:
So I guess that's my preference, but TBH this is also totally fine as-is and there are other things to implement. I'll leave it up to you if you want to change this or merge it as-is and move on to the next thing 😄. I appreciate the thought you put into this!
|
I forgot to mention it in the review, but I think the Actions column is a good pattern to use here. |
@jim Thanks for the excellent review! Very appreciated. |
What it does
Adds a new button to the admin/item's holds view so that individual holds may be ended. Originally the ticket discussed removing the hold entirely but after some discussion it was decided to set the
ended_atattribute so our future selves may benefit from the data.Why it is important
This would resolve #1756
UI Change Screenshot
BEFORE
AFTER
Implementation notes
This is my first PR on this project, and my return to coding after taking a year sabbatical, so just a heads-up I
may beam rusty and appreciate and feedback.One thing to confirm is the new "actions" column (see screenshot). In other apps I've worked on in the past we've used this column for a general one-size-fits-all place to put actions. Let me know if there is another solution we'd like to use.