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

[Improvement] Add Mail Lost Option as Replacement Reason #340

Merged
merged 9 commits into from
Jun 15, 2024

Conversation

kj3moraes
Copy link
Collaborator

Notion ticket link

Add MAIL_LOST as a reason for replacement

Implementation description

  • made a modification to the prisma schema to add MAIL_LOST as an option
  • made a modification to the reason replacement form to add mail lost as a radio button
  • made a modification to the GQL schema to add MAIL_LOST as an option

Notes

  • I had to add a migration using prisma migrate deploy that manually added another migration. I don't know if this is supposed to be done because there was only an init migration before me. Please check that this is valid
  • For LOST,STOLEN and OTHER there are additional fields for information. Will I need to add additional fields for MAIL_LOST
  • Loom demoing it working

Checklist

  • My PR name is descriptive, is in imperative tense and starts with one of the following: [Feature],[Improvement] or [Fix],
  • I have run the appropriate linter(s)
  • I have requested a review from the RCD team on GitHub, or specific people who are associated with this ticket

@kj3moraes
Copy link
Collaborator Author

kj3moraes commented Jan 23, 2024

It says that the Prettier linting has failed but I just ran it on my machine and this is what I get
CleanShot 2024-01-23 at 11 00 00

Furthermore on the general linting command I get this
CleanShot 2024-01-23 at 11 01 46

Copy link
Member

@sherryhli sherryhli left a comment

Choose a reason for hiding this comment

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

Re: lint error
Seems related to the changes in the yarn.lock file - reverting back to the current version in staging fixes it. See #342

I had to add a migration using prisma migrate deploy that manually added another migration. I don't know if this is supposed to be done because there was only an init migration before me. Please check that this is valid

Migration looks good to me! For future reference the procedure is to run prisma migrate dev after any schema change.

For LOST,STOLEN and OTHER there are additional fields for information. Will I need to add additional fields for MAIL_LOST

Do you mean fields like lostTimestamp, stolenPoliceFileNumber, etc. in the schema? Not necessary if that's what you're referring to.

onChange={() => {
setFieldValue('paymentInformation.processingFee', '0');
}}
>
{'Mail Lost'}
</Radio>
<Radio
value={'OTHER'}
Copy link
Member

Choose a reason for hiding this comment

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

Just occurred to me that we need to discuss how to handle existing replacement applications that were created after #325, which mapped the OTHER enum to the displayed value of 'Mail Lost'. Once this is PR merged, any application using OTHER will show as 'Other' even if they were supposed to be 'Mail Lost'.

Let's discuss in Wednesday's meeting, can you please hold off on merging this PR (at least the frontend portion)?

@kj3moraes kj3moraes force-pushed the km-mail-lost-option branch from e85a59d to be5f9d6 Compare June 15, 2024 22:52
@ChinemeremChigbo ChinemeremChigbo merged commit cb36e24 into staging Jun 15, 2024
1 check passed
@ChinemeremChigbo ChinemeremChigbo deleted the km-mail-lost-option branch June 15, 2024 23:21
leogjhuang pushed a commit that referenced this pull request Jun 21, 2024
* initial commit for the mail lost option

* changed the processing fee for mail lost

* made the migrations for mail lost

* reverted yarn lock to staging

* initial commit for the mail lost option

* changed the processing fee for mail lost

* made the migrations for mail lost

* reverted yarn lock to staging

---------

Co-authored-by: Chinemerem <[email protected]>
leogjhuang added a commit that referenced this pull request Jun 21, 2024
* Add Sharujan to employees.ts (#360)

* [Improvement] Add Mail Lost Option as Replacement Reason (#340)

* initial commit for the mail lost option

* changed the processing fee for mail lost

* made the migrations for mail lost

* reverted yarn lock to staging

* initial commit for the mail lost option

* changed the processing fee for mail lost

* made the migrations for mail lost

* reverted yarn lock to staging

---------

Co-authored-by: Chinemerem <[email protected]>

* Add $200 donation option (#355)

* Add $200 donation option

* Add $200 donation option

* Update .env.sample with $200 donation product id

---------

Co-authored-by: Chinemerem <[email protected]>

* Change address from unit 842 to 968 (#361)

* [Fix] Add AMEX as a second payment option (#362)

* [Feature] Add receipt numbers to application reports (#363)

* [Misc] Update README.md (#364)

---------

Co-authored-by: Chinemerem <[email protected]>
Co-authored-by: Keane Moraes <[email protected]>
Co-authored-by: Sherry Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants