4175 add request type flag #4271

nathangthomas · 2024-04-12T17:40:44Z

Resolves #4175

Description

Adds a request type flag to index and show pages as well as to the downloadable document so that banks can better see which type of request is being made.

Type of change

New feature (non-breaking change which adds functionality)

This can be manually tested by logging in as a bank user.

  Organization Admin
     Email: org_admin1@example.com
  Password: password!

  User
  Email: user_1@example.com
  Password: password!

Screenshots

Requests-2024-04-02.csv

…s export. Adds filter by request type to index.

…y for good.

app/controllers/partners/family_requests_controller.rb

dorner · 2024-04-12T19:41:04Z

app/services/partners/family_request_create_service.rb

@@ -19,6 +20,7 @@ def call
      return self unless valid?

      request_create_svc = Partners::RequestCreateService.new(
+        request_type: @request_type.to_i,


This service should already know the request type just by whether for_families is true or false.

I feel that defining request_type in this way is more transparent. Would you prefer to add logic in the controller to determine request_type rather than using the instance var?

I'd prefer putting the business logic as far back as possible. The controller is calling a service, which already has to know what kind of request it's creating. We can rely on the service to set the request type.

app/controllers/partners/individuals_requests_controller.rb

app/views/requests/_request_type.html.erb

config/database.yml

spec/models/request_spec.rb

cielf

Tried it on my local. Signed in as verified@example.com, entered a quantity request and got

cielf · 2024-05-16T20:17:47Z

Same effect for individual and child request. (Not at all surprised by that)

nathangthomas · 2024-05-17T13:12:06Z

Well that is embarrassing. I'll take a look at it this afternoon.

nathangthomas · 2024-05-17T20:56:03Z

Seems I either forgot to add or removed request_type from the initializers. I have replaced them and tested locally. Everything should work as expected now.

cielf

Alas, the quantity requests are showing up as "I" now. (Strange -- I think I confirmed them as working correctly before ...)

…man-essentials into 4175-add-request-type-flag

cielf · 2024-05-21T22:50:30Z

There's an issue with setup that's currently blocking me doing functional reviews -- just managing your expectations as to turnaround time.

cielf

LGTM now. @dorner?

addessed,

dorner · 2024-05-24T19:06:02Z

app/services/partners/family_request_create_service.rb

@@ -8,7 +8,7 @@ class FamilyRequestCreateService

    attr_reader :partner_user_id, :comments, :family_requests_attributes, :partner_request

-    def initialize(partner_user_id:, family_requests_attributes:, comments: nil, for_families: false)
+    def initialize(request_type:, partner_user_id:, family_requests_attributes:, comments: nil, for_families: false)


Wait, why was this put back? We agreed that it wouldn't be needed because the service itself knows what type it is just by the current parameters being passed in.

dorner · 2024-05-24T19:06:23Z

Had a question... also, lint is failing now.

dorner · 2024-08-16T19:35:48Z

@nathangthomas looks like this is almost ready to go - can you finish it up?

coalest · 2024-11-29T12:44:49Z

@nathangthomas This PR looks almost done. Could I help carry it across the finish line for you?

cielf · 2024-11-29T23:26:53Z

@coalest It hasn't been touched since May -- please go ahead.

coalest · 2024-12-03T15:53:27Z

A few questions:

Checking out this branch I see that the new request_type flag is blank for all existing requests. Which makes sense, but I'm wondering will this need to be backfilled in production?
I don't understand the point of the for_families parameter for the RequestCreateService service. We pass a value of true sometimes, but then we were doing nothing with it. Can it be removed?
On a similar note, I see that for_families is a column in the partner_requests table but that table doesn't seem to be used anywhere in the application. There isn't even a PartnerRequest model defined. Can this be deleted or does it have important info in production?

cielf · 2024-12-03T16:33:12Z

Unfortunately, we can't backfill the data for this -- there isn't a basis for telling the difference between an individual and a quantity request once it's been saved.

2/3. I expect we'll be able to get rid of those after a bit of investigation, and I've added it to our tech debt inbox. I think it's out of scope for this issue, though.

coalest · 2024-12-10T11:01:12Z

Closing this as #4827 was just merged which was built off of this work. Thanks @nathangthomas for doing the heavy lifting 🎉

cielf · 2024-12-15T19:57:34Z

"I don't understand the point of the for_families parameter for the RequestCreateService service. We pass a value of true sometimes, but then we were doing nothing with it. Can it be removed?"
In discussion in the planning meeting, we think you've confused RequestCreateService and FamilyRequestCreateService, where for_families is in use.

coalest · 2024-12-16T13:42:47Z

The for_families attribute used to be present in both RequestCreateService and FamilyRequestCreateService, but I removed it from `RequestCreateService in this commit: b761496.

If you look at the commit before this request_type PR was merge here, you can see that we pass in a for_families attribute to RequestCreateService from FamilyRequestCreateService, save it as an instance variable, but then do nothing with it. I removed it as I think it is more confusing to leave it there.

Regardless, I don't think we have to worry about this. I could have factored out for_families in the FamilyRequestCreateService, but I left it there so the PR would stay on scope without additional refactoring.

coalest · 2024-12-16T13:45:18Z

Since we're discussing it though, I opened a small PR (internal refactor only) to remove that attribute completely.

nathangthomas and others added 21 commits March 15, 2024 13:31

Adds type enum to request model.

35c4e27

Adds request type enum to request model.

d1911fe

Adds request_type to request.

3007e64

Adds request_type to request.

7ae36b4

Passes request_type to FamilyRequestCreateService.new

a2c79df

Passes request_type to FamilyRequestCreateService.new

7e17565

Passes request_type to RequestCreateService.new

9b5977e

Passes request_type to Request.new.

854cd39

Adds hidden field in order to pass request_type param on submit.

8435518

Adds request type flag to index and show pages as well as the request…

45a9495

…s export. Adds filter by request type to index.

Adds by_request_type scope for filtering by request type.

4791021

Adds request type flag to index and show pages as well as the request…

3866a8f

…s export. Adds filter by request type to index.

Adds by_request_type filter.

8a775ec

Updates flag names and removes flag styles based on feedback from rub…

a037b1c

…y for good.

Adds test for request_type enum.

26afeb4

Removes unnecessary line.

5c2997b

Adds Type to expected_headers.

0cd4451

Adds request_type traits to request factory.

94a1be4

Adds request_type to family_request_create_service_spec args.

42f1f84

Adds request_type to args.

c6e45c8

Merge branch 'main' into 4175-add-request-type-flag

0cfddca

dorner requested changes Apr 12, 2024

View reviewed changes

nathangthomas added 7 commits April 22, 2024 14:02

Pass request_type directly.

67e9d71

Remove request_type partial and enter text directly into reqest_row

d8e9806

Revert changes to database.yml

1e19d30

Revert changes to request_spec.rb

43f7365

Remove request_type

c1792ac

Remove hidden request_type fields.

6441532

Fix merge conflicts / merge remote.

a27b4e6

nathangthomas requested a review from dorner April 24, 2024 14:38

nathangthomas requested a review from dorner May 16, 2024 14:16

Add request_type_label method

3a0e841

nathangthomas requested a review from cielf May 16, 2024 14:38

cielf requested changes May 16, 2024

View reviewed changes

Add request_type to initialize

389e6d9

nathangthomas requested a review from cielf May 17, 2024 20:55

Merge branch 'main' into 4175-add-request-type-flag

c1acb98

cielf previously requested changes May 18, 2024

View reviewed changes

nathangthomas added 2 commits May 21, 2024 10:49

Fix request_type bug

15ecc79

Merge branch '4175-add-request-type-flag' of github.com:unabridged/hu…

9c0572b

…man-essentials into 4175-add-request-type-flag

nathangthomas requested a review from cielf May 21, 2024 17:27

cielf reviewed May 22, 2024

View reviewed changes

dorner requested changes May 24, 2024

View reviewed changes

coalest mentioned this pull request Dec 4, 2024

4175 add request type flag #4827

Merged

coalest closed this Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4175 add request type flag #4271

4175 add request type flag #4271

nathangthomas commented Apr 12, 2024

dorner Apr 12, 2024

nathangthomas Apr 24, 2024

dorner Apr 25, 2024

cielf left a comment

cielf commented May 16, 2024

nathangthomas commented May 17, 2024

nathangthomas commented May 17, 2024

cielf left a comment

cielf commented May 21, 2024

cielf left a comment

dorner May 24, 2024

dorner commented May 24, 2024

dorner commented Aug 16, 2024

coalest commented Nov 29, 2024

cielf commented Nov 29, 2024

coalest commented Dec 3, 2024 •

edited

Loading

cielf commented Dec 3, 2024 •

edited

Loading

coalest commented Dec 10, 2024

cielf commented Dec 15, 2024

coalest commented Dec 16, 2024 •

edited

Loading

coalest commented Dec 16, 2024 •

edited

Loading

4175 add request type flag #4271

4175 add request type flag #4271

Conversation

nathangthomas commented Apr 12, 2024

Description

Type of change

Screenshots

dorner Apr 12, 2024

Choose a reason for hiding this comment

nathangthomas Apr 24, 2024

Choose a reason for hiding this comment

dorner Apr 25, 2024

Choose a reason for hiding this comment

cielf left a comment

Choose a reason for hiding this comment

cielf commented May 16, 2024

nathangthomas commented May 17, 2024

nathangthomas commented May 17, 2024

cielf left a comment

Choose a reason for hiding this comment

cielf commented May 21, 2024

cielf left a comment

Choose a reason for hiding this comment

dorner May 24, 2024

Choose a reason for hiding this comment

dorner commented May 24, 2024

dorner commented Aug 16, 2024

coalest commented Nov 29, 2024

cielf commented Nov 29, 2024

coalest commented Dec 3, 2024 • edited Loading

cielf commented Dec 3, 2024 • edited Loading

coalest commented Dec 10, 2024

cielf commented Dec 15, 2024

coalest commented Dec 16, 2024 • edited Loading

coalest commented Dec 16, 2024 • edited Loading

coalest commented Dec 3, 2024 •

edited

Loading

cielf commented Dec 3, 2024 •

edited

Loading

coalest commented Dec 16, 2024 •

edited

Loading

coalest commented Dec 16, 2024 •

edited

Loading