Chore/1876 rebase pending revisions #1882

mikevespi · 2023-11-09T22:21:06Z

Adding the backend work for handling concurrent revisions.

pbastia · 2024-02-29T21:55:17Z

Needs rebase. I'll have a look early next week!

pbastia

Great work!
There's a few things we need to adjust I think, but the logic is sound and it seems like it'll work. Also it looks like we're missing some tests at the end?

A note about postgres count() function, not that it matters in this case since we have at most a few rows to count, but count(expression) is less performant than count(*) since the postgres query planner recognizes this as "count everything without evaluating the expression every row"

pbastia · 2024-03-07T18:30:52Z

schema/deploy/mutations/commit_form_change_internal.sql

@@ -1,10 +1,17 @@
 -- Deploy cif:mutations/commit_form_change to pg
 begin;

-create or replace function cif_private.commit_form_change_internal(fc cif.form_change)
+-- We need to explicitly drop the old function here since we're changing the signature.


if if if ...

Add a comment in here and a tech debt card to refactor this in a more OOP way

pbastia · 2024-03-07T20:35:48Z

schema/deploy/mutations/commit_form_change.sql

+    (select row(form_change.*)::cif.form_change from cif.form_change where id = row_id),
+    (select id  from cif.project_revision
+      where project_id=(select project_id from cif.project_revision where id = form_change_patch.project_revision_id)
+      and change_status = 'pending' and id != form_change_patch.project_revision_id limit 1)


limit 1

As a thought, what if there were more than one? Should we log something? Report an error? flag something for Sentry?

These constraints should prevent multiple from being possible, but what would we do when we log? Prevent the revision from being commit?

Since we have those constraints I don't think we even need to say limit 1 then. Maybe just a comment that we have unique indices would be good?

pbastia · 2024-03-08T01:00:10Z

schema/deploy/mutations/commit_form_change_internal.sql

