5070 - Update banner creation UX

[carrollsa]

What github issue is this PR for, if any?

Resolves #5070

First time using Stimulus. Open to any suggestions, and I'm fairly new at most of this!

What changed, and why?

Adds 2 UX improvements:

• Displays warning message that alternate active banner will be deactivated upon checking Active? on a new or edited banner form if alternate active banner exists.
• Deactivates alternate active banner upon submission if new or edited banner has Active? checked.

How will this affect user permissions?

• Volunteer permissions: No effect
• Supervisor permissions: No effect
• Admin permissions: No effect

How is this tested? (please write tests!) 💖💪

• System spec tests for deactivation of alternate active banner
• View tests for conditional render of red warning
• Helper tests for method to determine initial hidden status for warning message

Screenshots please :)

Feelings gif (optional)

What gif best describes your feeling working on this issue? https://giphy.com/
Not a gif, but sort of this guy because I created this issue, and this felt like a lot of conditional stuff to add for a fairly minor improvement.

Feedback please? (optional)

We are very interested in your feedback! Please give us some :) https://forms.gle/1D5ACNgTs2u9gSdh9

[kcdragon]

Nice work! I have a couple comments/questions

[kcdragon]

What's the benefit of maintaining this state? I think you could look at this.checkboxTarget.checked inside of toggle and not have to worry about maintaining this state.

[carrollsa]

It appears to be unnecessary! Thanks!

[schoork]

Might be worth trying to do this with just the stimulus-reveal-controller. Feels like you could put the action directly on the checkbox.

[carrollsa]

@schoork Good catch! I'm pretty sure I can remove checkbox_controller since the logic for when to show/hide ended up living outside of it.

[carrollsa]

@schoork @kcdragon
I'd like some input on these tests if you can offer it. A few questions:

• Are these necessary? I figured they couldn't hurt, but I don't want to bloat the test suite.
• Should I pretty much copy these into an edit.html.erb_spec.rb file? I have one written, but I wasn't sure if that was necessary, given both new and edit render the same form.
• I didn't seem to have access to the same actions like click_on, fill_in, etc. within the views specs, so I'm passing a banner in with the active field set already to what we want. This doesn't exactly text whether the view is responsive, but it's at least checking the conditions to show/hide the warning. Is there a way I can improve this?

[carrollsa]

Anybody know what might be going on with the docker check?

[littleforest]

You could also consider making this a method:

@org_has_alternate_active_banner = current_organization.has_active_banner?(@banner.id)

# app/models/casa_org

class CasaOrg
  ...

  def has_active_banner?(current_banner_id)
    banners.where(active: true).where.not(id: current_banner_id).exists?
  end
end

[littleforest]

Also if you make it a method, you could probably get away without passing the instance variable around, and just call the method on line 25 of app/views/banners/_form.html.erb

[carrollsa]

@compwron @littleforest @FireLemons

I think this one sort of fell below the pack because it was a draft for so long. Would any of you have time to take a look at this?

[littleforest]

@carrollsa we have actually been talking with the stakeholders about allowing multiple active banners 😬 . They are interested in having an admin-wide banner that shows up to everyone, and allowing smaller supervisor-level banners that would just show to a supervisor's volunteers. You've put in a lot of work on this PR, so I hate to put the kibosh on it, but I'm worried we are going to want to modify this functionality really soon.

[carrollsa]

@littleforest that's a bummer, but I understand. Thanks for letting me know!

[FireLemons]

🥇