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.

Sign up for GitHub

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

1756 add button to remove holds on an item #1835

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

1756 add button to remove holds on an item #1835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4999934
v1
RudyOnRails Jan 22, 2025
0ce9363
playing with a minitest
RudyOnRails Jan 22, 2025
acdb78a
test "ending a specific hold on an item"
RudyOnRails Feb 9, 2025
f8158ba
improve test description and add an assertion that ended_at gets set
RudyOnRails Feb 9, 2025
0fab47d
gem lock PLATFORM addition for my old ass x86 darwin19
RudyOnRails Feb 14, 2025
79a89b4
remove original controller test that i started with but abandoned for…
RudyOnRails Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,7 @@ PLATFORMS
arm64-darwin-22
arm64-darwin-23
arm64-darwin-24
x86_64-darwin-19
x86_64-darwin-22
x86_64-darwin-23
x86_64-linux
Expand Down
15 changes: 14 additions & 1 deletion app/controllers/admin/items/holds_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
module Admin
module Items
class HoldsController < BaseController
before_action :set_item
Copy link
Member

Choose a reason for hiding this comment

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

Love to see some refactoring 😄


def index
@item = Item.find(params[:item_id])
holds_scope = @item.holds.ordered_by_position.includes(:member)
@holds =
if params[:inactive]
Expand All @@ -11,6 +12,18 @@ def index
holds_scope.active
end
end

def remove
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@RudyOnRails RudyOnRails Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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:

circulate/app/controllers/admin/items/holds/positions_controller.rb

Lines 4 to 18 in c927219

class PositionsController < BaseController
def update
@hold = @item.active_holds.find(params[:hold_id])
@hold.insert_at(params[:position].to_i)
@holds = @item.active_holds.ordered_by_position.includes(:member)
respond_to do |format|
format.turbo_stream {
render turbo_stream: turbo_stream.replace("holds-list",
render_to_string(partial: "admin/items/holds/list", locals: {holds: @holds}))
}
end
end
end

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!

@hold = @item.holds.find(params[:hold_id])
@hold.update(ended_at: Time.current)
redirect_to admin_item_holds_path(@item), flash: {success: "Hold ended."}, status: :see_other
end

private

def set_item
@item = Item.find(params[:item_id])
end
end
end
end
13 changes: 13 additions & 0 deletions app/views/admin/items/holds/_list.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
<th>Wait time</th>
<th>Expires on</th>
<th class="text-center">Notified?</th>
<th class="text-center">Actions</th>
</thead>
<tbody data-hold-order-target="tbody">
<% @holds.ordered_by_position.each_with_index do |hold, index| %>
Expand All @@ -20,6 +21,18 @@
<td><%= wait_time(hold) %></td>
<td><%= date_with_time_title(hold.expires_at) if hold.expires_at %></td>
<td class="text-center"><%= hold.started? ? "✔️" : "" %></td>
<td class="text-center">
<%= button_to(
admin_item_hold_remove_path(@item, hold),
{class: "btn", method: :patch, data: {turbo_confirm: "Are you sure you want to remove this hold?"}, :title => "Remove Hold"},

) do %>
<%= feather_icon "trash-2" %>
<% end %>
</td>



</tr>
<% end %>
</tbody>
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
resource :loan_history, only: :show
resource :history, only: :show
resources :holds, only: :index do
patch :remove, action: "remove"
scope module: "holds" do
resource :position, only: :update
end
Expand Down
20 changes: 20 additions & 0 deletions test/system/admin/holds_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

TIL about create_list. So useful!

@holds = create_list(:started_hold, 3)
@a_hold = @holds.first
@item = @a_hold.item

# visit the admin item holds page
visit admin_item_holds_path(@item)

# end the hold we're testing and verify the # of active holds has decreased by 1
within "[data-hold-id='#{@a_hold.id}']" do
assert_difference("@item.active_holds.count", -1) do
accept_confirm { click_on "Remove Hold" }
end
end

# confirm the hold we ended now has a truthy ended_at
assert @a_hold.reload.ended_at.present?
end
end
Loading