Paginate Operations Form #269

marcellmueller · 2023-11-10T20:51:31Z

Implements #243

Might be good to take an extra close look at the api changes and updates to the endpoint tests since I am new to Django.

The PUT: /operations/{operationId} route now takes a query param ?submit=true so we can set the status to pending on the submission page.

This also includes some style improvements to the header, review buttons as well as fixes to the user operator form brought over from my abandoned user operator pagination PR.

BCerki

The form is looking gorgeous! I did end up with a couple more questions. I wrote comments but if walkthrough is easier, happy to do that too.

A few general things (some could probably be tech debt or aren't a priority):

The status comes out with an underscore:
The breadcrumb is showing the form step as an ID:
Not sure if this is part of your ticket, but when there are errors about missing fields, it looks like there should be a message:
the step circles get distorted at different screen sizes:
It looks like pending operations should be readonly. Operation 3 from the mock data is readonly, but I could edit the one I made: https://www.loom.com/share/7c67dd95a1df4416a6e9bf9bb5c19af3
At mobile sizes, the input ends up on top of the label:

BCerki · 2023-11-21T19:28:58Z

bc_obps/registration/api/operation.py

-        # set the operation status to 'pending' on update
-        operation.status = "Pending"
+            # set the operation status to 'pending' on update
+        if submit == "true":


What does submit true/false mean? Like if it's the entire form submission vs one of the steps?

On the final step we set ?submit=true so that we can set the status to pending. We want the status to stay as not_registered unless we are submitting.

BCerki · 2023-11-21T19:31:41Z

bc_obps/registration/tests/test_endpoints.py

@@ -90,10 +90,10 @@ def test_post_new_operation(self, client):
        )
        post_response = client.post(self.endpoint, content_type=content_type_json, data=mock_operation.json())
        assert post_response.status_code == 200
-        assert post_response.json() == {"name": "Springfield Nuclear Power Plant"}
+        assert post_response.json() == {"name": "Springfield Nuclear Power Plant", "id": 2}


how do we know that id is 2?

Not a Django testing expert but that is the id that is being created and returned in this test. I assume a test above it is creating "id: 1"

I don't think it's safe to assume the correct value is what the test returns. If you just run that single test do you get id=1?

It's not 100% guaranteed that this id value will always be 2, because the order in which unit tests are run can vary. In this test case we don't care what the specific valud of id is, we just want to make sure that the id key-value pair is returning in the json, so I'd recommend replacing this with something like

assert post_response.json().get('name') == 'Springfield...' assert post_response.json().get('id') is not None

bc_obps/registration/tests/test_endpoints.py

BCerki · 2023-11-21T19:34:17Z

bc_obps/registration/tests/test_endpoints.py

@@ -173,7 +173,7 @@ def test_put_operation_not_verified_when_not_registered(self, client):
    def test_put_operation_update_status_invalid_data(self, client):
        def send_put_invalid_data():
            operation = baker.make(Operation)
-            assert operation.status == Operation.Statuses.PENDING
+            assert operation.status == Operation.Statuses.NOT_REGISTERED


Why do we need to check the default status so often?

I'm not sure, I updated the original tests which were checking the default status and kept it in.

BCerki · 2023-11-21T19:37:52Z

client/app/(authenticated)/dashboard/operations/page.tsx

