Skip to content

Conversation

GilbertCherrie
Copy link
Member

@GilbertCherrie GilbertCherrie commented Sep 17, 2025

This pr fixes an issue with the tagging form. Also, this pr removes the checkboxes from the tagging form table.

Before:
Select multiple service catalog items then go to the Policy / Edit Tags form. Click cancel on the form. Then when selecting a single catalog item and going to the Edit Tags form from the summary screen still shows the multiple items being tagged on the form.
Screenshot 2025-09-17 at 12 01 12 PM

After:
Only the selected service catalog item from the summary screen will show up on the tagging form.
Screenshot 2025-09-17 at 12 02 53 PM

@GilbertCherrie GilbertCherrie requested a review from a team as a code owner September 17, 2025 15:53
@GilbertCherrie GilbertCherrie force-pushed the fix_single_tag branch 2 times, most recently from 201f0e9 to c42f2d1 Compare September 17, 2025 15:54
@GilbertCherrie GilbertCherrie requested review from Fryguy and elsamaryv and removed request for a team, Fryguy and elsamaryv September 17, 2025 16:03
@GilbertCherrie GilbertCherrie requested review from elsamaryv and a team September 17, 2025 16:22
@GilbertCherrie GilbertCherrie force-pushed the fix_single_tag branch 4 times, most recently from 0233b99 to 6721a36 Compare September 17, 2025 17:58
@Fryguy
Copy link
Member

Fryguy commented Sep 17, 2025

@elsamaryv Please review.

@GilbertCherrie GilbertCherrie force-pushed the fix_single_tag branch 2 times, most recently from 5821d47 to 867a78e Compare September 18, 2025 14:53
@GilbertCherrie
Copy link
Member Author

GilbertCherrie commented Sep 24, 2025

@elsamaryv please review and test again when you have time

Comment on lines +23 to +29
params.each do |var, val|
if var.starts_with?("check_")
has_no_check = false
else
next
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Simpler Ruby

Suggested change
params.each do |var, val|
if var.starts_with?("check_")
has_no_check = false
else
next
end
end
has_no_check = params.none? { |var, _val| var.starts_with?("check_") }

also, then you don't have to have line 13 anymore because it will be set here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fryguy I tried this out and hit an error on .none?

Copy link
Member

@Fryguy Fryguy Sep 30, 2025

Choose a reason for hiding this comment

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

Oh then use ! with .any?

Though I'm surprised .none? isn't working since I thought that's in active support

Copy link
Member

Choose a reason for hiding this comment

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

Oh I know the problem - the params hash isn't a "normal" hash. Try this:

Suggested change
params.each do |var, val|
if var.starts_with?("check_")
has_no_check = false
else
next
end
end
has_no_check = params.each_key.none? { |var| var.start_with?("check_") }

@GilbertCherrie In doing this, I remembered that starts_with? isn't a Ruby method, but should have been start_with?. That tells me this code path isn't tested (neither manually nor with automated tests). Please add specs covering this if possible otherwise, manually test.

Copy link
Contributor

@elsamaryv elsamaryv left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants