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

Proponent Contact Profile #235

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

sanjaytkbabu
Copy link
Contributor

@sanjaytkbabu sanjaytkbabu commented Jan 7, 2025

Description

PADS-167

Contact management

Frontend changes:
New contactProfileView added for viewing & updating contact info
New contactSearchParams & contactService added for searching and updating contacts
Updates - HeaderMenu, en-CA, application enums, router, authzStore

Backend changes:
New - controller, route for searching and updating contacts
New contactSearch type
New migration added for creating contact resource,
Updates - enquiry, submission controllers upsert calls, user service to create contact on login, renamed contacts validator

Types of changes

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

Copy link

github-actions bot commented Jan 7, 2025

Coverage Report (Frontend)

Totals Coverage
Statements: 46.78% ( 3268 / 6986 )
Methods: 35.69% ( 419 / 1174 )
Lines: 54.81% ( 1916 / 3496 )
Branches: 40.28% ( 933 / 2316 )

Copy link

github-actions bot commented Jan 7, 2025

Coverage Report (Application)

Totals Coverage
Statements: 36.8% ( 1104 / 3000 )
Methods: 26.82% ( 136 / 507 )
Lines: 48.7% ( 728 / 1495 )
Branches: 24.05% ( 240 / 998 )

app/src/controllers/contact.ts Dismissed Show dismissed Hide dismissed
@sanjaytkbabu sanjaytkbabu force-pushed the feature/contact-profile branch 2 times, most recently from 33733a1 to c25b259 Compare January 8, 2025 00:55
@sanjaytkbabu sanjaytkbabu reopened this Jan 8, 2025
@sanjaytkbabu sanjaytkbabu force-pushed the feature/contact-profile branch from c25b259 to fd1adca Compare January 8, 2025 01:09
@slhurley
Copy link
Collaborator

slhurley commented Jan 8, 2025

Look is great to me but a few issues:

  • Phone number should look like it does on the intake form with brackets around the area code etc.
  • Phone number validation error message is not intuitive
  • Phone number validation error sticks and allows you to leave with it still on the screen. It appears to let you save and leave to "Get Started" after you blank out the phone number even though it doesn't actually save a blank. Review the validation on this.
  • Check PADS-403 changes...you will need to conform with that change.
  • Design mentioned the Get Started button should not be available until all fields are filled in. Check with UX.

@Subin1Doo
Copy link

Looking great! Here are some things to look at

  • Getting a 422 error when trying to save the contact info.. maybe this is just me?
  • Increase drop shadow to make the contact profile box look more evident
  • "Get started" button should only show up for first-time login proponents and only when they've filled out their contact info and hit save for the first time
  • Edit button should be hidden when users are editing
  • Edit button styling: Button outline only shows when user hover/ clicks on the button, check Figma
  • Save button not working for me, so not sure if the success message banner is there.. "Success: Contact saved" banner should appear
  • Save and cancel button too close to the last field of the contact info, increase spacing.

Thank you!!

@sanjaytkbabu sanjaytkbabu force-pushed the feature/contact-profile branch from 564c66e to a1317e1 Compare January 8, 2025 22:33
@sanjaytkbabu
Copy link
Contributor Author

Look is great to me but a few issues:

  • Phone number should look like it does on the intake form with brackets around the area code etc.
  • Phone number validation error message is not intuitive
  • Phone number validation error sticks and allows you to leave with it still on the screen. It appears to let you save and leave to "Get Started" after you blank out the phone number even though it doesn't actually save a blank. Review the validation on this.
  • Check PADS-403 changes...you will need to conform with that change.
  • Design mentioned the Get Started button should not be available until all fields are filled in. Check with UX.

done!
re: PADS-403 - I'm reusing the code for the dropdown, it will show the new dropdown when that code gets merged.

@sanjaytkbabu
Copy link
Contributor Author

Look is great to me but a few issues:

  • Phone number should look like it does on the intake form with brackets around the area code etc.
  • Phone number validation error message is not intuitive
  • Phone number validation error sticks and allows you to leave with it still on the screen. It appears to let you save and leave to "Get Started" after you blank out the phone number even though it doesn't actually save a blank. Review the validation on this.
  • Check PADS-403 changes...you will need to conform with that change.
  • Design mentioned the Get Started button should not be available until all fields are filled in. Check with UX.

done! re: PADS-403 - I'm reusing the code for the dropdown, it will show the new dropdown when that code gets merged.

done!