@@ -8,7 +8,7 @@ export default function Page() {
  return (
    <>
      <h1>Operations List</h1>
-      <Link href="/dashboard/operations/create">
+      <Link href="/dashboard/operations/0/1">


I think the 0 is unclear because it looks like a real ID (especially because it shows in the breadcrumb:

)

It would be nice if we could create the operation and ID when someone clicks the Add Operation button. But I assume that doesn't work because there are mandatory fields for creating an operation that the user has to fill out?

I've discussed the breadcrumbs bugs a bit with Shon and this will be fixed. Though it's out of scope for this ticket.

BCerki · 2023-11-21T19:59:47Z

client/app/styles/rjsf/TitleOnlyFieldTemplate.tsx

 }: FieldTemplateProps) {
+  const options = uiSchema?.["ui:options"] || {};
+  const jsxTitle = options?.jsxTitle as any;


Yeah RJSF was picky with the type so I am using any, I don't think nits about types is super helpful in a front end web application tbh. Though happy to change it if you find something that works.

client/app/utils/jsonSchema/operations.ts

BCerki · 2023-11-21T22:24:33Z

client/app/components/form/MultiStepButtons.tsx

+  steps,
+  cancelUrl,
+  classNames,
+  submitEveryStep,


I'm having trouble with the submitEveryStep variable. Does it mean that all steps are mandatory? That a user has or hasn't filled all the steps?

submitEveryStep means the form submits between pages, if not it uses a button without type="submit" so it won't trigger RJSF validation.

If it's about validation, I'd suggest changing the variable name to something about validation or adding the explanation above in a comment. (Sidenote, why would we ever not want to validate?)

BCerki · 2023-11-21T22:25:41Z

client/app/components/form/MultiStepButtons.tsx

+            Back
+          </Button>
+        )}
+        {!isFinalStep && !submitEveryStep && (


Since I don't understand what submitEveryStep is, I'm having trouble following the logic here and below

client/app/components/form/OperationsForm.tsx

marcellmueller · 2023-11-22T00:03:25Z

The form is looking gorgeous! I did end up with a couple more questions. I wrote comments but if walkthrough is easier, happy to do that too.

A few general things (some could probably be tech debt or aren't a priority):

The status comes out with an underscore:

The breadcrumb is showing the form step as an ID:

Not sure if this is part of your ticket, but when there are errors about missing fields, it looks like there should be a message:

the step circles get distorted at different screen sizes:

It looks like pending operations should be readonly. Operation 3 from the mock data is readonly, but I could edit the one I made: https://www.loom.com/share/7c67dd95a1df4416a6e9bf9bb5c19af3

At mobile sizes, the input ends up on top of the label:
![image](https://user-imag> * The breadcrumb is showing the form step as an ID:

Not sure if this is part of your ticket, but when there are errors about missing fields, it looks like there should be a message:

the step circles get distorted at different screen sizes:

It looks like pending operations should be readonly. Operation 3 from the mock data is readonly, but I could edit the one I made: https://www.loom.com/share/7c67dd95a1df4416a6e9bf9bb5c19af3

The status handling has a lot of issues and is out of scope for this ticket. We discussed it and created an issue to fix it:
#291

Same with the breadcrumbs, out of scope and will be handled in the future.

BCerki

Where did we land on pending forms being readonly?

Nit on header spacing:

BCerki · 2023-11-22T21:34:39Z

client/app/components/form/MultiStepFormBase.tsx

+  const formSection = parseInt(params?.formSection as string) - 1;
+  const [formState, setFormState] = useState(formData || {});
+
+  const formSectionList = Object.keys(schema.properties as any);


Something along these lines would at least ensure it's an object: { [key: string]: any }

BCerki · 2023-11-22T21:52:45Z

client/app/components/form/MultiStepButtons.tsx

+  steps,
+  cancelUrl,
+  classNames,
+  submitEveryStep,


If it's about validation, I'd suggest changing the variable name to something about validation or adding the explanation above in a comment. (Sidenote, why would we ever not want to validate?)

BCerki · 2023-11-22T23:12:36Z

bc_obps/registration/tests/test_endpoints.py

@@ -90,10 +90,10 @@ def test_post_new_operation(self, client):
        )
        post_response = client.post(self.endpoint, content_type=content_type_json, data=mock_operation.json())
        assert post_response.status_code == 200
-        assert post_response.json() == {"name": "Springfield Nuclear Power Plant"}
+        assert post_response.json() == {"name": "Springfield Nuclear Power Plant", "id": 2}


I don't think it's safe to assume the correct value is what the test returns. If you just run that single test do you get id=1?

client/app/components/form/OperationsForm.tsx

andrea-williams · 2023-11-22T23:45:43Z

Added a suggestion for how the test case can be revised so that we don't run into issues with a hard-coded id value that might sometimes change. And also found a leftover typo that snuck in in a previous PR. That's all the feedback I have - otherwise I'm happy to get this PR merged in! I think the other nits Brianna mentioned will be addressed in other tickets we have in our backlog.

The Operations form is looking super slick @marcellmueller - fantastic work!

marcellmueller force-pushed the paginate-form branch 4 times, most recently from 384365c to 352bfe1 Compare November 17, 2023 14:51

marcellmueller marked this pull request as ready for review November 17, 2023 16:20

marcellmueller changed the title ~~paginate form~~ Paginate Operations Form Nov 17, 2023

marcellmueller force-pushed the paginate-form branch 2 times, most recently from 970efda to 1dcca7d Compare November 21, 2023 23:35

BCerki reviewed Nov 21, 2023

View reviewed changes

marcellmueller force-pushed the paginate-form branch 2 times, most recently from c0f1291 to b8b0beb Compare November 22, 2023 17:10

BCerki reviewed Nov 22, 2023

View reviewed changes

andrea-williams reviewed Nov 22, 2023

View reviewed changes

client/app/components/form/OperationsForm.tsx Show resolved Hide resolved

andrea-williams approved these changes Nov 22, 2023

View reviewed changes

marcellmueller force-pushed the paginate-form branch 3 times, most recently from 82195d9 to 022e5ba Compare November 23, 2023 00:14

marcellmueller added 11 commits November 22, 2023 16:21

refactor: add form base component and default rjsf theme

32ac6db

feat: set max page width and center header and footer

287f553

fix: adjust main div padding

247c0d0

fix: breadcrumbs hidden due to navidation updates

7c50c5d

chore: cleanup operations

f390087

refactor: destructure props in operations form

db6bd95

feat: create multi step button component

40de665

feat: add multistep header

5d78acb

fix: add multistep support to operations edit

51b1df5

refactor: create multistep form base for reusable form pagination

f2035b1

fix: pass form section titles to multistep header

29b398c

marcellmueller added 27 commits November 22, 2023 16:21

fix: multistep form header responsive mobile view

677c806

feat: style operations submission page

3fc8cda

chore: revert status string formatting

44303ab

fix: use 0 for create operator query param instead of string

2de2304

chore: run pre commit on all files

6d2b837

refactor: operation put api submit query param

fd3d738

test: update operation api tests

ec29bfe

fix: operation put api save

3d69ab2

test: add operations put api with submit test

bbaa6ec

chore: cleanup unused group schema

2f68838

fix: combobox enum error

6965bb5

feat: add widget error states

425f485

feat: add error state to inline field template

f0330c3

refactor: update multistep form base

9659194

feat: add custom titles to user operator form

6828090

feat: add tailwind global link colour override

410877e

fix: add header icon link and minor style improvements

13824ea

chore: update review button styles

636c158

fix: add file widget disabled colour

2a5f7be

chore: remove commented code in operations schema

2406d79

chore: remove commented code in theme.ts

70d5f4b

chore: remove unneeded eslint disable rule

d03a2f0

feat: remove use client

4c1b8a0

refactor: simplify form step math in operation form

a3e766d

feat: improve form responsive layout

0bd8830

fix: add min width to multstep header circles

a63d00d

test: update endpoint tests

123048c

marcellmueller force-pushed the paginate-form branch from 022e5ba to 123048c Compare November 23, 2023 00:21

marcellmueller merged commit dbd2df1 into develop Nov 23, 2023
15 of 17 checks passed

marcellmueller deleted the paginate-form branch November 23, 2023 00:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paginate Operations Form #269

Paginate Operations Form #269

marcellmueller commented Nov 10, 2023 •

edited

Loading

BCerki left a comment

BCerki Nov 21, 2023 •

edited

Loading

marcellmueller Nov 22, 2023

BCerki Nov 21, 2023

marcellmueller Nov 21, 2023

BCerki Nov 22, 2023

andrea-williams Nov 22, 2023

BCerki Nov 21, 2023

marcellmueller Nov 21, 2023

BCerki Nov 21, 2023

marcellmueller Nov 21, 2023

BCerki Nov 21, 2023

marcellmueller Nov 21, 2023

BCerki Nov 21, 2023

marcellmueller Nov 21, 2023

BCerki Nov 22, 2023

BCerki Nov 21, 2023

marcellmueller commented Nov 22, 2023

BCerki left a comment

BCerki Nov 22, 2023

BCerki Nov 22, 2023

BCerki Nov 22, 2023

andrea-williams commented Nov 22, 2023

Paginate Operations Form #269

Paginate Operations Form #269

Conversation

marcellmueller commented Nov 10, 2023 • edited Loading

BCerki left a comment

Choose a reason for hiding this comment

BCerki Nov 21, 2023 • 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

marcellmueller commented Nov 22, 2023

BCerki 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

andrea-williams commented Nov 22, 2023

marcellmueller commented Nov 10, 2023 •

edited

Loading

BCerki Nov 21, 2023 •

edited

Loading