Feature/deseng671: New Summary page in authoring section #2587

jareth-whitney · 2024-09-12T23:15:02Z

Issue #: https://citz-gdx.atlassian.net/browse/DESENG-671

Description of changes:

Feature New Summary page in authoring section 🎟️ DESENG-671
- Fetches values
- Saves values to database
- Returns success and error messages to user after form submission
- Resets form dirty state after submission

User Guide update ticket (if applicable):
N/A

Baelx

Thanks for this Jareth! Please have a look at my comments but the only thing really required is that we update the labels for the body text. The text there seems like it's referring to Section Headings.

Baelx · 2024-09-12T23:21:58Z

met-web/src/components/engagement/admin/create/authoring/AuthoringSummary.tsx


-const AuthoringBanner = () => {
+const AuthoringSummary = () => {


Thanks for this rename! Helps make it clearer that it's the for the page summary section

Baelx · 2024-09-12T23:28:59Z

met-web/src/components/engagement/admin/create/authoring/AuthoringSummary.tsx

+    // Check if the form has succeeded or failed after a submit, and issue a message to the user.
+    const dispatch = useAppDispatch();
+    useEffect(() => {
+        if ('success' === fetcher.data) {


You could potentially save some code like this:

Suggested change

if ('success' === fetcher.data) {

const responseSeverity = 'success' === fetcher.data ? 'success' : 'error';

const responseText = 'success' === fetcher.data ? 'Engagement saved successfully.' : 'Unable to save engagement.';

dispatch(

openNotification({

severity: responseSeverity,

text: responseText,

}),

);

The only issue here is that fetcher.data could be returning other values that aren't 'success' or 'error'. So we could have things that are called 'errors' that are false positives. Previous technique was an if > else if statement. However, this code has inspired me to make the code less verbose, so I'll find a solution.

Thanks for considering my suggestion and no worries if you don't end up reducing code here!

Your original code only handles two cases. So you're saying that there are other responses from the fetcher in which we want the request to complete but no message be displayed to the user? I feel like at the presentation layer, a user would only care about their request being successful or not.

Solution:
useEffect(() => {
if ('success' === fetcher.data || 'failure' === fetcher.data) {
const responseText =
'success' === fetcher.data ? 'Engagement saved successfully.' : 'Unable to save engagement.';
const responseSeverity = 'success' === fetcher.data ? 'success' : 'error';
dispatch(
openNotification({
severity: responseSeverity,
text: responseText,
}),
);
fetcher.data = undefined;
}
}, [fetcher.data]);

Baelx · 2024-09-12T23:32:47Z

met-web/src/components/engagement/admin/create/authoring/AuthoringSummary.tsx

+    };
+
+    // Determines whether a string is JSON parseable.
+    const tryParse = (json: string) => {


Do we do JSON parsing for sending/receiving json data elsewhere in the frontend? Just curious. Maybe this is something that is handled by yup elsewhere?

It looks like you can do something like this with yup but it's pretty much equally long-winded.

Yup.string().test("json", "Invalid JSON format", (value) => {
try {
JSON.parse(value);
return true;
} catch (error) {
return false;
}
}),

Okay yeah, thanks for looking into that! With this big app, I just think it's important that we don't reinvent the wheel on common operations like handling JSON in requests/forms. But it looks like if we're using yup elsewhere then it's about the same code footprint

Baelx · 2024-09-12T23:35:37Z

met-web/src/components/engagement/admin/create/authoring/AuthoringSummary.tsx

                    />
                </label>
            </Grid>

            <Grid sx={{ ...formItemContainerStyles, backgroundColor: colors.surface.blue[10] }} item>
-                <label htmlFor="body_copy">
+                <label htmlFor="editor_state">
                    <MetBigLabel style={metBigLabelStyles} role="document" tab-index="0">
                        Body Copy
                        <span style={{ fontWeight: 'normal' }}> (Required)</span>
                    </MetBigLabel>
                    <FormDescriptionText style={formDescriptionTextStyles}>
                        Your section heading should be descriptive, short and succinct.


I imagine this description needs to be updated for body text?

Funny story, the state of the RichTextArea was very hard to manage because of the Controller. However, I need the Controller because it checks if the form is dirty. So this is actually a dummy field that isn't sent to the API, but it manages the state of the RichTextArea. However, I think I should rename it to something like summary_editor_state because that would be specific to the summary page.

I'm curious about this and I'll message you to learn more!

Sorry I thought we were talking about the editor_state label but you meant the FormDescriptionText text. Fixed.

Baelx · 2024-09-12T23:36:59Z

met-web/src/components/engagement/admin/create/authoring/AuthoringSummary.tsx

+                                    render={({ field }) => {
+                                        return (
+                                            <RichTextArea
+                                                ariaLabel="Body Copy: Your section heading should be descriptive, short and succinct."


Some more directions that seem meant for the section header input!

Baelx · 2024-09-12T23:46:11Z

met-web/src/components/engagement/admin/create/authoring/AuthoringSummary.tsx

+                                                ariaLabel="Body Copy: Your section heading should be descriptive, short and succinct."
+                                                spellCheck
+                                                initialContentState={
+                                                    tryParse(content[0].json_content)


This would accomplish the same thing:

Suggested change

tryParse(content[0].json_content)

tryParse(content[0].json_content) || ''

Actually I'm taking a second look at this and it's not quite the same. The first function just checks if the JSON is valid, the second one actually parses it. I need both unfortunately.

But tryParse returns parsed JSON too, doesn't it?

...and upon third inspection, I think this actually will work, since I am returning the value if it is parseable (I thought I was just returning true).

Baelx · 2024-09-12T23:49:50Z

met-web/src/components/engagement/admin/create/authoring/AuthoringTemplate.tsx

@@ -108,7 +109,8 @@ const AuthoringTemplate = () => {
            <SystemMessage status="warning">
                Under construction - the settings in this section have no effect.
            </SystemMessage>
-            {'details' === slug && (
+
+            <When condition={'details' === slug}>


This looks pretty minimal but I'd say no need to modify the details section any further. I've changed a lot of this in my work so far.

Oh yes, this was the only modification to the Details section. I wanted to make it consistent with the rest of our code, where we use often instead of the && conditional.

Baelx · 2024-09-12T23:51:44Z

met-web/src/components/engagement/admin/create/authoring/AuthoringTemplate.tsx

+
+            <When condition={'summary' === slug}>
+                <Grid container sx={{ maxWidth: '700px', mt: '1rem' }} direction="column">
+                    <WidgetPicker location={1} />


Small thing but would you consider passing the enum so it's a bit more readable?

Suggested change

<WidgetPicker location={1} />

<WidgetPicker location={WidgetLocation.specify_your_location} />

…est WidgetPicker component from authoring tab, due to unit tests failing.

sonarqubecloud · 2024-09-13T21:52:35Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

codecov-commenter · 2024-09-13T21:57:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.27%. Comparing base (6ba4401) to head (bdc0dc9).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2587      +/-   ##
==========================================
- Coverage   75.80%   75.27%   -0.54%     
==========================================
  Files         607      612       +5     
  Lines       21881    22145     +264     
  Branches     1810     1839      +29     
==========================================
+ Hits        16587    16669      +82     
- Misses       5032     5203     +171     
- Partials      262      273      +11

Flag	Coverage Δ
metweb	`64.35% <100.00%> (-0.78%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../components/engagement/admin/view/AuthoringTab.tsx	`27.27% <100.00%> (+1.34%)`	⬆️
met-web/src/models/widget.tsx	`100.00% <100.00%> (ø)`

... and 12 files with indirect coverage changes

Baelx

Looks good thanks!

jareth-whitney added 2 commits September 12, 2024 16:11

feature/deseng671: integrated authoring summary page form.

d5cf794

Co-authored-by: Nat² <NatSquared@users.noreply.github.com>

4990784

jareth-whitney requested review from Baelx and NatSquared September 12, 2024 23:15

Baelx requested changes Sep 13, 2024

View reviewed changes

feature/deseng671: Made small revisions as per PR comments. Removed t…

bdc0dc9

…est WidgetPicker component from authoring tab, due to unit tests failing.

Baelx approved these changes Sep 13, 2024

View reviewed changes

jareth-whitney merged commit 5373540 into main Sep 13, 2024
7 of 8 checks passed

jareth-whitney deleted the feature/deseng671 branch September 13, 2024 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/deseng671: New Summary page in authoring section #2587

Feature/deseng671: New Summary page in authoring section #2587

jareth-whitney commented Sep 12, 2024

Baelx left a comment

Baelx Sep 12, 2024

Baelx Sep 12, 2024

jareth-whitney Sep 13, 2024 •

edited

Loading

Baelx Sep 13, 2024

jareth-whitney Sep 13, 2024

Baelx Sep 12, 2024

jareth-whitney Sep 13, 2024

Baelx Sep 13, 2024

Baelx Sep 12, 2024

jareth-whitney Sep 13, 2024

Baelx Sep 13, 2024

jareth-whitney Sep 13, 2024

Baelx Sep 12, 2024

jareth-whitney Sep 13, 2024

Baelx Sep 12, 2024

jareth-whitney Sep 13, 2024

Baelx Sep 13, 2024

jareth-whitney Sep 13, 2024

Baelx Sep 12, 2024

jareth-whitney Sep 13, 2024

Baelx Sep 12, 2024

jareth-whitney Sep 13, 2024

sonarqubecloud bot commented Sep 13, 2024

codecov-commenter commented Sep 13, 2024

Baelx left a comment


		const AuthoringBanner = () => {
		const AuthoringSummary = () => {

-        if ('success' === fetcher.data) {
+             const responseSeverity = 'success' === fetcher.data ? 'success' : 'error';
+             const responseText = 'success' === fetcher.data ? 'Engagement saved successfully.' : 'Unable to save engagement.';
+             dispatch(
+                openNotification({
+                    severity: responseSeverity,
+                    text: responseText,
+                }),
+            );

	tryParse(content[0].json_content)
	tryParse(content[0].json_content) \|\| ''

	<WidgetPicker location={1} />
	<WidgetPicker location={WidgetLocation.specify_your_location} />

Feature/deseng671: New Summary page in authoring section #2587

Feature/deseng671: New Summary page in authoring section #2587

Conversation

jareth-whitney commented Sep 12, 2024

Baelx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jareth-whitney Sep 13, 2024 • 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

sonarqubecloud bot commented Sep 13, 2024

Quality Gate passed

codecov-commenter commented Sep 13, 2024

Codecov Report

Baelx left a comment

Choose a reason for hiding this comment

jareth-whitney Sep 13, 2024 •

edited

Loading