@sanjaytkbabu sanjaytkbabu marked this pull request as ready for review January 8, 2025 22:40
@sanjaytkbabu sanjaytkbabu reopened this Jan 8, 2025
@sanjaytkbabu sanjaytkbabu force-pushed the feature/contact-profile branch from a1317e1 to 7853dca Compare January 8, 2025 23:49
@sanjaytkbabu sanjaytkbabu reopened this Jan 8, 2025
@sanjaytkbabu sanjaytkbabu force-pushed the feature/contact-profile branch 4 times, most recently from 42e02be to 8adf7e4 Compare January 9, 2025 22:02
@sanjaytkbabu sanjaytkbabu reopened this Jan 9, 2025
@sanjaytkbabu sanjaytkbabu force-pushed the feature/contact-profile branch 2 times, most recently from 79bb10b to 0cdbfe4 Compare January 10, 2025 00:00
Copy link

github-actions bot commented Jan 10, 2025

@sanjaytkbabu sanjaytkbabu reopened this Jan 10, 2025
const contactIds = mixedQueryToArray(req.query.contactId);
let userIds = mixedQueryToArray(req.query.userId);

if (!userIds && req.currentContext.userId) userIds = [req.currentContext.userId];
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reasoning for defaulting to searching the current user if no other IDs are provided?

Comment on lines 32 to 42
},
updateContact: async (req: Request<never, never, Contact, never>, res: Response, next: NextFunction) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style: New line between functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,229 @@
/* eslint-disable max-len */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Migration will have to be renamed before merging. An earlier PR, #231, has the 018 migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// Add all navigator read only role mappings
await addResourceRoles(navigator_read_group_id[0].group_id, Resource.CONTACT, [Action.READ, Action.UPDATE]);


Copy link
Collaborator

Choose a reason for hiding this comment

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

Style: Unnecessary new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// Add all admin role mappings
await addResourceRoles(admin_group_id[0].group_id, Resource.CONTACT, [Action.READ, Action.UPDATE]);


Copy link
Collaborator

Choose a reason for hiding this comment

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

Style: Unnecessary new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -129,5 +130,19 @@
"projectLocationDescriptionCard": "Is there anything else you would like to tell us about this project's location? (optional)",
"provincialPermitsCard": "Have you applied for any provincial permits for this project?",
"investigatePermitsCard": "Select all provincially issued permits you think you might need (optional)"
},
"contactProfileView": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please alphabetize the top level keys as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* @function updateEnquiry
* @returns {Promise} An axios response
*/
updateContact(contactId: string, data?: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

contactId is not necessary here. You can pull the path parameter from data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh right, missed that. thanks!

const submitData: Contact = setEmptyStringsToNull(values);
const result = await contactService.updateContact(values.contactId, submitData);
if (result.status === 200) {
toast.success('Form saved');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add all new text to the locale file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 143 to 150
h3 {
margin-top: 1em;
}
:deep(.p-card-body) {
width: 35rem;
box-shadow: 0 0 0.2rem #036;
padding-bottom: 0;
}
.display-inline {
display: inline;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style: A newline should be present between classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thank you!

Comment on lines 89 to 124
<FormNavigationGuard />
<InputText
name="firstName"
:label="t('contactProfileView.firstName')"
:disabled="true"
/>

<InputText
name="lastName"
:label="t('contactProfileView.lastName')"
:disabled="true"
/>
<InputText
name="email"
:label="t('contactProfileView.email')"
:disabled="true"
/>
<InputMask
name="phoneNumber"
mask="(999) 999-9999"
:label="t('contactProfileView.phone')"
/>

<Dropdown
name="contactApplicantRelationship"
:label="t('contactProfileView.relationshipToProject')"
:bold="true"
:options="PROJECT_RELATIONSHIP_LIST"
/>
<Dropdown
name="contactPreference"
:label="t('contactProfileView.preferredContact')"
:bold="true"
:options="CONTACT_PREFERENCE_LIST"
/>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Style: Inconsistent newlines between components. In this instance, we probably want to remove them. Precedence in the application currently is to add a newline between major blocks, such as <Card />.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@sanjaytkbabu sanjaytkbabu force-pushed the feature/contact-profile branch 2 times, most recently from 2234eef to 704948c Compare January 10, 2025 23:24
const identityIds = mixedQueryToArray(req.query.identityId);

console.log('\n\n\nidentityIds>>>>>' + identityIds);
console.log('\n\n\ncontactIds>>>>>' + contactIds);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These console statements are needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed! thanks

@sanjaytkbabu sanjaytkbabu force-pushed the feature/contact-profile branch from 704948c to a1a1ec6 Compare January 15, 2025 00:57
…tSearch type, migration for contact resource, updated - enquiry, submission controllers upsert calls, user service to create contact on login, renamed contacts validator
…actSearchParams type, contactService, tests, updated - HeaderMenu, en-CA, application enums, router, authzStore
@sanjaytkbabu sanjaytkbabu force-pushed the feature/contact-profile branch from a1a1ec6 to 7eb5a35 Compare January 15, 2025 01:02
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.

5 participants