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

Validate confirmation Id (activityId) for Enquiries #121

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

qhanson55
Copy link
Collaborator

Description

Made it so that a warning toast pops out when the confirmation id (activityId) entered by the proponent doesn't return a submissions search result.
Proponents will now be visually told that they are entering an invalid id but not stopped from doing so.
PADS-181

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

Copy link

github-actions bot commented Jul 26, 2024

Coverage Report (Application)

Totals Coverage
Statements: 46.06% ( 941 / 2043 )
Methods: 36.02% ( 134 / 372 )
Lines: 56.98% ( 612 / 1074 )
Branches: 32.66% ( 195 / 597 )

@qhanson55 qhanson55 force-pushed the feature/validate-enquiries branch 2 times, most recently from 1a0d931 to 4f990a4 Compare July 26, 2024 00:51
@@ -17,10 +17,6 @@ const { getConfig } = storeToRefs(useConfigStore());
const permissionService = new PermissionService();
const router = useRouter();

async function ssoRequestBasicAccess() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deleted as was triggering unused warning

Copy link

github-actions bot commented Jul 26, 2024

Coverage Report (Frontend)

Totals Coverage
Statements: 32.35% ( 1422 / 4395 )
Methods: 27.84% ( 245 / 880 )
Lines: 37.06% ( 879 / 2372 )
Branches: 26.07% ( 298 / 1143 )

Copy link

codeclimate bot commented Jul 26, 2024

Code Climate has analyzed commit 25882d0 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 1

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

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

View more on Code Climate.

(order: number) => {
sortOrder = order;
(order: number | undefined) => {
order !== undefined ? (sortOrder = order) : null;
Copy link
Collaborator Author

@qhanson55 qhanson55 Jul 26, 2024

Choose a reason for hiding this comment

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

triggered this error, this fixes it:

  Types of parameters 'order' and 'value' are incompatible.
    Type 'number | undefined' is not assignable to type 'number'.
      Type 'undefined' is not assignable to type 'number'.

206           @update:sort-order="

Comment on lines 241 to 243

if (activityId) {
const submission = (await submissionService.searchSubmissions({ activityId: [activityId] })).data[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

We could check the backend for activityId when there are exactly 8 characters, as that's the length of it. Will save on network calls.

On another note, I think we need to think a little ahead and make checking for submissions its own endpoint instead of using searchSubmissions. Right now, any user would be able to search our entire DB and retrieve any submission. There should be a new endpoint that only returns existence or non-existence for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

checking 8 characters now as well as if it is a hexadecimal value.

Implemented a new endpoint for checking the validility.

@@ -234,6 +234,19 @@ async function emailConfirmation(activityId: string) {
};
await submissionService.emailConfirmation(emailData);
}

async function checkValidility(event: Event) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Function name is unclear. What are we checking the validity of?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added ActivityId for clarity

@qhanson55 qhanson55 force-pushed the feature/validate-enquiries branch from 4f990a4 to fbd48ae Compare July 31, 2024 21:14
@@ -69,4 +69,9 @@ router.patch(
}
);

//** Validates an Activity Id */
router.get('/validate/:activityId', (req: Request, res: Response, next: NextFunction): void => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

}
const activity = await activityService.getActivity(activityId);

res.status(200).json({ valid: Boolean(activity && !activity.isDeleted) });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the Boolean wrapper necessary here? activity && !activity.isDeleted should evaluate to what you're looking for.

Copy link
Member

Choose a reason for hiding this comment

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

If boolean wrapping is required if/when one of the variables ends up being undefined/falsy, it would be better do do it with a double !! as we typically want to avoid using JS boxing functions when we are forced to cast to a different type.

res.status(200).json({ valid: !!activity && !activity.isDeleted });

Comment on lines 72 to 76
//** Validates an Activity Id */
router.get('/validate/:activityId', (req: Request, res: Response, next: NextFunction): void => {
enquiryController.validateActivityId(req, res, next);
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider moving this to a new activity route. As we are not touching any enquiry logic this feels out of place here.

}
const activity = await activityService.getActivity(activityId);

res.status(200).json({ valid: Boolean(activity && !activity.isDeleted) });
Copy link
Member

Choose a reason for hiding this comment

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

If boolean wrapping is required if/when one of the variables ends up being undefined/falsy, it would be better do do it with a double !! as we typically want to avoid using JS boxing functions when we are forced to cast to a different type.

res.status(200).json({ valid: !!activity && !activity.isDeleted });

…s and a new endpoint in backend to perform said check
@qhanson55 qhanson55 force-pushed the feature/validate-enquiries branch from fbd48ae to 25882d0 Compare August 1, 2024 19:50
@wilwong89 wilwong89 merged commit 41caf2f into master Aug 2, 2024
19 checks passed
@wilwong89 wilwong89 deleted the feature/validate-enquiries branch August 2, 2024 17:57
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.

4 participants