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

[CST] Modifies claim letter downloader to include a new field for a unified display description #19241

Merged
merged 9 commits into from
Nov 12, 2024

Conversation

iandonovan
Copy link
Contributor

@iandonovan iandonovan commented Nov 4, 2024

Summary

  • This work is behind a feature toggle (flipper): YES
  • The Claim Letter Downloader will now add a new field, called display_description, to the rendered letter json. This allows the clients (web and mobile) to display the same, human-readable name for each letter.
  • I'm on BMT2.
  • I am not introducing a new flipper, but in order for the additional claim letters to be rendered, we will have to enable the following two flippers:
  1. cst_include_ddl_5103_letters
  2. cst_include_ddl_sqd_letters

Related issue(s)

Testing done

  • New code is covered by unit tests
  • The Claim Letter downloader did not include a human-readable name with each letter. As a result, the client side was responsible for showing one (if at all). For example, this is the transformation done on vets-website.
  • Separate from the feature flipper, we will know my changes are working my seeing the new display_description field within the claim letter json.
  • When we enable the feature flippers, we will see more claim letters rendered to the client, and these new ones will also include display_description.

What areas of the site does it impact?

This affects the Claim Letters view.

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • Documentation has been updated (link to documentation)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog (if applicable)
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
  • I added a screenshot of the developed feature

Requested Feedback

I was looking around in the mobile app to see their analogous code to the frontend transformation linked above, but did not find any. How does mobile do this? My goal is to get the letter names standardized without the need for changing any mobile code, but I think adding a new field to the json response will necessitate accommodating changes to display that new field in the mobile app. Right?

@iandonovan iandonovan changed the title [CST]Modifies claim letter downloader to include a new field for a unified display description [CST] Modifies claim letter downloader to include a new field for a unified display description Nov 4, 2024
@@ -69,6 +69,7 @@ class ClaimLetters < ApplicationController
end
property :version, type: :string
property :type_description, type: :string
property :display_description, type: :string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm flying blind on this one, I'm not sure if this file is automated or if I need to change it or what.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would go ahead and call this display_name as that is used elsewhere in CST

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After this conversation I am going to reappropriate the type_description, actually. That's what mobile is displaying, so I will simply change that field to be the human-readable name.

Does that sound ok to you?

@@ -6,6 +6,20 @@ module ClaimStatusTool
class ClaimLetterDownloader
FILENAME = 'ClaimLetter'
DEFAULT_ALLOWED_DOCTYPES = %w[184].freeze
DOCTYPE_TO_DISPLAY_DESCRIPTION = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same mapping from the frontend, which also mirrors Julie's table in the initial change request.

@va-vfs-bot va-vfs-bot temporarily deployed to 96224-additional-claim-development-letters/main/main November 4, 2024 17:37 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 96224-additional-claim-development-letters/main/main November 4, 2024 18:43 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 96224-additional-claim-development-letters/main/main November 5, 2024 16:05 Inactive
@iandonovan iandonovan requested a review from jerekshoe November 5, 2024 17:55
@va-vfs-bot va-vfs-bot temporarily deployed to 96224-additional-claim-development-letters/main/main November 6, 2024 16:40 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 96224-additional-claim-development-letters/main/main November 6, 2024 23:10 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 96224-additional-claim-development-letters/main/main November 7, 2024 14:59 Inactive
@iandonovan iandonovan marked this pull request as ready for review November 7, 2024 16:07
@iandonovan iandonovan requested review from a team as code owners November 7, 2024 16:07
Copy link

@dysbo dysbo left a comment

Choose a reason for hiding this comment

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

pretty cool to have this mapping happening in one place

@rmtolmach
Copy link
Contributor

40 commits behind master. I'm updating your branch just in case.

Copy link

Backend-review-group approval confirmed.

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

Successfully merging this pull request may close these issues.

5 participants