#953 - Part 2 Generate review assignments action not auto assign final decision #955

nmadruga · 2022-11-07T03:46:09Z

Part 2 fix for #953

Front-end: msupply-foundation/conforma-web-app#1428

Briefing of changes

Removed action updateReviewAssignmentStatus not needed anymore - since we don't change isLocked for selfAssignments
Removed from ENUM assign_actions the column ASSIGN_LOCKED
Add new column auto-generated on Graphql calls to set available_sections in ReviewAssignment

Previous description

Changes to action generateReviewAssignments required for FinalDecision to act as any other self-assignable assignment. So only one reviewAssignment should stay as ASSIGNED status. The reviewer has to self-assign first and then start the review. This fix should be resolving the existing bug with #943

~~I also had to change how isLocked persists. Will need further testing to make sure this is consistent with other states.~~ Not needed now that action updateReviewAssignmentStatus has been fully removed.

… be assigned prior to work started

…nts' into #953-generateReviewAssignments-action-not-auto-assign-final-decision

… as unlocked on unassignment - so the newly asignee is not locked anymore

omsupply-bot · 2022-11-07T03:46:12Z

Beep boop! This pull request does not have an associated issue :(.

CarlosNZ

Cool, great to see a PR that is mainly removals :)

See my inline comment about the "is_locked" situation.

plugins/action_update_review_assignments_status/src/updateReviewAssignmentsStatus.ts

…ment ReviewAssignments. The isLocked only is set on updateReviewStatus now

CarlosNZ

See comments. Looking good though :)

CarlosNZ · 2022-11-07T22:50:40Z

plugins/action_generate_review_assignment_records/src/generateReviewAssignments.ts

  // 1. If existing
  //   - keep same status, isSelfAssignable
-  //   - just update isLocked = true (if already assigned to another)
+  //   - keep same isLocked if assigned


// - keep same isLocked if assigned

Shouldn't existing review_assignments get isLocked = false always? (assuming this action runs on submission of applications and reviews). Otherwise, when do the "locked" assignments get "unlocked"?

Fixed. But basically I added one specific case for when one ReviewAssignment was added while in the state of last submitted Review has locked others (LOQ submitted). So for that reason I have also changed updateReviewStatus which will set ReviewAssignment to be locked in this case (and then checked here). Please have a look on these 2 files:

plugins/action_generate_review_assignment_records/src/generateReviewAssignments.ts

plugins/action_update_review_status/src/updateReviewsStatuses.ts

CarlosNZ · 2022-11-07T22:54:14Z

documentation/List-of-Action-plugins.md

@@ -461,7 +460,7 @@ See [Grant Permissions](#grant-permissions) above regarding acting on user-only

 ### Generate Review Assignments

-Generates records in the `review_assignment` table -- i.e. which users (reviewers) are allowed to do a review for the current stage/level (and for which Sections). The records are set with `status` "Available" or "Assigned" and flags for `isSelfAssignable` and `isLocked` to help define when is allowed self-assignment.
+Generates records in the `review_assignment` table -- i.e. which users (reviewers) are allowed to do a review for the current stage/level (and for which Sections). The records are set with `status` "Available" or "Assigned". With the properties to specify the type of assignament: `isSelfAssignable` if it should show for self-assignment when not assigned by another user. The `isLocked` property helps defining that the review can start but not be submitted - after the application has been back to Applicant for ammendments.


Maybe bullet-point these properties to make easier to read?

Also:

isSelfAssignable if it should show for self-assignment when not assigned by another user.

Isn't "self-assignable" a template configuration? So maybe should say "determines whether this assignment is self-assignable as determined by the template configuration for this stage/level" ?

Typo on assignament => "assignment" ;)

Thanks. Done

…ns_list_view and add new function to auto-generate column resulting the available_sections for one assignment

…s only

database/migration/migrateData.ts

CarlosNZ · 2022-11-08T04:06:08Z

database/migration/migrateData.ts

+    console.log(' - Update existing review_assignment to set is_locked=false when isSelfAssigned')
+    // TODO - Not sure how to do this


What happens if we don't do this?

Removed. 👍

database/buildSchema/43_views_functions_triggers.sql

