5309 update contact type selection #5564

afogel · 2024-03-09T17:34:32Z

What github issue is this PR for, if any?

Resolves #5309

What changed, and why?

This code change an existing code dependency library to create a better UX when selecting contact types.

To accomplish this, I refactored an existing stimulus controller to allow for the instantiation a multi-select with option groups. The tomselect library provides all the requested functionality, though there are a few differences in UX from the figma spec (e.g., no checkboxes, once options are selected, they disappear from the dropdown options).

Though not explicitly requested in spec, the solution also provides highlighting to improve UX of search functionality.

This change also removes some custom JS (related to checkboxes) and view logic that seems unnecessary, given that this partial is only used in this form.

How will this affect user permissions?

Volunteer permissions: N/A
Supervisor permissions: N/A
Admin permissions: N/A

This code change is expressly focused on view logic.

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

Since this is a change to view logic, the best testing is either manual QA or e2e tests.

User story for manual QA:
As a volunteer, I should be able to arrive on this form page.
Selecting the dropdown should allow me to see different options, grouped by contact group type.
I should be able to select and remove multiple contact types using the mouse.
I should be able to select and remove multiple contact types using the keyboard.
When I submit this stage of the contact wizard, I should be able to return and see that the contact types have persisted (those I selected are still there).

Screenshots please :)

Feedback please? (optional)

Well scoped ticket, fun to work on. Well done!

…ype_selection

afogel · 2024-03-09T17:42:20Z

app/javascript/controllers/multiple_select_controller.js

