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.

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

Add Contact Topics (#5388) #5517

Merged
merged 9 commits into from
Mar 15, 2024

Conversation

elasticspoon
Copy link
Collaborator

@elasticspoon elasticspoon commented Feb 9, 2024

Purpose

Fixes #5388

Models

Adds 2 Models, modifies 2 existing models:
image

Why?

I tried an approach where every topic would be a single record, however, that leads to a lot of duplicated information in the database. Saving a details with every record would result in a lot of extra data.

Technically, we could store details and question as single objects and have each ContactTopic reference then, however, that result in issues with cross org changes to those records. Given in production there are 10 Orgs and 44k Cases that optimization did not seem worth it.

Testing

  • casa_org: checks that correct defaults are generated
  • case_contact: checks the creation method works
  • contact_topic: checks that correct defaults are used and invalid values do not pass

Controllers

Added contact_topic controller. Call don't happen directly, happen as nested attributes from orgs
Added policy for changing contact topics: only same org admin can CRUD.
Generation of new topics happen after org creation in controller

Tests

  • contact_topics: test new, edit, create, update and soft delete
  • form: tests update functions correctly during details, notes and final steps
  • case_contacts: checks that new case_contacts copy org topics
  • casa_org: check that topics can be edited, created

policy:

  • only same org admins should be able to change topics

Views

Admin Panel:
image

Delete Modal:
image

Form Details:
image

Form Notes:
image

Tests

Systems tests focused on checking the javascript: making sure the dropdowns, read more/less work

Components

The admin panel called for a kebab menu and a modal. I chose to make them generic and extract them to ViewComponents.

DropdownMenu Component: added a generic drop down menu component to wrap bootstrap behavior. It expects a body of items and have options to change how the button for the dropdown itself appears. The default if no icon is given and label is disabled is a kebab style icon. (I would have prefered to not add the svg but the icon pack did not include that icon)

Modal Component:

  • group: combines a header, body and footer into a working modal. requires only the header or body + footer. needs a open button or link separately to open it.
  • header: generic header with optional icon and text. if given content that will override
  • body: takes text as string or array of strings and wraps in p tags. if given content that will override
  • footer: has a close button, take content which it will render next to the close button
  • open button/link: button/link to open the modal. requires the id of the modal to work. take optional icon. content overrides.

Other Notes

@github-actions github-actions bot added ruby Pull requests that update Ruby code Tests! 🎉💖👏 labels Feb 9, 2024
@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch 2 times, most recently from 0c8965a to 50c304a Compare February 9, 2024 16:52
@elasticspoon elasticspoon changed the title 5500 add contact topic backend Adds CaseContactTopics model and controller portions (#5500) Feb 9, 2024
@elasticspoon elasticspoon marked this pull request as ready for review February 9, 2024 17:03
@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch from 50c304a to 0e91ce3 Compare February 13, 2024 23:20
@bcastillo32
Copy link
Collaborator

@elasticspoon do you need this approved and merged to continue with the original ticket? Let me know and I can help get that done.

@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch from 0e91ce3 to 7331a5d Compare February 14, 2024 03:16
@schoork
Copy link
Collaborator

schoork commented Feb 15, 2024

@elasticspoon I would highly suggest adding in the views for the Casa Org portion of this. That will give you more information about the UX, which should drive the backend as much as possible.

@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch from 7331a5d to 1ff2627 Compare February 18, 2024 02:11
@github-actions github-actions bot added the erb label Feb 18, 2024
@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch 2 times, most recently from fd234e6 to d847305 Compare February 19, 2024 22:20
@elasticspoon
Copy link
Collaborator Author

@schoork I have redone the models and such and added the org views. They are not complete yet but I think they give a general sense of the UX. I could not for the life of me figure out how to properly test policies. So those tests are currently incorrect. This PR has gotten extremely large, so I would love some more input before I move in any direction.

@elasticspoon elasticspoon requested a review from schoork February 19, 2024 22:32
@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch from 51409f0 to 6e216d3 Compare February 21, 2024 02:24
Copy link
Collaborator

@schoork schoork left a comment

Choose a reason for hiding this comment

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

Overall it's looking great, and I can tell you've put a lot of time in this.

db/migrate/20240216013254_add_contact_topics.rb Outdated Show resolved Hide resolved
app/views/contact_topics/_form.html.erb Outdated Show resolved Hide resolved
app/views/casa_org/_contact_topics.html.erb Outdated Show resolved Hide resolved
app/controllers/contact_topics_controller.rb Outdated Show resolved Hide resolved
app/controllers/contact_topics_controller.rb Show resolved Hide resolved
app/views/case_contacts/form/details.html.erb Outdated Show resolved Hide resolved
app/views/case_contacts/form/details.html.erb Outdated Show resolved Hide resolved
app/views/case_contacts/form/details.html.erb Outdated Show resolved Hide resolved
app/views/case_contacts/form/details.html.erb Outdated Show resolved Hide resolved
spec/policies/contact_topic_policy_spec.rb Outdated Show resolved Hide resolved
@elasticspoon
Copy link
Collaborator Author

@schoork I'll fix the stuff you mentioned. I didn't actually you realize you would go straight to UX tests. Most of the view stuff is pretty incomplete because I didn't want to build on models that I was unsure about.

@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch from 886328e to bdb0390 Compare March 1, 2024 05:17
app/components/modal_component.html.erb Outdated Show resolved Hide resolved
app/components/modal_component.html.erb Outdated Show resolved Hide resolved
app/components/modal_component.html.erb Outdated Show resolved Hide resolved
app/controllers/casa_cases_controller.rb Outdated Show resolved Hide resolved
app/controllers/case_contacts_controller.rb Outdated Show resolved Hide resolved
app/views/case_contacts/form/_contact_topic_notes.html.erb Outdated Show resolved Hide resolved
app/views/case_contacts/form/details.html.erb Show resolved Hide resolved
app/models/contact_topic_answer.rb Show resolved Hide resolved
app/models/contact_topic_answer.rb Show resolved Hide resolved
@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch 5 times, most recently from d27d176 to 8b11a6e Compare March 4, 2024 20:42
fix: chevron respect select status

chore: removes a TODO

refactor: modal view component into subcomponents
adds tests

refactor: kebab to generic dropdown menu

fix: unsafe save

fix: view changes
@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch from 8b11a6e to daf7fea Compare March 4, 2024 20:47
@elasticspoon elasticspoon requested a review from schoork March 4, 2024 21:27
After talking with PMs alphabetical scoping doesn't make sense.

Topics should be set in the order they are on organization, maybe
in the future they should even be swappable in order.
@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch from 99151a8 to 897d5f7 Compare March 8, 2024 02:29
@elasticspoon
Copy link
Collaborator Author

After some clarification on how topics are represented in reports I went back on the idea of sorting topics in alphabetical order. Casa wants them in a particular order for the reports which is not alphabetical.

Depending on how it goes there might be need to implement reordering of topics. That said, apparently topics rarely change so that should not be much of an issue.

<div class="card-style-1 pl-25 mb-10">
<h4 class="mb-20 details__topics-label">Court report <%= "topic".pluralize(@case_contact.contact_topic_answers.count) %> <span class="content-1">(optional)</span></label></h4>
<div class="">
<%= form.fields_for(:contact_topic_answers) do |f| %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

These change order when you move through and back in the form. A little confusing. Can we do alphabetical?

Comment on lines +10 to +16
def body_content
return content if content.present?

Array.wrap(@text).map do |text|
content_tag :p, text
end.join.html_safe
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems pretty restrictive to what can go in to the body. Why not just insert what the content is and do something like this when rendering...?

<% component.with_body do %>
  <p>
    This topic and its related questions...
  </p>
  <p>
    This will not affect case contacts that...
  </p>
<% end %>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can do that, currently if you provide a block it overrides and text is optional.

It's just if you provide text as a string or array of strings. It gets wrapped in ptags.

it "renders the body with content" do

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you're using this anymore. Is there a reason to keep this?
Screenshot 2024-03-09 at 11 18 51 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea was to provide two different options to open the model. As a button tag and a link tag, they format slightly differently.

In most situations you would want to use a button because it's semantically correct. But in some situations you're forced to use a link like in my case. So I wanted to have both.

@schoork
Copy link
Collaborator

schoork commented Mar 9, 2024

This is a really flaky test. I think I wrote it. And I'm fine with you removing it. Maybe change it to an xit so we know to re-write it in the future.
Screenshot 2024-03-09 at 11 24 36 AM

@schoork
Copy link
Collaborator

schoork commented Mar 9, 2024

@elasticspoon This looks really good! A few extra notes. Nothing blocking that I can see. Ready when you are to get this across the line.

@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch 2 times, most recently from f4ee194 to 1874ee5 Compare March 10, 2024 02:55
fix: order answers based on topics

fix: set flaky test pending

fix: remove TODOs

fix: lint
@elasticspoon elasticspoon force-pushed the 5500-add-contact-topic-backend branch from 1874ee5 to a822a4c Compare March 10, 2024 05:47
@elasticspoon
Copy link
Collaborator Author

@schoork Cleaned up the remaining comments. I think we are good to go with this part. I will work on the report generation in a new PR.

@schoork schoork merged commit f848255 into rubyforgood:main Mar 15, 2024
13 of 14 checks passed
@elasticspoon elasticspoon deleted the 5500-add-contact-topic-backend branch March 15, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file erb javascript for use by Github Labeler to mark pull requests that update Javascript code ruby Pull requests that update Ruby code Tests! 🎉💖👏
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "4. Court report topics (optional)" card/section to case contact form
4 participants