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

Feature/refactor review responses #1381

Merged
merged 11 commits into from
Jul 8, 2024
Merged

Conversation

JR40159
Copy link
Member

@JR40159 JR40159 commented Jul 3, 2024

Includes:

  1. Fixing the entity in review response notification emails.
  2. Fixing the auditing request object for view responses.
  3. Fixing access requests.

break
}
default:
throw GenericError(500, 'Review Kind not recognised', reviewIdQuery)
Copy link
Member

Choose a reason for hiding this comment

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

Is this suppose to be a Generic Error or an Internal Error?

@@ -171,6 +160,7 @@ describe('services > review', () => {
expect(smtpMock.requestReviewForAccessRequest).toBeCalled()
})

/*
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason these tests are commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot, thank you! I commented them out and then moved them to the new response.spec.ts for some reason. I'll delete the comment block now they've been moved.

Comment on lines +77 to +78

// Store the response
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth checking if a review has already been approved? And perhaps throwing an error?

@JR40159 JR40159 merged commit fde2239 into main Jul 8, 2024
14 checks passed
@JR40159 JR40159 deleted the feature/refactor-review-responses branch July 8, 2024 15:18
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