Iplementing rjsf form, rewriting to functional components #3043

alexcottner · 2023-10-27T20:39:23Z

This pull request:

Implements the @rjsf/bootstrap4 package in place of kinto-admin-form. See Swap kinto-admin-form with rjsf/bootstrap-x? #2118
Rewrites our components to be functional components. See Migrate away from container components in favor of functional compomnents with react-redux hooks #3026 .
Refactors some of the larger components to be smaller and more manageable for those of us with bad short term memories.
Fixes a few unit tests, mainly around the AuthForm component. But I still would like to do a larger rewrite for this and all unit tests in the near future.

There are also a couple freebies we get:

Validate collection JSON schema. See Validate provided collection JSON schema #75
Refactor JSONEditor component. See Refactor the JSONEditor component #237

This PR does not completely move away from redux or untangle us from the other dependencies we want to remove. However I think this is a good foundation to start with that should make that work easier in the near future.

This PR is huge but I would like to see it implemented as a whole if we can. I have tried to organize the commits into manageable pieces that can be reviewed independently. Requests for additional changes will be made in new commits for easy tracking.

…s and test failures. Started converting to functional components.

…ts. Added rjsf types.

…sf types.

…pes.

…ypes.

…types.

…pes.

leplatrem

Niiiiice!
Are there areas where you need us to concentrate more?

I made a first pass, could not review every detail thoroughly because my brain tends to scroll changes too fast, but I ran it on https://remote-settings-dev.allizom.org/v1/ and found a few glitches

title of Authentication method field is not bold
Drop down is a bit higher than form line

Pop up after login success/error

records list seems broken

A non-serializable value was detected in an action, in the path: `next`. Value: 
BoundFunctionObject { … }
 
Take a look at the logic that dispatched this action:  
Object { type: "COLLECTION_HISTORY_SUCCESS", entries: (1) […], hasNextPage: false, next: BoundFunctionObject, … }

in DEV we can create collections. The permission seems to "lost" with this PR

leplatrem · 2023-10-30T10:20:23Z

src/components/AuthForm.tsx

      },
      server: {
        ...schema.properties.server,
        default: serverURL,
      },
    },
  };
+  return foo;


nitpick: form?

Should have removed that, I was debugging something and wanted to look at the object structure before it was returned. Removing in next commit.

leplatrem · 2023-10-30T10:51:10Z

src/components/ServerHistory.tsx