+          project_revision_id => pending_project_revision_id,
+          json_schema_name => fc.json_schema_name,
+          new_form_data => (fc.new_form_data || format('{"contactIndex": %s}',
+            (select max(new_form_data ->> 'contactIndex')::int from cif.form_change


max(text value) uses alphabetical order so we'd have an index of '3' ranking above '11' or '25', the cast needs to happen inside the call

Suggested change

(select max(new_form_data ->> 'contactIndex')::int from cif.form_change

(select max((new_form_data ->> 'contactIndex')::int) from cif.form_change

pbastia · 2024-03-08T17:02:44Z

schema/deploy/mutations/commit_form_change_internal.sql

+          project_revision_id => pending_project_revision_id,
+          json_schema_name => fc.json_schema_name,
+          new_form_data => (fc.new_form_data || format('{"reportingRequirementIndex": %s}',
+            (select max(new_form_data ->> 'reportingRequirementIndex')::int from cif.form_change 


Suggested change

(select max(new_form_data ->> 'reportingRequirementIndex')::int from cif.form_change

(select max((new_form_data ->> 'reportingRequirementIndex')::int) from cif.form_change

pbastia · 2024-03-08T17:22:04Z

schema/deploy/mutations/commit_form_change_internal.sql

+          project_revision_id => pending_project_revision_id,
+          json_schema_name => fc.json_schema_name,
+          new_form_data => (fc.new_form_data || format('{"reportingRequirementIndex": %s}',
+            (select max(new_form_data ->> 'reportingRequirementIndex')::int from cif.form_change 


Suggested change

(select max(new_form_data ->> 'reportingRequirementIndex')::int from cif.form_change

(select max((new_form_data ->> 'reportingRequirementIndex')::int) from cif.form_change

pbastia · 2024-03-08T20:11:27Z

schema/test/unit/functions/jsonb_minus_test.sql

+select is(
+  (select * from cif.jsonb_minus('{"a": 1, "b": 0, "c": 1, "d": null}'::jsonb, '{"a": 0, "b": 0}'::jsonb)),
+  '{"a": 1, "c": 1, "d": null}'::jsonb,
+  'Added fields are included in the retuen value, including when their value is null'


Suggested change

'Added fields are included in the retuen value, including when their value is null'

'Added fields are included in the return value, including when their value is null'

pbastia · 2024-03-08T20:13:36Z

schema/test/unit/functions/jsonb_minus_test.sql

+  '{"a": 1, "c": 1, "d": null}'::jsonb,
+  'Added fields are included in the retuen value, including when their value is null'
+);
+


missing a test to make sure extra fields in the second param (subtrahend, boy what a word) are not added:

select is( (select * from cif.jsonb_minus('{"a": 1, "b": 0}'::jsonb, '{"a": 1, "b": 2, "d": "test value"}'::jsonb)), '{"b": 0}'::jsonb, 'Extra fields in the subtrahend are properly ignored' );

pbastia · 2024-03-08T20:16:03Z

schema/deploy/functions/jsonb_minus.sql

+-- This behaviour fits our needs at the time of writing this, so the additional complexity of handling the other cases is not needed.
+
+
+create or replace function cif.jsonb_minus(minuend jsonb, subtrahend jsonb)


It would be fun for this one (but just fun, not that we have to do it here) to declare the - operator for the jsonb type with this function
https://www.postgresql.org/docs/current/sql-createoperator.html

It would! This is cool I didn't know that you could add them like this

pbastia · 2024-03-08T21:20:01Z

schema/test/unit/mutations/commit_form_change_internal_test.sql

+-- project_manager  projectManagerLabelId 1 is update and 2 is created in pending
+-- reporting_requirement should be 3 total with 1,2,3 for reportingRequirementIndex
+-- milestone should be 3 total with 1,2,3 for reportingRequirementIndex
+-- attachment


It looks like we're missing some assertions?

pbastia · 2024-03-08T21:21:36Z

schema/test/unit/mutations/commit_form_change_test.sql

@@ -68,7 +68,7 @@ select results_eq(
      null::int,
      'pending'::varchar,
      'reporting_requirement'::varchar,
-      '["hazErrors"]'::jsonb
+      '[]'::jsonb


any reason why we removed the errors?

These came out to try to determine why this doesn't throw an error.

if fc.validation_errors != '[]' then raise exception 'Cannot commit change with validation errors: %', fc.validation_errors; end if;

In commit_form_change_internal that clause right inside of the do block should be throwing when it sees those errors, but does not, it simply commits the form change which the rest of the tests confirm. I might be missing something, this was from the original tests so maybe behaviour elsewhere has changed that affected this

pbastia · 2024-03-25T17:44:09Z

schema/deploy/mutations/commit_form_change_internal.sql

+*/
+      if pending_minus_pendings_parent is not null then
+        update cif.form_change set
+          operation = 'create'::cif.form_change_operation,
+          form_data_record_id = null,
+          previous_form_change_id = null
+        where id = pending_form_change.id;
+
+      else
+/*
+  If pending has not made changes to the form data, the pending record can be deleted as it never would have been made
+*/
+        delete from cif.form_change
+        where project_revision_id = pending_project_revision_id
+          and form_data_table_name = fc.form_data_table_name
+          and form_data_record_id = fc.form_data_record_id;
+      end if;


pbastia · 2024-03-25T17:59:13Z

schema/test/unit/concurrent_revisions/conflicting_creates_test.sql

+  (select (new_form_data ->> 'provinceSharePercentage')::int from cif.form_change where project_revision_id = 2 and json_schema_name = 'funding_parameter_EP'),
+  1,
+  'When committing and pending have both created a funding parameter form, pending maintains its form values'
+);


This is a tricky one. If a value is not set by the amendment in the funding parameter form, but set in the general revision, would a user expect that to be reflected in the amendment?
This is probably a rare edge case where there might be the impression that data was lost. This can probably be resolved with some training.

It's a very good question. I leaned towards "the amendment is right" when I wasn't sure but I'd love to know where Business Area sit on the specific cases.

Probably careful examination of the changes before submitting an amendment or a revision would do the trick

pbastia

Fantastic job with the testing especially!!

…tting form change

…on is pending on the project

…rnal

… elsewhere

…_change

…nding has not

…isions are committed

… operations

…tting creates them

…pending form changes

…_internal

…ct tests to them

…te the tests

…ange pendings operation to create and null the record id and previous form change

…e specific test files

…and pending form changes create the same contact index

…s test

chore: run precommit on all files

mikevespi force-pushed the chore/1876-rebase-pending-revisions branch from 41104a7 to 7d30a53 Compare November 22, 2023 18:23

mikevespi marked this pull request as ready for review November 28, 2023 19:36

mikevespi force-pushed the chore/1876-rebase-pending-revisions branch from 218964e to e649d35 Compare March 6, 2024 23:08

pbastia requested changes Mar 8, 2024

View reviewed changes

pbastia reviewed Mar 25, 2024

View reviewed changes

pbastia approved these changes Mar 25, 2024

View reviewed changes

mikevespi added 22 commits April 2, 2024 15:36

chore: add jsonb minus

52baf12

chore: rework setup for commit_form_change_internal

39b47cd

chore: pass correct arguments to commit form change internal

921f579

chore: rebase changes made in pending form change on top of the commi…

983ad50

…tting form change

chore: handle the merging of form changes when another project revisi…

34c77b9

…on is pending on the project

test: initial tests for concurrent revisions merging

9e5ee2f

test: test the updates of the simple forms in commit_form_change_inte…

52e2301

…rnal

chore: fix revert sqitch function

6a1bcad

chore: fix signature in rever

371807f

chore: explicitly drop the old function signature

ac8c74d

test: add create and mutual update tests

e83dfee

chore: explicitly drop other function signature in revert file

29afc50

chore: fix db deploy

d4d974e

chore: switch test check from form change to project table, fc tested…

d8f52b0

… elsewhere

test: explicit argument list in function should exist test

bbddafc

test: typecast values in test resturn

e461f80

chore: fix quotes in pg test

1723f74

chore: handle the new commit_form_change_internal args in commit_form…

c51a4c3

…_change

test: test the case when the commiting fc has made changes but the pe…

1b0669b

…nding has not

test: test that attachments are handled correctly when concurrent rev…

8527c05

…isions are committed

chore: set the previous_form_change_id on pending form changes

c23fe07

test: check previous_form_change_id and form_data_record_id on create…

a16a838

… operations

mikevespi added 22 commits April 2, 2024 15:37

docs: add start of concurrent revision handling documentation

e1152b0

docs: update concurrent revision doc

d99a18e

chore: address PR comments

68b0b93

chore: set the operation of pending form changes to update when commi…

ee1108e

…tting creates them

test: add tests for adding project_managers with pending revision

15e9846

test: tests for adding reporting requirements in both committing and …

fd25539

…pending form changes

chore: fix update previous_form_change_id scope in commit_form_change…

90550cf

…_internal

chore: remove redundant limit 1

ef243d8

chore: added comments for clarity

f9d8947

chore: updated function comment to be more accurate

bf476bf

test: update the milestone concurrent revision tests

c728566

test: move concurrent revision tests to a separate file and add conta…

bb2240f

…ct tests to them

test: add directory of tests related to concurrent revisions and upda…

3a96df5

…te the tests

chore: when committing archives something that pending has updated ch…

c69066b

…ange pendings operation to create and null the record id and previous form change

chore: remove test file that whose tests have been distributed to mor…

0a7101d

…e specific test files

chore: clean out unrelated items from test

85ce6b4

chore: cleanup commit_form_change_internal

5c1c620

chore: fix contact index clashes from happening when both committing …

3a592c8

…and pending form changes create the same contact index

test: fix contact scenarios in concurrent revision test

94b1142

test: add emission_intensity and funding_parameter tests

59d154d

test: confirm form_data_record_ids are being set in concurrent create…

f15e533

…s test

test: add tests for concurrent revisions that discard form changes

1a2ac24

chore: run precommit on all files

mikevespi force-pushed the chore/1876-rebase-pending-revisions branch from 70040ea to 1a2ac24 Compare April 2, 2024 22:37

mikevespi merged commit 00bf683 into develop Apr 8, 2024
17 checks passed

mikevespi deleted the chore/1876-rebase-pending-revisions branch April 8, 2024 17:06

bc-cas-shipit bot requested a deployment to dev April 8, 2024 17:25 Pending

bc-cas-shipit bot requested a deployment to dev April 8, 2024 23:15 Pending

bc-cas-shipit bot requested a deployment to dev April 9, 2024 01:01 Pending

bc-cas-shipit bot temporarily deployed to test April 18, 2024 17:52 Inactive

bc-cas-shipit bot temporarily deployed to prod May 23, 2024 22:13 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore/1876 rebase pending revisions #1882

Chore/1876 rebase pending revisions #1882

mikevespi commented Nov 9, 2023

pbastia commented Feb 29, 2024 •

edited

Loading

pbastia left a comment

pbastia Mar 7, 2024

mikevespi Mar 13, 2024

pbastia Mar 7, 2024

mikevespi Mar 12, 2024

pbastia Mar 13, 2024

pbastia Mar 8, 2024

pbastia Mar 8, 2024

pbastia Mar 8, 2024

pbastia Mar 8, 2024

pbastia Mar 8, 2024

pbastia Mar 8, 2024

mikevespi Mar 12, 2024

pbastia Mar 8, 2024

pbastia Mar 8, 2024

mikevespi Mar 12, 2024

pbastia Mar 25, 2024

pbastia Mar 25, 2024

mikevespi Mar 25, 2024

pbastia Mar 25, 2024

pbastia left a comment

	(select max(new_form_data ->> 'contactIndex')::int from cif.form_change
	(select max((new_form_data ->> 'contactIndex')::int) from cif.form_change

	(select max(new_form_data ->> 'reportingRequirementIndex')::int from cif.form_change
	(select max((new_form_data ->> 'reportingRequirementIndex')::int) from cif.form_change

	'Added fields are included in the retuen value, including when their value is null'
	'Added fields are included in the return value, including when their value is null'

		-- This behaviour fits our needs at the time of writing this, so the additional complexity of handling the other cases is not needed.


		create or replace function cif.jsonb_minus(minuend jsonb, subtrahend jsonb)

Chore/1876 rebase pending revisions #1882

Chore/1876 rebase pending revisions #1882

Conversation

mikevespi commented Nov 9, 2023

pbastia commented Feb 29, 2024 • edited Loading

pbastia 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

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

pbastia left a comment

Choose a reason for hiding this comment

pbastia commented Feb 29, 2024 •

edited

Loading