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.

Sign up for GitHub

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

Jump to bottom

Proponent Confirm Email Updates #150

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Proponent Confirm Email Updates #150

merged 1 commit into from
Sep 11, 2024

Conversation

sanjaytkbabu
Copy link
Contributor

@sanjaytkbabu sanjaytkbabu commented Sep 5, 2024 •
edited
Loading

Description

PADS-282
Email template updated
Enquiry Description added to the email template
html query parameter added to email endpoint
email service layer updated to pass html data to chefs depending on the content

Types of changes

New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@codeclimate CodeClimate
Copy link

codeclimate bot commented Sep 5, 2024 •
edited
Loading

Code Climate has analyzed commit 2d3917f and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 26.3% (50% is the threshold).

This pull request will bring the total coverage in the repository to 36.6% (0.0% change).

View more on Code Climate.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 5, 2024 •
edited
Loading

Coverage Report (Frontend)

Totals Coverage
Statements: 27.2% ( 1558 / 5727 )
Methods: 23.29% ( 256 / 1099 )
Lines: 31.56% ( 969 / 3070 )
Branches: 21.37% ( 333 / 1558 )

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 5, 2024 •
edited
Loading

Coverage Report (Application)

Totals Coverage
Statements: 37.62% ( 944 / 2509 )
Methods: 26.92% ( 119 / 442 )
Lines: 48.94% ( 626 / 1279 )
Branches: 25.25% ( 199 / 788 )

@sanjaytkbabu sanjaytkbabu force-pushed the feature/confirm-email-updates branch 7 times, most recently from 6690acc to 0694111 Compare September 6, 2024 16:05
@Subin1Doo
Copy link

It's looking great! I think we should decrease the size of the logo and add the same margin to the right of the confirmation email message. Thank you!

@sanjaytkbabu sanjaytkbabu force-pushed the feature/confirm-email-updates branch 3 times, most recently from 99bec22 to 6c2aa34 Compare September 6, 2024 23:20
@wilwong89
Copy link
Contributor

There's a issue that stems from a prior change that we should fix in this PR.
The Submission Assistance flow, because it uses onSaveDraft to manage the logic, does not send a confirmation email even though it in essence submits an enquiry intake and saves a submission draft.

Could you please add that logic in?

kyle1morel
kyle1morel reviewed Sep 9, 2024
View reviewed changes
frontend/src/components/housing/enquiry/EnquiryIntakeForm.vue Outdated
@@ -256,7 +266,7 @@ async function emailConfirmation(activityId: string) {
bodyType: 'text',
Copy link
Collaborator

Choose a reason for hiding this comment

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

The bodyType is defined here, and can be set to html as we know the email contents we wish to send. The changes to the email api don't feel necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that works!

@sanjaytkbabu sanjaytkbabu force-pushed the feature/confirm-email-updates branch from 6c2aa34 to f2c8062 Compare September 10, 2024 20:54
codeclimate[bot]
codeclimate bot reviewed Sep 10, 2024
View reviewed changes
frontend/src/utils/templates.ts
return replacePlaceholders(baseTemplate, replaceConfig);
};

export const confirmationTemplateEnquiry = (replaceConfig: { [key: string]: string | string[] | undefined }) => {
Copy link

Choose a reason for hiding this comment

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

Function confirmationTemplateEnquiry has 27 lines of code (exceeds 25 allowed). Consider refactoring.

@sanjaytkbabu sanjaytkbabu force-pushed the feature/confirm-email-updates branch from f2c8062 to 7bb28a6 Compare September 10, 2024 21:11
@sanjaytkbabu
Copy link
Contributor Author

sanjaytkbabu commented Sep 10, 2024 •
edited
Loading

There's a issue that stems from a prior change that we should fix in this PR. The Submission Assistance flow, because it uses onSaveDraft to manage the logic, does not send a confirmation email even though it in essence submits an enquiry intake and saves a submission draft.

Could you please add that logic in?

done!

@sanjaytkbabu sanjaytkbabu force-pushed the feature/confirm-email-updates branch 3 times, most recently from f66dcd6 to cf9b837 Compare September 10, 2024 21:29
@Subin1Doo
Copy link

other than the assistance confirmation email mentioned above, everything is looking good!

kyle1morel
kyle1morel reviewed Sep 10, 2024
View reviewed changes
frontend/src/utils/templates.ts Outdated
Comment on lines 63 to 65
const PCNS_URL = 'https://pcns-prod-master.apps.silver.devops.gov.bc.ca';
const BC_BANNER = 'https://coms.api.gov.bc.ca/api/v1/object/446ee8ee-e302-4cb8-b44d-24a1f583edba';
const BC_FOOTER = 'https://coms.api.gov.bc.ca/api/v1/object/853de44a-e62f-41f5-81fd-6eff6cb66d52';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be a single set of constants in this file instead of duplicated in each function

frontend/src/utils/templates.ts Outdated
Comment on lines 93 to 95
const PCNS_URL = 'https://pcns-prod-master.apps.silver.devops.gov.bc.ca';
const BC_BANNER = 'https://coms.api.gov.bc.ca/api/v1/object/446ee8ee-e302-4cb8-b44d-24a1f583edba';
const BC_FOOTER = 'https://coms.api.gov.bc.ca/api/v1/object/853de44a-e62f-41f5-81fd-6eff6cb66d52';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be a single set of constants in this file instead of duplicated in each function

frontend/src/utils/templates.ts Outdated
@@ -59,13 +59,64 @@ const replacePlaceholders = (baseText: string, replacementConfig: { [key: string
return newText;
};

export const confirmationTemplate = (replaceConfig: { [key: string]: string | string[] | undefined }) => {
export const confirmationTemplateSubmission = (replaceConfig: { [key: string]: string | string[] | undefined }) => {
const PCNS_URL = 'https://pcns-prod-master.apps.silver.devops.gov.bc.ca';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you ref the url from the route instead of hard coding in the production url?

@sanjaytkbabu
Proponent Confirm Email Updates
2d3917f 
Confirmation message text change
@sanjaytkbabu sanjaytkbabu force-pushed the feature/confirm-email-updates branch from cf9b837 to 2d3917f Compare September 11, 2024 18:02
@kyle1morel kyle1morel merged commit 2db9f9a into master Sep 11, 2024
18 of 19 checks passed
@kyle1morel kyle1morel deleted the feature/confirm-email-updates branch September 11, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate[bot] left review comments

@kyle1morel kyle1morel kyle1morel approved these changes

@wilwong89 wilwong89 Awaiting requested review from wilwong89 wilwong89 is a code owner

@qhanson55 qhanson55 Awaiting requested review from qhanson55 qhanson55 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sanjaytkbabu @Subin1Doo @wilwong89 @kyle1morel