Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
63 changes: 51 additions & 12 deletions app/controllers/partners/family_requests_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ def new
end

def create
family_requests_attributes = build_family_requests_attributes(params)
begin
family_requests_attributes = build_family_requests_attributes(params)
rescue ArgumentError => e
Copy link
Collaborator

Choose a reason for hiding this comment

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

ArgumentError is a built-in Ruby exception that shouldn't be used for business logic validation. Probably best to create your own error class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Where would be the best place for the error class?

The file that looks most like is app/events/inventory_error.rb, so something like app/events/item_visibility_error.rb?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're moving this to the service class, you can define the error inside the class itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After looking at the service class, I felt it made more sense to just use the error handling that was already present. If you'd like me to create a custom error class instead just let me know

redirect_to new_partners_family_request_path, error: e
return
end

create_service = Partners::FamilyRequestCreateService.new(
partner_user_id: current_user.id,
Expand All @@ -28,7 +33,7 @@ def create
if create_service.errors.none?
redirect_to partners_request_path(create_service.partner_request), notice: "Requested items successfully!"
else
redirect_to new_partners_family_request_path, error: "Request failed! #{create_service.errors.map { |error| error.message.to_s }}}"
redirect_to new_partners_family_request_path, error: "Request failed! #{create_service.errors.map { |error| error.message.to_s }}"
end
end

Expand All @@ -55,21 +60,55 @@ def validate
private

def build_family_requests_attributes(params)
children_ids = []
child_ids = params.keys.grep(/^child-/).map { |key| key.split('-').last }

params.each do |key, _|
is_child, id = key.split('-')
if is_child == 'child'
children_ids << id
end
end
validate_visible_items!(child_ids)

children = current_partner.children.where(id: children_ids).joins(:requested_items).select('children.*', :item_id)
children_grouped_by_item_id = current_partner
.children
.where(id: child_ids)
.joins(:requested_items)
.select('children.*', 'items.id as item_id',
'items.name as item_name',
'items.visible_to_partners')
.group_by(&:item_id)

children_grouped_by_item_id = children.group_by(&:item_id)
children_grouped_by_item_id.map do |item_id, item_requested_children|
{ item_id: item_id, person_count: item_requested_children.size, children: item_requested_children }
{
item_id: item_id,
person_count: item_requested_children.size,
children: item_requested_children
}
end
end

def validate_visible_items!(child_ids)
invisible_item_requests = current_partner
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can reuse most if not all of the query that was created earlier. You can pass the query object into this method and just add the changes you need onto it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having said that, all of this is starting to seem very heavy for controller logic. I think we should move most of this to the service class.

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 has been moved to the service class.

.children
.where(id: child_ids)
.joins(:requested_items)
.where(items: { visible_to_partners: false })
.select(
'children.first_name',
'children.last_name',
'items.id as item_id',
'items.name as item_name',
'items.visible_to_partners'
)
.group_by(&:item_id)

return if invisible_item_requests.empty?

# raise an exception if there are requests
# to items that aren't visible to partners
item_errors = invisible_item_requests.map do |_, children|
children_names = children.map { |c| "#{c.first_name} #{c.last_name}" }.join(", ")
item_name = children.first.item_name

"\"#{item_name}\" requested for #{children_names} is not currently available for request."
end

raise ArgumentError.new item_errors.join(", ")
end
end
end
2 changes: 1 addition & 1 deletion app/views/layouts/partners/application.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
<div class="content-wrapper">
<% flash.each do |key, value| %>
<div class="<%= flash_class(key) %> alert-dismissible fade show" role="alert">
<a href="#" class="close btn-close" data-bs-dismiss="alert" aria-label="Close" style="text-decoration: none;"><%= fa_icon('times') %></a>
<a href="#" class="close btn-close" data-bs-dismiss="alert" aria-label="Close" style="text-decoration: none;"></a>
<%= sanitize(value, tags: %w(ul li)) %>
</div>
<% end %>
Expand Down
16 changes: 16 additions & 0 deletions spec/requests/partners/family_requests_requests_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,22 @@
expect(subject).to redirect_to(partners_requests_path)
end

it "does not allow requesting non-visible items" do
partner.update!(status: :approved)

# update child item to not be visible to partners
i = Item.first
i.update!(visible_to_partners: false)

subject

child_with_unavailable_item = children[0]

expected = "\"#{i.name}\" requested for #{child_with_unavailable_item.first_name} #{child_with_unavailable_item.last_name} is not currently available for request."

expect(response.request.flash[:error].message).to eql expected
end

it "submits the request" do
partner.update!(status: :approved)

Expand Down