Bugfix/bai 862 request reviews on release creation #733

JR40159 · 2023-09-14T11:13:17Z

No description provided.

a3957273 · 2023-09-14T11:39:01Z

backend/package.json

@@ -106,7 +106,7 @@
    "eslint-plugin-simple-import-sort": "^10.0.0",
    "mongodb-memory-server": "^8.13.0",
    "nodemon": "^3.0.1",
-    "prettier": "3.0.3",
+    "prettier": "2.8.8",


We should just update everything to 3.0.3

I'm just avoiding the extra styling while I'm still deving, I'll add it back once I'm done :)

a3957273 · 2023-09-14T11:41:30Z

backend/src/connectors/v2/authorisation/silly.ts

+  }
+
+  async getGroupMembers(entity: string): Promise<string[]> {
+    if (fromEntity(entity).kind !== EntityKind.User) {


This is wrong, surely the entity HAS to not be a user to get the members of it. I think you want the inverse of this conditional.

Copy and paste error, good spot

a3957273 · 2023-09-14T11:53:23Z

backend/src/models/v2/ReviewRequest.ts

+  modelId?: string
+  kind: ReviewKindKeys
+
+  entity: CollaboratorEntry


This should not be a collaborator entry. It should just be a string I think. There should be another entry on this for the type of review it is (MSRO, MTR, etc). A collaborator entry could have multiple roles on it, including roles that don't have approvals.

a3957273 · 2023-09-14T12:01:35Z

backend/src/models/v2/ReviewRequest.ts

+
+const ReviewRequestSchema = new Schema<ReviewRequestInterface>(
+  {
+    model: { type: String, required: true },


You have both model and model id on this model. Pick one! Probably modelId.

a3957273 · 2023-09-14T12:07:55Z

backend/src/models/v2/ReviewRequest.ts

+    },
+    kind: { type: String, enum: Object.values(ReviewKind), required: true },
+
+    entity: {


Talked about this over call. Entities should likely be an array, roles should be singular.

a3957273 · 2023-09-14T13:16:21Z

backend/src/services/v2/smtp/smtp.ts

+  const text = email.getBody(release.name, reviewRequest.kind, release.modelId, appBaseUrl, 'unknown')
+  const html = email.getHtml(release.name, reviewRequest.kind, release.modelId, appBaseUrl, 'unknown')
+
+  if (!config.smtp.enabled) {


We should do this check at the start of the function, before we've tried to get every users email, the list of users to send to, created the email, etc.

If you look in the if statement, you'll see, as with the original implementation, we log out the subject and the address. But I do think the subject and the address is pretty irrelevant if smtp has been disabled, so I'll update this

a3957273 · 2023-09-14T13:16:40Z

backend/src/services/v2/smtp/smtp.ts

+    return
+  }
+
+  if (!transporter) {


Creating a transport is not async. There's no reason we need to do this inside the function.

Even if it were async, we have top level await now.

This is so we only need to create a transporter once as a pose to every time we call this function?

a3957273 · 2023-09-14T15:51:41Z

backend/src/services/v2/smtp/templates/releaseReviewRequest.ts

+  `)
+  }
+
+  getBody(releaseName: string, reviewKind: ReviewKindKeys, modelId: string, baseUrl: string, author: string) {


This should probably be called getText, as it's the text mime. Both this and the HTML are technically the body.

a3957273 · 2023-09-14T15:54:24Z

backend/src/utils/v2/config.ts

  log: {
    level: bunyan.LogLevel
  }
 }

 const config: Config = _config.util.toObject()
+deepFreeze(config)
 export default config


Nitpick: export default deepFreeze(config) might be clearer.

a3957273 · 2023-09-14T15:55:19Z

backend/src/utils/v2/entity.ts

-export function toEntity(kind: string, value: string) {
+export const EntityKind = {
+  User: 'user',
+  Group: 'group',


I don't think this can be an enum, necessarily. Systems can generate their own entity kinds, e.g. we'll have ones specific to our internal names.

…irements-on-release-creation

a3957273 · 2023-09-17T17:50:16Z

backend/src/models/v2/Schema.ts

@@ -14,7 +14,7 @@ export interface SchemaInterface {
  hidden: boolean

  kind: SchemaKindKeys
-  jsonSchema: unknown
+  jsonSchema: object


object is really not a good type in TypeScript. Here, we'll want a partially typed interface, something like:

interface UploadSchema { properties: { highLevelDetails: { title: string description: string type: string properties: { name: { title: string, description: string, [x: string]: unknown }, [x: string]: unknown } [x: string]: unknown } [x: string]: unknown } [x: string]: unknown }

It's not particularly nice, but we've run into some issues with schemas being any in v1.

Because the above interface is bad, you can probably use some types from 'json-schema' to help out (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/json-schema/index.d.ts)

I think this is out of scope for this PR. I'll revert back to unknown

a3957273 · 2023-09-17T17:50:28Z

backend/src/routes/v2/reviewRequest/getReviewRequests.ts

@@ -13,16 +13,18 @@ export const getReviewRequestsSchema = z.object({
 })

 interface GetReviewRequestsResponse {
-  approvals: Array<ReviewRequestInterface>
+  reviewRequests: Array<ReviewRequestInterface>


Nice catch!

a3957273 · 2023-09-17T17:52:37Z

backend/src/utils/v2/mongo.ts

@@ -11,3 +12,10 @@ export function isMongoServerError(err: unknown): err is MongoServerError {

  return false
 }
+
+export function handleDuplicateKeys(error: unknown) {


This is probably more reusable as:

if (isMongoServerError(error) && error.code == 11000) { throw BadReq(`The following is not unique: ${JSON.stringify(error.keyValue)}`) }

Without handling the case of it not being a duplicate key, and immediately throwing the error. It's rarely useful to return errors.

a3957273 · 2023-09-17T17:54:01Z

backend/src/services/v2/release.ts

  await release.save()
+  } catch(error) {
+    handleDuplicateKeys(error)


You're not throwing anything here. I've suggested some changes to the utility, but this should still throw other errors, e.g:

catch(error) { handleDuplicateKeys(error) throw error }

a3957273 · 2023-09-17T17:54:44Z

backend/src/services/v2/release.ts

+  }
+
+  try {
+  await createReleaseReviewRequests(model, release)


Is this valid indentation? I can't believe it doesn't want await create... to be indented twice more.

a3957273 · 2023-09-17T18:04:18Z

backend/src/models/v2/ReviewRequest.ts

  semver?: string
  modelId?: string
  kind: ReviewKindKeys

-  entity: CollaboratorEntry
+  role: string
+  entities: Array<string>


As mentioned in chat, we don't want entities to be on the review request.

a3957273 · 2023-09-17T18:06:19Z

backend/src/services/v2/review.ts

-  }
+function getEntitiesForRole(collaborators: Array<CollaboratorEntry>, role: string): string[] {
+  const roleEntities: string[] = collaborators
+    .filter((collaborator) => {


.filter((collaborator) => collaborator.roles.includes(role) is better.

a3957273 · 2023-09-17T18:06:55Z

backend/src/services/v2/schema.ts

-      throw BadReq(`The following is not unique: ${JSON.stringify(error.keyValue)}`)
-    }
-    throw error
+    throw handleDuplicateKeys(error)


With other suggested change to handleDuplicateKeys this becomes:

handleDuplicateKeys(error) throw error

a3957273 · 2023-09-17T18:09:20Z

backend/src/services/v2/smtp/templates/baseEmailTemplate.ts

-  getSubject(resourceName: string): string
-  getHtml(releaseName: string, reviewKind: ReviewKindKeys, modelId: string, baseUrl: string, author: string): string
-  getBody(releaseName: string, reviewKind: ReviewKindKeys, modelId: string, baseUrl: string, author: string): string
+  to


These should be typed, e.g. as string.

I don't think you need this interface for anything. You can use the class to automatically generate these types. Just add these as abstract functions to your class, and add the types in. Then you can just use the class and not this interface. See our v2 authorization class as an example.

a3957273 · 2023-09-17T18:20:42Z

backend/src/services/v2/smtp/templates/releaseReviewRequest.ts

-  getSubject(resourceName: string): string {
-    return dedent(`
-    You have been requested to review '${resourceName}' on Bailo
+export class ReleaseReviewRequestEmail extends BaseEmailTemplate implements IEmailTemplate {


This seems over complicated. I think I now understand why you're using both extends and implements at the same time.

interface Email { subject: string text: string html: string to: string } function emailWrapper(body: string) { ... } function sendEmail(email: Email) { ... } function sendReviewRequestNotifications(to: string, model: ModelDoc, ...) { const subject = `${reviewerRole.toUpperCase()}: You have been requested to review '${resourceName}' on Bailo` const text = dedent(` // some text `) const html = emailWrapper(dedent(` // some html `)) await sendEmail({ to, subject, html, text }) }

Seems much more understandable, without losing any of our type safety.

I may be misunderstanding what you're suggesting here but it seems like you're wanting to include all of the static email content like the HTML inside sendReviewRequestNotifications which is in the smtp service? I think this is a bad idea and the static content specific to our different emails should not be included in the generic smtp service

…irements-on-release-creation

JR40159 added 20 commits September 5, 2023 11:32

Add smtp service and a happy test

4e0efbd

Make config immutable

2138d58

Add test for disabled smtp

7b04aa2

Change to mockReturnValueOnce

4a29659

remove console logs

9c5825b

Add Review Request creation on release creation

a2c1c62

Replace approvals with review requests

7f89020

Send email to groups

1c51d10

Remove role from review respoonse object

f93b16e

Fix bugs

acaabf2

Add email templates

3664c9e

revert prettier and fix existing style issues

1450011

Fix problems in the email

b6c1d42

Rename approvals to review requests

8a9ec03

Update mock

c8ed25a

Fix broken test

a7f9a0a

Test email service

7f1863e

Create a review request for each role

f146c60

Fix filter

606b273

Update get Review Requests by active

ac537e2

a3957273 requested changes Sep 14, 2023

View reviewed changes

JR40159 added 6 commits September 14, 2023 16:37

Review comments

35a5df7

Rename methods for 'release' reviews

5e88078

Merge branch 'main' into bugfix/BAI-862-create-approval-requests-requ…

d0d9af8

…irements-on-release-creation

Move entity logic out of smtp service

36fb5cc

Make entityKind internal to the auth mechanism

d800aa9

Add a list of reviewer roles for a release

992f9ea

a3957273 requested changes Sep 17, 2023

View reviewed changes

JR40159 added 2 commits September 18, 2023 08:36

PR feedback

a1ca487

Remove entities from reviews

f5edf09

JR40159 added 2 commits September 18, 2023 09:29

Rename ReviewRequests to Reviews

045915e

Remove throwing of error, log it instead

4deb39b

a3957273 approved these changes Sep 18, 2023

View reviewed changes

JR40159 added 6 commits September 18, 2023 13:56

Update tests

f3a0c5c

Update prettier and style

3bf0f6e

Merge branch 'main' into bugfix/BAI-862-create-approval-requests-requ…

a2db73c

…irements-on-release-creation

Fix lint issues

c891364

Update package lock

03f36af

Remove commented out line

3196362

JR40159 merged commit b794839 into main Sep 18, 2023

JR40159 deleted the bugfix/BAI-862-create-approval-requests-requirements-on-release-creation branch September 18, 2023 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/bai 862 request reviews on release creation #733

Bugfix/bai 862 request reviews on release creation #733

JR40159 commented Sep 14, 2023

a3957273 Sep 14, 2023

JR40159 Sep 14, 2023

a3957273 Sep 14, 2023

JR40159 Sep 14, 2023

a3957273 Sep 14, 2023

a3957273 Sep 14, 2023

a3957273 Sep 14, 2023

a3957273 Sep 14, 2023

JR40159 Sep 14, 2023

a3957273 Sep 14, 2023

a3957273 Sep 14, 2023

JR40159 Sep 14, 2023

a3957273 Sep 14, 2023

a3957273 Sep 14, 2023

a3957273 Sep 14, 2023

a3957273 Sep 17, 2023

a3957273 Sep 17, 2023

JR40159 Sep 18, 2023

a3957273 Sep 17, 2023

a3957273 Sep 17, 2023

a3957273 Sep 17, 2023

a3957273 Sep 17, 2023

a3957273 Sep 17, 2023

a3957273 Sep 17, 2023

a3957273 Sep 17, 2023

a3957273 Sep 17, 2023

a3957273 Sep 17, 2023

a3957273 Sep 17, 2023

JR40159 Sep 18, 2023

Bugfix/bai 862 request reviews on release creation #733

Bugfix/bai 862 request reviews on release creation #733

Conversation

JR40159 commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment