-
Notifications
You must be signed in to change notification settings - Fork 1
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
MPDX-8496 - Replace hard coded emails with env variables #1227
Conversation
Preview branch generated at https://add-donor-services-email-env.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against eaf5e36 No significant changes found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention the new variables in the readme?
...nents/Contacts/ContactDetails/ContactDetailsHeader/DeleteContactModal/DeleteContactModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and it worked. I have a few recommendations in the comments here and in my previous review.
...nents/Contacts/ContactDetails/ContactDetailsHeader/DeleteContactModal/DeleteContactModal.tsx
Outdated
Show resolved
Hide resolved
...nents/Contacts/ContactDetails/ContactDetailsHeader/DeleteContactModal/DeleteContactModal.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/Organization/Contacts/ContactRow/ContactRow.tsx
Outdated
Show resolved
Hide resolved
I have used the donor account IDs as I couldn't find the partner account ID you mentioned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Code Health Quality Gates: FAILED
Change in average Code Health of affected files: -0.01 (9.59 -> 9.58)
- Declining Code Health: 2 findings(s) 🚩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Code Health Quality Gates: FAILED
Change in average Code Health of affected files: -0.01 (9.59 -> 9.58)
- Declining Code Health: 1 findings(s) 🚩
@@ -1,6 +1,16 @@ | |||
query ContactSource($accountListId: ID!, $contactId: ID!) { | |||
query Contact($accountListId: ID!, $contactId: ID!) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the rename? Honestly ContactSource
seems like a more descriptive name for this operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⬆️ @dr-bizz Did you see this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it because we're getting more info than just the contactSource, we're now getting all the people on the contact
...nents/Contacts/ContactDetails/ContactDetailsHeader/DeleteContactModal/DeleteContactModal.tsx
Show resolved
Hide resolved
...nents/Contacts/ContactDetails/ContactDetailsHeader/DeleteContactModal/DeleteContactModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to approve, but I think it would be good to document the new variables in the readme before merging.
ab74387
to
1f91215
Compare
Description
Remove all hardcoded emails from MPDX and replace them with variables so non-Cru organisations can use their emails without editing the source code.
We don't need to add any new variables to our Amplify as the default values are the ones we use.
This was spotted on Jira task MPDX-8492 and PR #1216
Checklist: