Feat: multiple project managers #304

dleard · 2022-02-28T22:48:13Z

Allows adding multiple managers to a project.

addManagerToRevision and deleteManagerFromRevision mutations have a query in the mutation return. The record returned from projectManagerFormChangesByLabel is a composite return value that doesn't represent an actual single database record. This means that we can't use the connection directive to update the data. We may want to look into subscriptions at some point. I think they may end up being necessary to handle concurrent editing.

We could also look into using refetchable fragments to refresh the ProjectManagerForm fragment, though it kind of sounds like this is essentially the same as adding a query to the mutation return.

matthieu-foucault · 2022-03-03T00:24:18Z

app/components/Form/ProjectManagerForm.tsx

+              {projectRevision.managerFormChanges.edges.map(({ node }) => (
+                <React.Fragment key={node.projectManagerLabel.id}>
+                  <Grid.Row>
+                    <label>{node.projectManagerLabel.label}</label>


Suggested change

<label>{node.projectManagerLabel.label}</label>

<label htmlFor={your_select_id}>{node.projectManagerLabel.label} (optional)</label>

In the current behaviour, all managers are optional, so this should be reflected in the label.
We do have a FieldLabel component, which could be reused here. At least the getRequiredLabel function could be reused.

app/components/Form/ProjectManagerForm.tsx

matthieu-foucault · 2022-03-03T00:51:15Z

app/components/Form/ProjectManagerForm.tsx

+                        size="small"
+                        onClick={() => deleteManager(node.formChange?.id)}
+                      >
+                        Clear


should be disabled if there is no manager selected, or if the mutation is already in flight for this manager. You may need to refactor the react fragment into a separate component to track individual mutations

As an alternative, we could also make the clear button a noop if the mutation is in flight or if there is no manager selected, I don't think that would harm the UX, things would be cleared again from the user's perspective

matthieu-foucault · 2022-03-03T00:55:05Z

app/next-env.d.ts

@@ -1,4 +1,5 @@
 /// <reference types="next" />
+/// <reference types="next/types/global" />


you need to update your local yarn packages. This was removed in the latest nextjs update

pbastia · 2022-03-03T01:06:51Z

app/tests/unit/components/Form/ProjectManagerForm.test.tsx

+      },
+    });
+  });
+


We're missing a test that makes sure the updateFormChangeMutation is called when we update a dropdown field without clearing it

pbastia

Nice! 👌 Just missing a small test

matthieu-foucault · 2022-03-04T16:30:29Z

app/mutations/Manager/addManagerToRevision.ts