…ssignments-action-not-auto-assign-final-decision

Fix mistake in review_assignment validity trigger

CarlosNZ · 2022-11-09T04:04:45Z

Okay, @nmadruga I've updated and fixed a bunch of those things so we could get this branch working. Can see my changes in 52ce0d8

I've marked all the comments "Resolved" that I've taken care off -- you can show them again if you want to see what they were. So the only comments still visible should be minor things or questions for you to take a look at. The only one that seems fairly important is this one.

…ignment

… LOQ so any other ReviewAssignments created also are locked at that point.

…ed from Applicant to be used as model for new ReviewAssignments created after that (so those wouldn't generate an unlocked assignment in the same level/stage)

nmadruga · 2022-11-10T01:23:54Z

Okay, @nmadruga I've updated and fixed a bunch of those things so we could get this branch working. Can see my changes in 52ce0d8

I've marked all the comments "Resolved" that I've taken care off -- you can show them again if you want to see what they were. So the only comments still visible should be minor things or questions for you to take a look at. The only one that seems fairly important is this one.

Looks good!

Thanks Carl, I have made suggestions. Please just note the changes flagged on this comment, I'm happy to merge if you are ok with that. Although I haven't got much further with testing this changes yet

CarlosNZ · 2022-11-10T01:31:16Z

plugins/action_update_review_status/src/updateReviewsStatuses.ts

@@ -92,10 +92,23 @@ const updateReviewsStatuses: ActionPluginType = async ({
          ReviewStatus.Draft,
        ])

+        // Lock ReviewAssignment of Review subimmet as changes required to applicant


Suggested change

// Lock ReviewAssignment of Review subimmet as changes required to applicant

// Lock ReviewAssignment of Review submitted as changes required to applicant

CarlosNZ · 2022-11-10T01:34:34Z

plugins/action_update_review_status/src/databaseMethods.ts

+  getReview: async (reviewId: number) => {
+    const text = `
+    SELECT
+      review_assignment_id AS "reviewAssignmentId"
+      FROM review
+      WHERE id = $1
+    `
+
+    try {
+      const result = await DBConnect.query({ text, values: [reviewId] })
+      return result.row[0]
+    } catch (err) {
+      console.log(err.message)
+      throw err
+    }
+  },


Not sure why we need an extra database call. We already fetched all the associated reviews at the beginning, you should be able to pull it out of that result.

Reverting this - as per convo in person

Also - I'll be adding a briefing issue on what we have agreed on

…eReviewStatuses. isLocked logic as it is currently. And new ReviewAssignments gets created always as isLocked = false

CarlosNZ

LGTM, Thanks.

Nicole Madruga added 3 commits November 7, 2022 14:05

Stop creating FinalDecision assignments differently, they should also…

e0059c4

… be assigned prior to work started

Merge remote-tracking branch 'origin/54T-prevent-overlapping-assignme…

545628d

…nts' into #953-generateReviewAssignments-action-not-auto-assign-final-decision

Fix AssignmentStatus update for selfAssignment reviews setting others…

520d7fc

… as unlocked on unassignment - so the newly asignee is not locked anymore

nmadruga mentioned this pull request Nov 7, 2022

Summary of Final Decision improvements #953

Closed

12 tasks

CarlosNZ reviewed Nov 7, 2022

View reviewed changes

plugins/action_update_review_assignments_status/src/updateReviewAssignmentsStatus.ts Outdated Show resolved Hide resolved

Nicole Madruga added 2 commits November 8, 2022 11:06

Delete action not used - after decision to not lock other self-assign…

203d601

…ment ReviewAssignments. The isLocked only is set on updateReviewStatus now

Update docs to remove action and generate new Assignment isLocked = true

bbb0491

CarlosNZ reviewed Nov 7, 2022

View reviewed changes

Nicole Madruga added 2 commits November 8, 2022 16:46

Remove type ASSIGN_LOCKED, remove duplicated declartion of applicatio…

a1db36b

…ns_list_view and add new function to auto-generate column resulting the available_sections for one assignment

use plural name for variable

9d106bc

nmadruga mentioned this pull request Nov 8, 2022

#953B - Part2 generate review assignments action not auto assign final decision msupply-foundation/conforma-web-app#1428

Merged

Nicole Madruga added 4 commits November 8, 2022 16:56

Revert wrong removal of file

f570ea6

Delete table from separated file to keep in 43_view_functions_trigger…

a5aff90

…s only

Delete table from separated file to keep in 43_view_functions_trigger…

22623a1

…s only

Revert change to remove table for view. Just update on migration code

3fa2d71

CarlosNZ reviewed Nov 8, 2022

View reviewed changes

database/buildSchema/43_views_functions_triggers.sql Outdated Show resolved Hide resolved

CarlosNZ reviewed Nov 8, 2022

View reviewed changes

database/buildSchema/43_views_functions_triggers.sql Outdated Show resolved Hide resolved

CarlosNZ added 2 commits November 8, 2022 17:55

Merge branch 'feature/final-decision-fixes' into #953-generateReviewA…

94f06b6

…ssignments-action-not-auto-assign-final-decision

Fix functions and migration script

52ce0d8

Fix mistake in review_assignment validity trigger

CarlosNZ force-pushed the #953-generateReviewAssignments-action-not-auto-assign-final-decision branch from 3827593 to 52ce0d8 Compare November 9, 2022 04:03

Nicole Madruga added 5 commits November 10, 2022 12:42

Set isLocked to be false in generateReviewAssignment for existing ass…

4469444

…ignment

Have to also lock the ReviewAssignment linked to the Review sending a…

ec05384

… LOQ so any other ReviewAssignments created also are locked at that point.

Revert accidental change

33d8b60

Set ReviewAssignment to locked for Review submitted as Changes requir…

6f187f3

…ed from Applicant to be used as model for new ReviewAssignments created after that (so those wouldn't generate an unlocked assignment in the same level/stage)

Improve documentation about action to Generate Review Assignments

20a80b4

CarlosNZ reviewed Nov 10, 2022

View reviewed changes

Reverting changed for isLocked in generateReviewAssignments and updat…

5131912

…eReviewStatuses. isLocked logic as it is currently. And new ReviewAssignments gets created always as isLocked = false

CarlosNZ approved these changes Nov 10, 2022

View reviewed changes

Add log of reviews to be updated

eb76dcb

nmadruga merged commit cc7f7d7 into feature/final-decision-fixes Nov 10, 2022

nmadruga deleted the #953-generateReviewAssignments-action-not-auto-assign-final-decision branch November 10, 2022 02:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#953 - Part 2 Generate review assignments action not auto assign final decision #955

#953 - Part 2 Generate review assignments action not auto assign final decision #955

nmadruga commented Nov 7, 2022 •

edited

Loading

omsupply-bot bot commented Nov 7, 2022

CarlosNZ left a comment

CarlosNZ left a comment

CarlosNZ Nov 7, 2022

nmadruga Nov 10, 2022

CarlosNZ Nov 7, 2022

CarlosNZ Nov 7, 2022

nmadruga Nov 10, 2022

CarlosNZ Nov 8, 2022

nmadruga Nov 10, 2022

CarlosNZ commented Nov 9, 2022

nmadruga commented Nov 10, 2022

CarlosNZ Nov 10, 2022

CarlosNZ Nov 10, 2022

nmadruga Nov 10, 2022

CarlosNZ left a comment

		console.log(' - Update existing review_assignment to set is_locked=false when isSelfAssigned')
		// TODO - Not sure how to do this

	// Lock ReviewAssignment of Review subimmet as changes required to applicant
	// Lock ReviewAssignment of Review submitted as changes required to applicant

#953 - Part 2 Generate review assignments action not auto assign final decision #955

#953 - Part 2 Generate review assignments action not auto assign final decision #955

Conversation

nmadruga commented Nov 7, 2022 • edited Loading

Briefing of changes

Previous description

omsupply-bot bot commented Nov 7, 2022

CarlosNZ left a comment

Choose a reason for hiding this comment

CarlosNZ left a 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

CarlosNZ commented Nov 9, 2022

nmadruga commented Nov 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlosNZ left a comment

Choose a reason for hiding this comment

nmadruga commented Nov 7, 2022 •

edited

Loading