feat(back-matter): edit backmatter resources #768

tuckerzp · 2023-04-12T17:54:28Z

This adds basic editing functionality for backmatter resource
titles and descriptions.

Our original editing functionality made some assumptions that made
implementing this tricky. Namely, we assumed that the text field we are
updating is the direct child of the field we are sending a PATCH request
for. ¹

As you can see in the code, the way we handle editing is a bit hacky and
naive. We most likely want to revisit this and redo a lot of that code.
This would not be an insignificant change, hence it being pushed for
now. These changes make editing at least work for now so that our other
editing tasks are no longer blocked.

closes EasyDynamics/comply0-private#68

For example, metadata.title updating metadata vs backmatter.resources[i].title updating all of backmatter. ↩

tuckerzp · 2023-04-12T18:27:37Z

Tests are failing because I changed how description is displayed. Should be an easy fix, I will wait until we discuss approach before I fix it.

packages/oscal-react-library/src/components/OSCALBackMatter.js

kylelaker · 2023-04-12T18:56:34Z

Great stuff! This isn't quite as thorough of a rework as I'd like to have seen overall but it gets things working for this task. What we still need is a written description of how to use this framework; can you please write some documentation?

I don't care whether it's docs on those lower-level functions or docs in Markdown written somewhere but we need documentation on how this works because we have a ton of upcoming editing tasks.

packages/oscal-react-library/src/components/OSCALEditableFieldActions.js

kylelaker

Hey! I know that some of these asks go a little beyond your ticket but I am hopeful that we can do a little more cleanup while we're in here to improve the type checking and composability of some of these components. I'd even like to see us be able to export BackMatterDisplay when this is done.

If some of these are too complicated to do now, please be very explicit about the errors that tsc or eslint gives you and we can track them as tech debt to fix later.

Thank you so much for all your work on this so far! I'd approve as-is but I think it'd be good to use this to set a good example for what we're looking for in some of our refactoring as we incrementally adopt TypeScript. Especially since there's at least one bug in this code today that wasn't so obvious until now.

packages/oscal-react-library/src/components/OSCALBackMatter.tsx

kylelaker · 2023-04-13T15:10:14Z

packages/oscal-react-library/src/components/OSCALBackMatter.tsx