+        projectRevision(id: $projectRevision) {
+          managerFormChanges: projectManagerFormChangesByLabel {
+            edges {
+              node {
+                projectManagerLabel {
+                  id
+                  rowId
+                  label
+                }
+                formChange {
+                  id
+                  newFormData
+                }
+              }
+            }


https://bcgov-cas.promyze.app/#/CAS%20GGIRCS/home?practiceId=61e1eb156e4cd70fe286ec9f
You could reuse ProjectManagerForm_query here, or, if that fragment was split in two, ProjectManagerForm_projectRevision

matthieu-foucault · 2022-03-04T17:15:26Z

app/components/Form/ProjectManagerForm.tsx

+  // Delete a manager from the project revision
+  const [discardFormChange] = useDeleteManagerFromRevisionMutation();
+  const deleteManager = (id: string) => {
+    discardFormChange({


The action here needs to depend on the form_change operation:

If operation === "CREATE", then you can discard it, as the manager was added in the same revision

If operation === "UPDATE", then the form_change.operation should be updated to ARCHIVE

matthieu-foucault

Haven't done a pass on the code yet, but there's an issue with the screenshots:

"required" validation should not show if this is optional

pbastia · 2022-03-09T00:10:46Z

app/components/Form/ProjectManagerForm.tsx

+    // If a form_change exists, and the payload does not contain a cifUserId delete it
+    else if (formChangeId && !formChange.cifUserId && !discardInFlight) {
+      deleteManager(formChangeId);
+    }


I feel this entire pattern of creating / updating / deleting form changes depending on whether we have a certain field or not will be reused over and over, just a noting a thought for later on how we could reuse that logic

Yes, this may be refactored into a hook. We can do that after we have two examples of this pattern (there will be a second one in #280 )

matthieu-foucault

-- nvm -- reviewing too many PRs in parallel

pbastia · 2022-03-09T00:18:02Z

app/pages/cif/project-revision/[projectRevision]/index.tsx

+              revision={query.projectRevision}
+              projectManagerFormRef={projectManagerFormRef}
+              setValidatingForm={(validator) =>
+                (projectManagerFormRef.current = validator)


I'd really like to find better wording for this function / callback if you have ideas, I'm quite unhappy with what I chose when I first implemented it.
This is just confusing

Too many tabs, wrong PR

pbastia

looks good! I'll let @matthieu-foucault approve if he has more comments

matthieu-foucault · 2022-03-09T00:52:23Z

app/components/Form/ProjectManagerForm.tsx

-        allCifUsers {
-          edges {
-            node {
-              rowId
-              firstName
-              lastName
-            }
-          }


Why is this removed? You should have two fragments instead of passing allCifUsers as a prop, no? You can use useFragment multiple times in the same component.

matthieu-foucault · 2022-03-09T00:55:44Z

app/components/Form/ProjectManagerFormGroup.tsx

+  const { allCifUsers } = useFragment(
+    graphql`
+      fragment ProjectManagerFormGroup_query on Query {
+        allCifUsers {
+          edges {
+            node {
+              rowId
+              firstName
+              lastName
+            }
+          }
+        }
+      }
+    `,
+    props.query
+  );


Why is this fragment here and not in the child component?

matthieu-foucault · 2022-03-09T00:57:38Z

app/components/Form/ProjectManagerFormGroup.tsx

+        :global(button.pg-button) {
+          margin-left: 0.4em;
+          margin-right: 0em;
+        }
+        :global(.right-aligned-column) {
+          display: flex;
+          justify-content: flex-end;
+          align-items: flex-start;
+        }


See #324 (comment)
I think that we need to use an outer div to scope those styles, otherwise they're no better than if you used <style jsx global> (I could be wrong though, haven't fully tested this)

app/data/jsonSchemaForm/projectManagerSchema.ts

matthieu-foucault · 2022-03-09T17:44:28Z

app/components/Form/ProjectManagerForm.tsx

+    <>
+      <React.Fragment key={change.projectManagerLabel.id}>


Don't need those two fragments

dleard marked this pull request as ready for review February 28, 2022 23:02

matthieu-foucault suggested changes Mar 3, 2022

View reviewed changes

matthieu-foucault reviewed Mar 3, 2022

View reviewed changes

pbastia reviewed Mar 3, 2022

View reviewed changes

pbastia requested changes Mar 3, 2022

View reviewed changes

dleard force-pushed the feat/multiple-project-managers branch from 60161af to aef7b35 Compare March 4, 2022 00:12

matthieu-foucault reviewed Mar 4, 2022

View reviewed changes

matthieu-foucault suggested changes Mar 4, 2022

View reviewed changes

matthieu-foucault mentioned this pull request Mar 8, 2022

Feat/project details #323

Merged

dleard force-pushed the feat/multiple-project-managers branch from fb9f614 to 3ef1b05 Compare March 8, 2022 21:31

matthieu-foucault suggested changes Mar 9, 2022

View reviewed changes

pbastia reviewed Mar 9, 2022

View reviewed changes

matthieu-foucault previously approved these changes Mar 9, 2022

View reviewed changes

matthieu-foucault self-requested a review March 9, 2022 00:15

pbastia reviewed Mar 9, 2022

View reviewed changes

pbastia previously approved these changes Mar 9, 2022

View reviewed changes

dleard dismissed pbastia’s stale review via 249c692 March 9, 2022 00:44

dleard added 11 commits March 8, 2022 16:45

chore: create_project() does not create a project_manager form change

b874a2d

chore: start updating front-end

a9b55bf

feat: multiple project managers can be added to a revision

1a4318a

chore: add a custom delete mutation

34cfe98

chore: remove old project_manager_form_change computed column

64bb2ad

chore: fix copy-paste error

7774164

test: update projectRevision jest test

43ec23b

test: add unit test for ProjectManagerForm

d5fb237

chore: linting fixes

68af6c6

chore: computed column should ignore changes with an archive operation

ce8e527

refactor: move each individual form into a single fragment component

6a48b3e

dleard added 9 commits March 8, 2022 16:45

chore: fix validation refs

604b619

chore: properly type typescript 'any' types

38ecd71

test: update revision page unit test

d84b2ff

test: add ProjectManager unit tests

a9f4579

test: add test for update mutation

9127dab

chore: fix incorrect interface types

4f70271

test: update cypress test

6e68125

chore: project manager fields are optional

295fa62

test: update cypress test

c82b5c0

dleard force-pushed the feat/multiple-project-managers branch from 249c692 to c82b5c0 Compare March 9, 2022 00:46

matthieu-foucault suggested changes Mar 9, 2022

View reviewed changes

dleard added 2 commits March 9, 2022 09:12

refactor: move allCifUsers fragment into component that uses it

a2607ae

chore: fix typo

db173cf

matthieu-foucault previously approved these changes Mar 9, 2022

View reviewed changes

chore: remove unnecessary fragment

43cbccc

dleard dismissed matthieu-foucault’s stale review via 43cbccc March 9, 2022 17:49

dleard added 2 commits March 9, 2022 09:54

chore: remove unnecessary React.Fragment

6c0b00f

chore: remove unnecessary React import

60f8e08

matthieu-foucault approved these changes Mar 9, 2022

View reviewed changes

matthieu-foucault merged commit e9dd666 into develop Mar 9, 2022

matthieu-foucault deleted the feat/multiple-project-managers branch March 9, 2022 19:42

bc-cas-shipit bot temporarily deployed to dev March 9, 2022 20:16 Inactive

bc-cas-shipit bot had a problem deploying to test March 15, 2022 21:38 Failure

bc-cas-shipit bot temporarily deployed to test March 15, 2022 22:23 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: multiple project managers #304

Feat: multiple project managers #304

dleard commented Feb 28, 2022 •

edited

Loading

matthieu-foucault Mar 3, 2022

matthieu-foucault Mar 3, 2022

pbastia Mar 3, 2022 •

edited

Loading

matthieu-foucault Mar 3, 2022 •

edited

Loading

pbastia Mar 3, 2022

pbastia left a comment

matthieu-foucault Mar 4, 2022

matthieu-foucault Mar 4, 2022

matthieu-foucault left a comment

pbastia Mar 9, 2022

matthieu-foucault Mar 9, 2022

matthieu-foucault left a comment •

edited

Loading

pbastia Mar 9, 2022

pbastia left a comment

matthieu-foucault Mar 9, 2022

matthieu-foucault Mar 9, 2022

matthieu-foucault Mar 9, 2022

matthieu-foucault Mar 9, 2022

	<label>{node.projectManagerLabel.label}</label>
	<label htmlFor={your_select_id}>{node.projectManagerLabel.label} (optional)</label>

		@@ -1,4 +1,5 @@
		/// <reference types="next" />
		/// <reference types="next/types/global" />

Feat: multiple project managers #304

Feat: multiple project managers #304

Conversation

dleard commented Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbastia Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

matthieu-foucault Mar 3, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthieu-foucault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthieu-foucault left a comment • edited Loading

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

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

dleard commented Feb 28, 2022 •

edited

Loading

pbastia Mar 3, 2022 •

edited

Loading

matthieu-foucault Mar 3, 2022 •

edited

Loading

matthieu-foucault left a comment •

edited

Loading