+
+  const select = useCallback(
+    server => event => {
+      console.log("server select");


Yup, removing in next commit.

leplatrem · 2023-10-30T10:51:39Z

src/components/ServerHistory.tsx

+
+  const onServerChange = useCallback(
+    event => {
+      console.log("onServerChange");


Yup, removing in next commit.

leplatrem · 2023-10-30T10:54:01Z

src/components/collection/JSONCollectionForm.tsx

-    const collectionData = { ...JSON.parse(formData.data), id: formData.id };
-    this.props.onSubmit({ formData: collectionData });
+  }) => {
+    console.log(formInput);


Yup, removing in next commit.

leplatrem · 2023-10-30T10:56:45Z

src/components/collection/JSONCollectionForm.tsx

+      schema={schema}
+      uiSchema={_uiSchema}
+      formData={formDataSerialized}
+      validator={validator}


I imagine that this would fix:

Validate provided collection JSON schema #75
And may make this Refactor the JSONEditor component #237 obsolete

Sweet, will add these to the description.

leplatrem · 2023-10-30T11:04:46Z

test/components/AuthForm_test.js

@@ -254,7 +222,7 @@ describe("AuthForm component", () => {
        target: { value: "http://test.server/v1" },
      });
      expect(serverField.value).eql("http://test.server/v1");
-      expect(authTypeField.value).eql("anonymous");
+      expect(authTypeField.value).eql("openid-google");


out of curiosity? Did some order change?

The test was sort-of broken before because of a quirk in how the component worked. In the old authForm it would default to anonymous auth after you changed servers, and an async event would search history (from localStorage) and update the value again if it found a match (through componentDidUpdate). But this test didn't go further to check the updated match was found later.

In the new code this happens in one step. If we have history for the new server we immediately default to the previous auth method. So I had to update the unit test to check the immediate value we're setting.

leplatrem · 2023-10-30T11:05:01Z

test/setup-tests.js

+// Form.defaultProps = {
+//   ...Form.defaultProps,
+//   safeRenderCompletion: true,
+// };


Yes, good catch. I commented this out and forgot to cleanup later. Deleting in next commit.

leplatrem · 2023-10-30T11:22:41Z

src/components/bucket/CollectionDataList.tsx

+
+import { Gear } from "react-bootstrap-icons";
+import { Justify } from "react-bootstrap-icons";
+import { ClockHistory } from "react-bootstrap-icons";


We don't want to group them?

Huh, they used to be like that in BucketCollections.tsx and I didn't fix it when I copied/pasted over. I guess that's what happens when I update 70 components in a few days. Fixing in next commit.

leplatrem · 2023-10-30T12:16:53Z

src/components/collection/CollectionHistory.tsx

-      history: { entries, loaded, hasNextPage },
-    } = collection;
+  useEffect(() => {
+    console.log(`collectionHistory useEffect: ${props.location}`);


Yup, fixing in next commit.

leplatrem · 2023-10-30T12:54:54Z

src/components/group/GroupForm.tsx

+      const attributes = JSON.parse(data);
+      propOnSubmit({
+        ...omit(formData, ["data"]),
+        ...omit(attributes, ["members"]),


We don't want to keep the comment about why we omit this field here?

Woops, putting it back.

alexcottner · 2023-10-30T15:07:54Z

Niiiiice! Are there areas where you need us to concentrate more?

I'm most concerned around things that were hard for me to test locally. The unit tests passed, but I don't have 100% faith in those by any means. Things like:

All the authentication form stuff.
The review/approval processes.
Users/groups/permissions stuff.

I made a first pass, could not review every detail thoroughly because my brain tends to scroll changes too fast, but I ran it on https://remote-settings-dev.allizom.org/v1/ and found a few glitches

Perfect, I will start going through these notes so far. I see you mentioned some styling changes, these are mostly from the @rjsf/bootstrap4 default theming. I only made a few small changes to the default styling so far, but I'm more than happy to make it closer to what we had before.

I'm going to start going through the rest of your feedback today and get things fixed up though. Thanks for looking at it already!

alexcottner · 2023-10-30T20:47:01Z

* records list seems broken

A non-serializable value was detected in an action, in the path: `next`. Value: 
BoundFunctionObject { … }
 
Take a look at the logic that dispatched this action:  
Object { type: "COLLECTION_HISTORY_SUCCESS", entries: (1) […], hasNextPage: false, next: BoundFunctionObject, … }

This is actually happening for me in the current code as well. Can anybody confirm? It doesn't appear to break the records list for me though.

alexcottner · 2023-10-30T21:38:20Z

Fixed most issues, need to dig into the permissions issue some more. I can repro that against the remote-settings-dev test site.

…cks to fix errors with older code. Responding to first round of PR feedback.

alexcottner · 2023-10-31T16:46:08Z

I think I've resolved all known issues. Some issues appeared to exist in the current main branch though. I think we have some older code that isn't playing nice with newer redux, so I've adjusted middle-ware behavior for now.

grahamalama

The unit tests passed, but I don't have 100% faith in those by any means.

Yeah ... that's what makes a PR of this size so concerning.

Our test coverage numbers aren't great for our components and containers:

npx jest --coverage --coverageReporters="text-summary" --collectCoverageFrom="src/{components,containers}/**/*.{ts,tsx}"
...
=============================== Coverage summary ===============================
Statements   : 50.88% ( 546/1073 )
Branches     : 45.05% ( 346/768 )
Functions    : 47.14% ( 157/333 )
Lines        : 50.52% ( 525/1039 )
================================================================================

and there be dragons when converting class components to functional componets, for example when migrating lifecycle methods to useEffect calls.

I would like to see it implemented as a whole if we can

I'll echo @leplatrem by saying that it was a challenge to review a PR of this size thoroughly. Are there maybe specific commits that represent trickier component migrations or conversely, commits that were easier that I don't need to pay too much attention to?

grahamalama · 2023-11-01T14:34:04Z

src/components/Breadcrumbs/Breadcrumbs.tsx

-export const Breadcrumbs = ({ separator }: BreadcrumbsProps) => {
+export default function Breadcrumbs({ separator }: BreadcrumbsProps) {


This change reminds me that we should pick a lane when it comes to default vs named exports. I prefer named exports, personally, but we can bikeshed that at another time 😉.

That being said, was there as reason we changed existing components in this PR? I get going with what you like for new components, but maybe we could leave existing components alone for this PR. That would be one thing we could do to make the overall diff smaller.

I am indifferent to which pattern we follow for named vs default exports. In this specific case with the Breadcrumbs component, I was just trying to follow the "if you're only exporting one thing, make it default" pattern that we have in most of the code.

Similarly, I made everything export a function rather than declaring a const and exporting it later. in code I don't really have a strong opinion here either, just picked a direction and went with it for consistency. I think exporting the function is slightly cleaner in that it's slightly fewer characters. But I can see the benefit to having one export statement at the end of a file too.

grahamalama · 2023-11-01T14:37:59Z

src/components/bucket/BucketAttributes.tsx

+  capabilities,
+  updateBucket,
+  deleteBucket,
+}) {


I'm surprised that typescript doesn't complain about us not typing these props, which we were doing with the old component. Can we put that back and look for other places where we might have forgotten to type the props?

Woops, putting that back in.

alexcottner · 2023-11-01T20:22:00Z

First - I'm sorry for the 4k+ line delta. I maybe went a little hard on cleanup, although I'm already finding it easier to work in the repo on my next PR.

We can merge this a bit at a time if that gives us more confidence. I made the commits whole pieces, and I can rewrite history to bring in the bugfixes with them as well if we want to go this route. Although if we don't have a deployment that will immediately use what we've merged this may not be beneficial.

Second - The two commits that I think need the most scrutiny are the 1st one (with AuthForm.tsx) and the 6th one (signoff components). AuthForm had the most difficult to understand change due to componentDidUPdate and all the redux/session state changes. And the sign-off functionality probably (definitely) has some more functionality to it than I think. The rest of the commits felt pretty light in comparison.

Third - The biggest banana peel when converting from class components to functions is rewriting componentDidUpdate functions, at least for me. Everything else is a pretty straight forward conversion (ex: class functions just become normal functions). There are 9 components that used to have a componentDidUpdate function in them that I had to rewrite. Most of these were pretty simple but there is always a chance I missed something in a cut/paste.

leplatrem · 2023-11-02T10:04:33Z

I made some tests this morning against the DEV server.

I could log / log out successfully with all methods
I could request review / rollback on the product-integrity collection
I could navigate everywhere randomly without error (my only issue was actually this [signoff] List of records is not refreshed after rollback #2361)

Only real issue is this one:

Collection creation form is still marked as You don't have the required permission to edit this collection.. Whereas https://remote-settings-dev.allizom.org/v1/admin/#/buckets/main-workspace/collections/create looks good
This is not a major feature, we can fix in a follow-up if that helps.

I only saw this apparence issue, which is minor and can be fixed in a follow-up:

As for testing and reviewing, I think we reached an acceptable state in this PR. And since master has a lot more issues than this branch, I would say that we don't too much risk merging (since I could perform most actions successfully).

Before shipping this to PROD, we can ask our users to try it thoroughly on our STAGE instance, making sure it doesn't break their workflow

alexcottner · 2023-11-02T18:04:53Z

Creating a followup on the permission error and merging. Will have some more PR's to follow up on this quickly.

alexcottner added 7 commits October 27, 2023 14:31

Implemented rjsf package for our forms. Fixed immediate styling issue…

0c9ec38

…s and test failures. Started converting to functional components.

Migrated breadcrumbs and all bucket components to functional componen…

e9a2cdc

…ts. Added rjsf types.

Migrated all collection components to functional components. Added rj…

413192b

…sf types.

Migrated all group components to functional components. Added rjsf ty…

c49be63

…pes.

Migrated all record components to functional components. Added rjsf t…

fa8f2b0

…ypes.

Migrated all signoff components to functional components. Added rjsf …

82ced35

…types.

Migrated top level components to functional components. Added rjsf ty…

64b64b4

…pes.

leplatrem reviewed Oct 30, 2023

View reviewed changes

Simplified CollectionForm permissions check. Disabling some redux che…

7813daa

…cks to fix errors with older code. Responding to first round of PR feedback.

grahamalama reviewed Nov 1, 2023

View reviewed changes

Added missing type BucketAttributes properties

ae3def5

leplatrem approved these changes Nov 2, 2023

View reviewed changes

alexcottner merged commit 52da91b into Kinto:master Nov 2, 2023
6 checks passed

alexcottner deleted the functional-components branch November 2, 2023 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iplementing rjsf form, rewriting to functional components #3043

Iplementing rjsf form, rewriting to functional components #3043

alexcottner commented Oct 27, 2023 •

edited

Loading

leplatrem left a comment •

edited

Loading

leplatrem Oct 30, 2023

alexcottner Oct 30, 2023

leplatrem Oct 30, 2023

alexcottner Oct 30, 2023

leplatrem Oct 30, 2023

alexcottner Oct 30, 2023

leplatrem Oct 30, 2023

alexcottner Oct 30, 2023

leplatrem Oct 30, 2023

alexcottner Oct 30, 2023

leplatrem Oct 30, 2023

alexcottner Oct 30, 2023

leplatrem Oct 30, 2023

alexcottner Oct 30, 2023

leplatrem Oct 30, 2023

alexcottner Oct 30, 2023

leplatrem Oct 30, 2023

alexcottner Oct 30, 2023

leplatrem Oct 30, 2023

alexcottner Oct 30, 2023

alexcottner commented Oct 30, 2023 •

edited

Loading

alexcottner commented Oct 30, 2023

alexcottner commented Oct 30, 2023

alexcottner commented Oct 31, 2023

grahamalama left a comment

grahamalama Nov 1, 2023

alexcottner Nov 1, 2023

grahamalama Nov 1, 2023

alexcottner Nov 1, 2023

alexcottner commented Nov 1, 2023

leplatrem commented Nov 2, 2023

alexcottner commented Nov 2, 2023

		export const Breadcrumbs = ({ separator }: BreadcrumbsProps) => {
		export default function Breadcrumbs({ separator }: BreadcrumbsProps) {

Iplementing rjsf form, rewriting to functional components #3043

Iplementing rjsf form, rewriting to functional components #3043

Conversation

alexcottner commented Oct 27, 2023 • edited Loading

leplatrem left a comment • edited Loading

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

alexcottner commented Oct 30, 2023 • edited Loading

alexcottner commented Oct 30, 2023

alexcottner commented Oct 30, 2023

alexcottner commented Oct 31, 2023

grahamalama 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

alexcottner commented Nov 1, 2023

leplatrem commented Nov 2, 2023

alexcottner commented Nov 2, 2023

alexcottner commented Oct 27, 2023 •

edited

Loading

leplatrem left a comment •

edited

Loading

alexcottner commented Oct 30, 2023 •

edited

Loading