+          },
+          item: function(data, escape) {
+            return `
+              <div class="badge rounded-pill primary-bg py-2 px-3 active form-check-label">


custom badges of selected items

afogel · 2024-03-09T17:42:41Z

app/javascript/controllers/multiple_select_controller.js

+          },
+          optgroup_header: function(data, escape) {
+            return `
+              <span class='d-flex optgroup-header'>


make sure that highlighting doesn't mess up spacing of optgroup header

afogel · 2024-03-09T17:44:30Z

app/javascript/controllers/multiple_select_controller.js

+          option: function(data, escape) {
+            return `
+                <div class='d-flex gap-1'>
+                  <span class='d-flex gap-1'>${escape(data.label)}</span>


b/c of a quirk w/ the highlighting implementation in tomselect, using gap-1 and d-flex to keep everything on the same line. My preference exists for maintaining spaces and adding one extra space to the right than removing spaces altogether.

I think this implementation is good enough, though if we choose to remove highlighting altogether, we would likely avoid this being a problem.

afogel · 2024-03-09T17:45:06Z

app/javascript/controllers/multiple_select_controller.js

+          remove_button: {
+            title: 'Remove this item',
+            className: 'text-primary p-1 mx-2 deactive-bg rounded-circle',
+            label: `<i class="lni lni-close p-1"></i>`


custom remove button, as per figma mockups

afogel · 2024-03-09T17:46:30Z

app/views/case_contacts/form/_contact_types.html.erb

-            <div class="form-check checkbox-style mb-1">
-              <%=
-                contact_type_exists = default_checked ? true : @casa_case.contact_types.include?(contact_type)
-                contact_type_exists = false unless controller.controller_name == 'casa_cases'


I'm lacking the context here for what this view logic is doing, but when I grepped the code base, I couldn't find other circumstances in which this partial is reused, so I'm removing.

makes me feel icky to have a call based on controller name in my view, anyway 😬

following up -- I think this may be the context in which an admin is working with the file. this may require additional investigation.

afogel · 2024-03-09T17:47:41Z

app/views/case_contacts/form/_contact_types.html.erb

-      </div>
-    </div>
+  <p>Choose from the available options below by searching or selecting from the dropdown menu.</p>
+  <% last_case = @casa_cases.sort_by(&:created_at).last %>


seems like there was already a convention of storing some of the view logic state in the partial before, so I stuck with it for speed. Would be happy to extract to a presenter or a decorator in order to encapsulate viewlogic here.

afogel · 2024-03-09T17:48:05Z

app/views/case_contacts/form/_contact_types.html.erb

 </div>
-<script type="text/javascript">


dead code once we remove checkboxes

this is still dead code.

afogel · 2024-03-12T12:01:19Z

app/controllers/case_contacts/form_controller.rb

@@ -51,7 +51,7 @@ def update

  def get_cases_and_contact_types
    @casa_cases = policy_scope(current_organization.casa_cases)
-    @casa_cases = @casa_cases.where(id: @case_contact.casa_case_id) if @case_contact.active?
+    @casa_cases = @casa_cases.find(@case_contact.casa_case_id) if @case_contact.active?


this is wrong, will revert

…l/casa into 5309_update_contact_type_selection

bcastillo32 · 2024-04-07T19:25:13Z

Hi @afogel thanks again for jamming on this. Are you still working on this or does it just need review? Happy to push that along if so :)

afogel · 2024-04-07T20:40:42Z

Hi @afogel thanks again for jamming on this. Are you still working on this or does it just need review? Happy to push that along if so :)

this still needs work wrt to tests. I've had less bandwidth over the last bit of time, but the core of the functionality is there.
Most of the remaining painful here is e2e test that are breaking b/c the functionality is no longer selected w/ checkboxes. Moreover, since I'm using tomselect (already in the existing codebase, but doesn't seem to be in e2e tests), the existing testing framework needs some custom helpers to modify the existing tests. In the event someone else can take this over, I'd be fine with it.

I do think there are some scenarios that may be caught by tests which aren't simply visual changes. That said, it's hard to gauge which are which as since most of the breaking tests are just brittle tests that don't catch real bugs.
If my schedule opens up a bit more, I'll try to take it on, but if not, I'd be fine with the rest of the work being taken over.

bcastillo32 · 2024-04-07T22:37:42Z

Hi @afogel thanks again for jamming on this. Are you still working on this or does it just need review? Happy to push that along if so :)

this still needs work wrt to tests. I've had less bandwidth over the last bit of time, but the core of the functionality is there. Most of the remaining painful here is e2e test that are breaking b/c the functionality is no longer selected w/ checkboxes. Moreover, since I'm using tomselect (already in the existing codebase, but doesn't seem to be in e2e tests), the existing testing framework needs some custom helpers to modify the existing tests. In the event someone else can take this over, I'd be fine with it.

I do think there are some scenarios that may be caught by tests which aren't simply visual changes. That said, it's hard to gauge which are which as since most of the breaking tests are just brittle tests that don't catch real bugs. If my schedule opens up a bit more, I'll try to take it on, but if not, I'd be fine with the rest of the work being taken over.

thank you @afogel I'll see what I can do about getting help with tests :)

compwron · 2024-04-09T15:32:37Z

app/models/learning_hour.rb

@@ -52,7 +52,6 @@ def user_org_learning_topic_enable?
 #  id                     :bigint           not null, primary key
 #  duration_hours         :integer          not null
 #  duration_minutes       :integer          not null
-#  learning_type          :integer          default(5)


strange removal?

compwron

Let's fix the tests and prove that it works :)

afogel · 2024-04-10T04:21:30Z

Let's fix the tests and prove that it works :)

I couldn't agree more that the tests should be fixed (though that's not what proves that it works 🙃 ); I'm just limited in the amount of time I have rn. I'll try to turn back to it, but may not be able to finish pushing it over the finish line for a bit.

elasticspoon

Seems solid overall, that being said I don't have that much context. Most of my feedback is minor questions / suggestions so don't mind that there is a lot of it. 🙂

Only parts I would consider blockers would be the highlight styling, potentially unused code and whether or not contact types are required.

elasticspoon · 2024-04-11T20:26:44Z

app/models/case_contact.rb

@@ -27,7 +27,7 @@ class CaseContact < ApplicationRecord
  validate :draft_case_ids_not_empty, if: :active_or_details?

  has_many :case_contact_contact_type
-  has_many :contact_types, through: :case_contact_contact_type, source: :contact_type
+  has_and_belongs_to_many :contact_types, join_table: "case_contact_contact_types"


Are we sure that habtm is more correct than many through? Cause the CaseContactContactType model exists.

In general has_many, :through is preferred over HABTM.

elasticspoon · 2024-04-11T20:30:57Z

app/models/casa_org.rb

+  def contact_types_as_hash_map
+    ContactType.joins(:contact_type_group).where(active: true, contact_type_group: {casa_org: self}).order(:name).map(&:hash_for_multiple_select)
+  end
+


Bit of a nit:
I feel like having hash_for_multiple_select in a different class makes this harder to understand. I'm not sure what the shape of the data is and I need to find where that method lives to find out.

contact_types = query stuff contact_types.map { |type| {hash: stuff}}

I think doing all the work in this method is easier to follow especially since hash_for_multiple_select isnt used anywhere else.

I'll add some Rdoc here, but the hash_for_multiple_select is being used twice now and it acts upon the ContactType.

elasticspoon · 2024-04-11T20:34:56Z

db/schema.rb

changes seem unrelated to PR?

elasticspoon · 2024-04-11T20:35:39Z

app/components/form/multiple_select/item_component.rb

Is this getting used?

elasticspoon · 2024-04-11T20:39:49Z

app/components/form/multiple_select_component.html.erb

+  <template data-multiple-select-target="option">
+    <div class='d-flex gap-1'>
+      <span class='d-flex gap-1'>DATA_LABEL</span>
+      <% if @option_sub_text %>
+        <span class="fst-italic text-muted">DATA_SUB_TEXT</span>
+      <% end %>
+    </div>
+  </template>


Currently, this look real funky with highlighting. You can do something like :

#parent_element_whatever_it_is { div { white-space: break-space; } span + span { margin-inline-start: 1.5rem; } }

and get rid of the gap classes and it will get rid of the weird spacing around the highlights.

elasticspoon · 2024-04-11T20:42:53Z

app/components/form/multiple_select/item_component.html.erb

Is this getting used?

elasticspoon · 2024-04-11T20:52:03Z

app/javascript/controllers/multiple_select_controller.js

The select menu looks great! Two things worth considering (I don't have enough background on the issue to tell how much they matter):

You got rid of the optgroups with group headings (CASA, family, etc). I think that makes it a bit unclear that you can use them to filter the search. like if I search for "CASA" its not clear to me why I get the results I do:

I don't know if there were technical reasons for this but I think the optgroups were nice from a UX perspective.

No clue if this is an improvement from a UX perspective but maybe the input should clear on selecting an item. That way if you do something like "father enter mother enter" you don't have to backspace out what you inputted.

Was part of the original ticket/Figma

I like that, will investigate

That totally fair, kinda assumed it was from original screenshots. My thoughts were that since they want "A user should be able to fuzzy search based on “Contact Type Groups” (see “Editing CASA Organization page in admin portal), ie searching “Family” would still display “Grandparent”

I think a user isn't gonna realize they can search by group without some sort of indication of it. Idk if that makes sense.

elasticspoon · 2024-04-11T20:54:38Z

app/models/casa_case.rb

@@ -43,10 +43,8 @@ class CasaCase < ApplicationRecord
  belongs_to :judge, optional: true
  belongs_to :casa_org
  validates :birth_month_year_youth, presence: true
-  validates_presence_of :casa_case_contact_types, message: ": At least one contact type must be selected", if: :validate_contact_type


Based on the form
it seems like either the validation should remain or that field should not marked with a *.

Moved back to has_many and re-added validation

elasticspoon · 2024-04-11T21:10:28Z

app/views/casa_cases/_form.html.erb

+        <!-- TODO - need to refactor this -->
+        <%= render "case_contacts/form/contact_types", form: form, options: @contact_type_options, selected_items: @contact_type_selected_items %>


This is just a general comment. But like what even is the functionality on this page?

Selecting stuff there doesn't seem to affect future case contacts? What does contact_type selection even do there?

Based on talk to Linda. This should restrict the contact types available for a case. It currently does not seem to be working, if I pick a type of "CASA" for a case, then try to make a case contact for that case I see all the options available.

Also pretty sure this is related to a validation but I can update the case to not have no contact type set.

I also noticed that you can circumvent contact type restrictions but that is unrelated to the PR. #5605

elasticspoon · 2024-04-11T21:24:31Z

app/components/form/multiple_select_component.rb

+    @name = name
+    @options = options.to_json
+    @selected_items = selected_items
+    @option_sub_text = option_sub_text


Nit: maybe render_option_subtext? I initially thought this would be the text itself (I think the text is in the options hash right?).

afogel · 2024-04-12T09:09:42Z

Seems solid overall, that being said I don't have that much context. Most of my feedback is minor questions / suggestions so don't mind that there is a lot of it. 🙂

Only parts I would consider blockers would be the highlight styling, potentially unused code and whether or not contact types are required.

Awesome, appreciate the thorough review! I'll try to poke around over the next bit of time and respond. I've honestly lost much of the context since I worked on the ticket a month ago, and I was also lacking some of the context around use cases, so it'll take a bit of time to go through, but I appreciate the feedback!

compwron · 2024-04-12T18:10:45Z

I am hearing worries from stakeholders Sarah Bosken and Jocie that this will remove the feature of "show how long since a particular contact type has been contacted" which they very much value. Because there should only people on the list who the volunteer is supposed to be contacting (because of the feature that lets us hide non-relevant parties)

elasticspoon · 2024-04-12T18:59:19Z

app/views/casa_cases/_form.html.erb

+        <!-- TODO - need to refactor this -->
+        <%= render "case_contacts/form/contact_types", form: form, options: @contact_type_options, selected_items: @contact_type_selected_items %>


Based on talk to Linda. This should restrict the contact types available for a case. It currently does not seem to be working, if I pick a type of "CASA" for a case, then try to make a case contact for that case I see all the options available.

Also pretty sure this is related to a validation but I can update the case to not have no contact type set.

I also noticed that you can circumvent contact type restrictions but that is unrelated to the PR. #5605

schoork · 2024-04-12T20:58:14Z

I am hearing worries from stakeholders Sarah Bosken and Jocie that this will remove the feature of "show how long since a particular contact type has been contacted" which they very much value. Because there should only people on the list who the volunteer is supposed to be contacting (because of the feature that lets us hide non-relevant parties)

Brandon reached out to some stakeholders (not sure who) at my request to determine what this functionality should look like. Some ways they might want it are harder than others. There are two pieces of functionality that are not related but might seem related to them:

The time since the contact type has been used (ie. about a week)
The showing of only those contact types tied to certain cases

1 is what Brandon was checking on. I think Mallory, Brandon, and I might have a discussion about the best user experience here. There's a thread in the Slack about it so far.

2 I initially pulled out, not quite understanding the context. Yuri rightfully pointed out after discussing with Linda that this is a much used feature. I will be putting it back in. Here's the Slack thread for that convo.

schoork · 2024-04-12T21:37:57Z

Seems solid overall, that being said I don't have that much context. Most of my feedback is minor questions / suggestions so don't mind that there is a lot of it. 🙂
Only parts I would consider blockers would be the highlight styling, potentially unused code and whether or not contact types are required.

Awesome, appreciate the thorough review! I'll try to poke around over the next bit of time and respond. I've honestly lost much of the context since I worked on the ticket a month ago, and I was also lacking some of the context around use cases, so it'll take a bit of time to go through, but I appreciate the feedback!

It really was some cool work you did here @afogel. I did some extra stuff to try to get this over the line quickly, since you were busy. Very good bones here.

afogel · 2024-04-14T07:07:09Z

awesome, thanks! So sorry I'm strapped for time now 😬

bcastillo32 · 2024-05-11T23:12:11Z

@afogel @schoork @elasticspoon - not sure if I ever gave the feedback here but volunteers wanted to keep the logic as is for the "show how long since a particular contact type has been contacted". Is this info what is needed to get this wrapped up? Sorry I have been so busy lately.

schoork · 2024-05-17T16:19:30Z

@afogel @schoork @elasticspoon - not sure if I ever gave the feedback here but volunteers wanted to keep the logic as is for the "show how long since a particular contact type has been contacted". Is this info what is needed to get this wrapped up? Sorry I have been so busy lately.

Sorry, also been busy. Will take a look but pretty sure that's how I implemented it after some initial feedback

schoork · 2024-05-18T15:33:15Z

@bcastillo32 This has the requested functionality.

afogel added 2 commits March 9, 2024 12:16

enable multiselect for contact types

7194d22

Merge remote-tracking branch 'origin/main' into 5309_update_contact_t…

30370e0

…ype_selection

afogel requested review from compwron, FireLemons and littleforest as code owners March 9, 2024 17:34

github-actions bot added javascript for use by Github Labeler to mark pull requests that update Javascript code erb labels Mar 9, 2024

use selectTarget, prettify the indentation

0f6abeb

afogel commented Mar 9, 2024

View reviewed changes

afogel added 2 commits March 9, 2024 12:53

lint fix

9772987

remove spec on contact type alphabetical grouping

59924f2

github-actions bot added ruby Pull requests that update Ruby code Tests! 🎉💖👏 labels Mar 9, 2024

afogel added 2 commits March 9, 2024 12:55

erblint fix

b627f0f

update to handle admin edge case

301f8a6

afogel commented Mar 12, 2024

View reviewed changes

afogel added 8 commits April 6, 2024 13:32

enable multiselect for contact types

a9ee4c2

use selectTarget, prettify the indentation

c2db7dc

lint fix

9bd3b3f

remove spec on contact type alphabetical grouping

7c1cf4b

erblint fix

a486de5

update to handle admin edge case

01cfe27

revert the change re: get_cases_and_contact_types

b97664c

Merge branch '5309_update_contact_type_selection' of github.com:afoge…

bde3651

…l/casa into 5309_update_contact_type_selection

github-actions bot added the dependencies Pull requests that update a dependency file label Apr 6, 2024

afogel added 2 commits April 6, 2024 13:45

undo the find/where change

7bc4105

add annotations etc. from latest pull

a9d4a3f

afogel changed the base branch from main to lint April 6, 2024 13:46

afogel changed the base branch from lint to main April 6, 2024 13:47

compwron reviewed Apr 9, 2024

View reviewed changes

Major modularity changes

c7af3b3

elasticspoon reviewed Apr 11, 2024

View reviewed changes

afogel changed the base branch from main to lint April 12, 2024 09:00

afogel changed the base branch from lint to main April 12, 2024 09:00

elasticspoon requested changes Apr 12, 2024

View reviewed changes

Fixes and reverts

5ccc707

Sam Williams added 4 commits April 17, 2024 08:55

With time subtext

fcc7f7a

Merge branch 'main' into 5309_update_contact_type_selection

9438adf

Fixes and adds tests

34ba654

Formatting fixes

e19b646

schoork merged commit 582f436 into rubyforgood:main May 18, 2024
13 of 14 checks passed

		<!-- TODO - need to refactor this -->
		<%= render "case_contacts/form/contact_types", form: form, options: @contact_type_options, selected_items: @contact_type_selected_items %>

5309 update contact type selection #5564

5309 update contact type selection #5564

Conversation

afogel commented Mar 9, 2024

What github issue is this PR for, if any?

What changed, and why?

How will this affect user permissions?

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

Screenshots please :)

Feedback please? (optional)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcastillo32 commented Apr 7, 2024

afogel commented Apr 7, 2024

bcastillo32 commented Apr 7, 2024

Choose a reason for hiding this comment

compwron left a comment

Choose a reason for hiding this comment

afogel commented Apr 10, 2024

elasticspoon left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticspoon Apr 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afogel commented Apr 12, 2024

compwron commented Apr 12, 2024 • edited

elasticspoon Apr 12, 2024 • edited

Choose a reason for hiding this comment

schoork commented Apr 12, 2024

schoork commented Apr 12, 2024

afogel commented Apr 14, 2024

bcastillo32 commented May 11, 2024

schoork commented May 17, 2024

schoork commented May 18, 2024

elasticspoon left a comment •

edited

elasticspoon Apr 12, 2024 •

edited

compwron commented Apr 12, 2024 •

edited

elasticspoon Apr 12, 2024 •

edited