+  const backMatterDisplay = (resource: Resource) => (
+    <Grid item xs={3} key={resource.uuid}>


For composability, rather than being a nested function, this almost feels like it should be its own component in the root of the document...

interface EditableFieldProps { isEditable?: boolean; onFieldSave?: (...args: any[]) => void; partialRestData?: Record<string, any>; } interface BackMatterResourceProps extends EditableFieldProps { resource: Resource; } function BackMatterResource(props: BackMatterResourceProps): ReactElement { //... the body of this function }

I have tried to do this and it is not much cleaner. I think this is becuase you can't really edit a resource title without it being generally aware of all other resources. It may be that I am just missing something, so I will push it and see what everyone thinks.

packages/oscal-react-library/src/components/OSCALEditableFieldActions.test.tsx

packages/oscal-react-library/.eslintrc.json

tuckerzp · 2023-04-13T20:03:19Z

As a note to reviewers, the diff has gotten a bit nasty here. While there is really not much change, there was enough with switching to .tsx that some files are not recognized as the same file.

Feel free to comment on specific lines or I can provide a bit more context on the major changes if that is helpful. Hopefully between the pr comment and some of my commit messages, what is happening is clear enough.

packages/oscal-react-library/docs/adding-editing.md

packages/oscal-react-library/src/components/OSCALBackMatter.tsx

packages/oscal-react-library/src/components/OSCALEditableTextField.tsx

tuckerzp · 2023-04-14T16:28:20Z

packages/oscal-react-library/src/components/OSCALBackMatter.tsx

+                <OSCALEditableTextField
+                  fieldName="title"
+                  canEdit={props.isEditable}
+                  editedField={props.isEditable ? [objectKey, "back-matter", "resources"] : null}
+                  editedValue={props.backMatter?.resources}
+                  editedValueId={resource.uuid}
+                  onFieldSave={props.onFieldSave}
+                  partialRestData={
+                    props.isEditable
+                      ? {
+                          [objectKey]: {
+                            uuid: props.partialRestData?.[objectKey].uuid,
+                            "back-matter": props.backMatter,
+                          },
+                        }
+                      : null
+                  }
+                  value={resource.title}
+                />


Actual change here

tuckerzp · 2023-04-14T16:28:30Z

packages/oscal-react-library/src/components/OSCALBackMatter.tsx

+          <OSCALEditableTextField
+            fieldName="description"
+            canEdit={props.isEditable}
+            editedField={props.isEditable ? [objectKey, "back-matter", "resources"] : null}
+            editedValue={props.backMatter?.resources}
+            editedValueId={resource.uuid}
+            onFieldSave={props.onFieldSave}
+            partialRestData={
+              props.isEditable
+                ? {
+                    [objectKey]: {
+                      uuid: props.partialRestData?.[objectKey].uuid,
+                      "back-matter": props.backMatter,
+                    },
+                  }
+                : null
+            }
+            value={resource.description}
+          />


packages/oscal-react-library/src/components/OSCALBackMatter.tsx

packages/oscal-react-library/src/components/OSCALBackMatter.test.tsx

packages/oscal-react-library/src/components/OSCALBackMatter.tsx

Bronstrom

Looking good so far!

Bronstrom · 2023-04-14T17:52:58Z

packages/oscal-react-library/src/components/OSCALBackMatter.test.tsx

-    renderer();
+describe("OSCAL Backmatter", () => {
+  test("displays resource title", () => {
+    render(<OSCALBackMatter backMatter={backMatterTestData} parentUrl={parentUrlTestData} />);


This back matter test data is duplicated throughout the tests. Is there any reason to avoid having a renderer function?

I think that for tests being explicit and a bit repetitive is a good thing. I want to be able to look at a test and understand what is happening without having to look through a bunch of code.

I agree 100% with @tuckerzp. This is a small enough difference that I think it's valuable to just make the call. Additional indirection is not useful here and is actually less helpful for debugging.

I am very much opposed to these styles of "renderer()" functions.

What is the alternative? Calling renderer() over and over? It's still one line repeated many times. Not sure the juice is worth the squeeze.

In fact, I'd argue that our tests should not be using the same data and the same parameters every time; and that would discourage some magic function that solves the problem of repetition. We should solve it by it not existing in the first place

Bronstrom · 2023-04-14T17:53:51Z

packages/oscal-react-library/src/components/OSCALEditableTextField.tsx

+    <Grid item>
      <Typography variant={props.typographyVariant}>{props.value}</Typography>
    </Grid>


I'm wondering how much value this Grid adds. Would removing the Grid and just having props.value within the Typography support the same cause? Or could we use a Stack instead?

kylelaker

This is amazing! Thank you SO MUCH for the TypeScript refactoring and for fixing up other issues as you were in here! It looks like we not only added this feature but fixed a few subtle bugs. I also appreciate you highlighting where the changes actually happened -- that was helpful in this large diff.

Bronstrom

Overall, this looks great! I love all the refactoring choices made to improve the overall structure of editing and the implementations made with TypeScript.

Although, it appears the JSON editor window is blank. Maybe during the refactoring process some properties were lost and not passed to OSCALEditableTextField?

packages/oscal-react-library/src/components/OSCALBackMatter.tsx

Bronstrom · 2023-04-17T19:40:17Z

Also, one other difference I noticed when comparing backmatter resources between this branch and develop, is the DescriptionIcon (looks like a document in the right corner) is missing.

Before:

After:

This adds basic editing functionality for backmatter resource titles and descriptions. Our original editing functionality made some assumptions that made implementing this tricky. Namely, we assumed that the text field we are updating is the direct child of the field we are sending a PATCH request for. For example, `metadata.title` updating `metadata` vs `backmatter.resources[i].title` updating all of `backmatter`. As you can see in the code, the way we handle editing is a bit hacky and naive. We most likely want to revisit this and redo a lot of that code. This would not be an insignificant change, hence it being pushed for now. These changes make editing at least work for now so that our other editing tasks are no longer blocked.

This attempts to document how one would go about adding editing to a text field. This **does not** mean this is the best way to actually do this. To clear up the blocker of editing, this at least makes it possible to edit. We will want to revisit editing at some point in the future.

It is sadly needed here.

Switching to .tsx helped highlight some flaws with the tests. The major difference is that we are no longer testing things in such a wack way. Each component tests itself. The only exception is EditableFieldActions, as that component does not make much sense existing on it's own.

Rather than documenting these variables in an outer .md file, this documents each variable and provides some examples.

tuckerzp · 2023-04-17T20:08:59Z

Although, it appears the JSON editor window is blank. Maybe during the refactoring process some properties were lost and not passed to OSCALEditableTextField?

I think #776 fixes that. develop just has not been merged in. From my understanding, the EditableTextField and the Drawer do not really interact. Should be good now!

the DescriptionIcon (looks like a document in the right corner) is missing.

Yeah it did not make sense to edit a little icon hover box. So description is now a text field.

tuckerzp force-pushed the tuckerzp/edit-backmatter-resources branch from 25d7607 to 7815490 Compare April 12, 2023 18:04

kylelaker reviewed Apr 12, 2023

View reviewed changes

packages/oscal-react-library/src/components/OSCALBackMatter.js Outdated Show resolved Hide resolved

kylelaker reviewed Apr 12, 2023

View reviewed changes

packages/oscal-react-library/src/components/OSCALBackMatter.js Outdated Show resolved Hide resolved

kylelaker reviewed Apr 12, 2023

View reviewed changes

packages/oscal-react-library/src/components/OSCALEditableFieldActions.js Outdated Show resolved Hide resolved

tuckerzp force-pushed the tuckerzp/edit-backmatter-resources branch from 7815490 to 8d92880 Compare April 13, 2023 14:52

kylelaker reviewed Apr 13, 2023

View reviewed changes

tuckerzp force-pushed the tuckerzp/edit-backmatter-resources branch 2 times, most recently from adaa248 to f562c28 Compare April 13, 2023 16:38

tuckerzp marked this pull request as ready for review April 13, 2023 16:57

easy-dynamics-oscal-automation bot requested a review from a team April 13, 2023 16:57

kylelaker reviewed Apr 14, 2023

View reviewed changes

packages/oscal-react-library/docs/adding-editing.md Outdated Show resolved Hide resolved

easy-dynamics-oscal-automation bot requested a review from a team April 14, 2023 13:13

kylelaker reviewed Apr 14, 2023

View reviewed changes

packages/oscal-react-library/src/components/OSCALBackMatter.tsx Outdated Show resolved Hide resolved

packages/oscal-react-library/src/components/OSCALEditableTextField.tsx Outdated Show resolved Hide resolved

easy-dynamics-oscal-automation bot requested a review from a team April 14, 2023 13:32

tuckerzp commented Apr 14, 2023

View reviewed changes

kylelaker reviewed Apr 14, 2023

View reviewed changes

easy-dynamics-oscal-automation bot requested a review from a team April 14, 2023 17:09

Bronstrom reviewed Apr 14, 2023

View reviewed changes

easy-dynamics-oscal-automation bot requested a review from a team April 14, 2023 17:54

kylelaker approved these changes Apr 17, 2023

View reviewed changes

easy-dynamics-oscal-automation bot requested a review from a team April 17, 2023 15:07

Bronstrom requested changes Apr 17, 2023

View reviewed changes

packages/oscal-react-library/src/components/OSCALBackMatter.tsx Outdated Show resolved Hide resolved

packages/oscal-react-library/src/components/OSCALBackMatter.tsx Show resolved Hide resolved

tuckerzp added 4 commits April 17, 2023 16:00

fix linting errors

d81c79e

Switch files to .tsx

9aa3d17

Fix errors with linter

90e688e

tuckerzp added 14 commits April 17, 2023 16:00

Update files to pass tests using .tsx

40c04b7

Add interfaces to prop passing

74073b4

Fix md linting

67038f7

Pass uuid

658240d

It is sadly needed here.

feat(documentation): Document interface variables

0ca38c8

Rather than documenting these variables in an outer .md file, this documents each variable and provides some examples.

Update interface documentation

9f22f8f

make isEditable optional

7c1acdc

Switch to isEditable

78509a6

Use a Stack

5fd8dd0

Fix test names

d45ed87

Remove Grid

e8517b5

Add docs for TitleDisplayProps

43a9b8e

tuckerzp force-pushed the tuckerzp/edit-backmatter-resources branch from 1920130 to 43a9b8e Compare April 17, 2023 20:08

tuckerzp requested a review from Bronstrom April 17, 2023 20:10

Bronstrom approved these changes Apr 17, 2023

View reviewed changes

Bronstrom merged commit 6dc2a53 into develop Apr 17, 2023
5 checks passed

Bronstrom deleted the tuckerzp/edit-backmatter-resources branch April 17, 2023 20:27

kylelaker added the enhancement New feature or request label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(back-matter): edit backmatter resources #768

feat(back-matter): edit backmatter resources #768

tuckerzp commented Apr 12, 2023 •

edited

tuckerzp commented Apr 12, 2023

kylelaker commented Apr 12, 2023

kylelaker left a comment

kylelaker Apr 13, 2023

tuckerzp Apr 13, 2023

tuckerzp commented Apr 13, 2023

tuckerzp Apr 14, 2023

tuckerzp Apr 14, 2023

Bronstrom left a comment

Bronstrom Apr 14, 2023 •

edited

tuckerzp Apr 14, 2023

kylelaker Apr 14, 2023 •

edited

Bronstrom Apr 14, 2023

kylelaker left a comment

Bronstrom left a comment

Bronstrom commented Apr 17, 2023

tuckerzp commented Apr 17, 2023 •

edited

		const backMatterDisplay = (resource: Resource) => (
		<Grid item xs={3} key={resource.uuid}>

feat(back-matter): edit backmatter resources #768

feat(back-matter): edit backmatter resources #768

Conversation

tuckerzp commented Apr 12, 2023 • edited

Footnotes

tuckerzp commented Apr 12, 2023

kylelaker commented Apr 12, 2023

kylelaker left a comment

Choose a reason for hiding this comment

kylelaker Apr 13, 2023

Choose a reason for hiding this comment

tuckerzp Apr 13, 2023

Choose a reason for hiding this comment

tuckerzp commented Apr 13, 2023

tuckerzp Apr 14, 2023

Choose a reason for hiding this comment

tuckerzp Apr 14, 2023

Choose a reason for hiding this comment

Bronstrom left a comment

Choose a reason for hiding this comment

Bronstrom Apr 14, 2023 • edited

Choose a reason for hiding this comment

tuckerzp Apr 14, 2023

Choose a reason for hiding this comment

kylelaker Apr 14, 2023 • edited

Choose a reason for hiding this comment

Bronstrom Apr 14, 2023

Choose a reason for hiding this comment

kylelaker left a comment

Choose a reason for hiding this comment

Bronstrom left a comment

Choose a reason for hiding this comment

Bronstrom commented Apr 17, 2023

tuckerzp commented Apr 17, 2023 • edited

tuckerzp commented Apr 12, 2023 •

edited

Bronstrom Apr 14, 2023 •

edited

kylelaker Apr 14, 2023 •

edited

tuckerzp commented Apr 17, 2023 •

edited