fix: Map and Label breaking on back action #3825

RODO94 · 2024-10-18T11:58:07Z

The initial bug centred on navigating back after completing the Map and Label component, and finding the active tab not selected, then when adding a new point, the tabs would break. My solution was to change how the ActiveIndex was assigned and handled, and when the previous data from the passport was added to features. From this, I found a couple other bugs on how we handle labels when copying data.

Should all be fixed now with testing around navigating back. There is no test coverage right now for navigating back, adding new data, continuing forward, then navigating back again.

I would be interested to know if I should be rendering a different component within the tests so I can test clicking "Continue" then clicking "Back" - at the moment, I just recreate rendering a component with the previouslySubmittedData. Would be great to have feedback on whether this would be overkill.

github-actions · 2024-10-18T12:20:47Z

Removed vultr server and associated DNS entries

jessicamcinchak

Looks like you're on the right track for this bug! Know this is still draft, but want to flag what's seeming like a possible new side-effect bug of your changes:

Here's steps to recreate:

Navigate forwards to Map&Label & draw two features
Click "reset all" button the map -> features and tabs should both clear
Draw two features again
"Continue" forwards this time
Navigate "back" to Map&Label
Confirm I can add another feature to map and tab is added (works now!)
Click "reset all" button on the map -> map is cleared but the tabs are not (new bug?!)

Can you recreate too?

First guess would be to double check that any changes you're making to the map context methods stay in sync with their form method counterpart !

move test files to test folder tidy test expects fix: remove unused imports

RODO94 · 2024-10-21T09:53:54Z

editor.planx.uk/src/@planx/components/MapAndLabel/Public/Context.tsx

  );

  const [minError, setMinError] = useState<boolean>(false);
  const [maxError, setMaxError] = useState<boolean>(false);
+  const [loadPreviousValues, setLoadPreviousValues] = useState<boolean>(


Went round the blocks with this trying to find an elegant solution, this felt like the best option to ensure previous values only loaded once

I don't really understand the addition of these local state variables - why can't we simply ensure addInitialFeaturesToMap is only called if when previouslySubmittedData is present? What's a scenario where we're trying to load initial values more than once?

If we do indeed need this, can we rename it to something like loadPreviousValuesOnMap because it's not actually handling the form tab data (formik.initialValues is, right?)

From what I could understand, previouslySubmittedData is always present once you navigate backwards, so when you add a new point the code will run again and it will add the new point to features then hit the if() statement then reassign features to be previouslySubmittedData.data...

I initially tried to place it in a useEffect() to only run the if statement on initial mount then whenever previouslySubmittedData changed, but I couldn't get it to work so easily, and it felt like a larger change than the state variable.

Have I missed a simpler solution?

Thanks for explanation - happy to keep this approach then, it seems to be achieving what we want !

RODO94 · 2024-10-21T09:55:06Z

editor.planx.uk/src/@planx/components/MapAndLabel/Public/Context.tsx

-    formik.setFieldValue(`schemaData[${destinationIndex}]`, sourceFeature);
+    formik.setFieldValue(`schemaData[${destinationIndex}]`, {
+      ...sourceFeature,
+      label: `${destinationIndex + 1}`,


The label was being assigned based on the object you were copying from rather than the object we were copying to

Good catch but I don't think this is quite working in all scenarios correctly yet!

For example:

I add two trees on the map

I fill out tree two tab

On tab 1, I "copy from" tree 2

I "continue" & my labels are correct

I come "back" and "remove tree" from tab 1 - this shifts the map & tab labels for previous "tree 2" to 1 now

I "continue" again and my passport data has a single feature with label: 2 when it should be label: 1 to reflect map/tab changes after a point was removed !

Thanks so much for catching this, I'll add some adjustments

Testing added for this and hope I have solved it. I couldn't find another bug through manual testing...

editor.planx.uk/src/@planx/components/MapAndLabel/test/mocks/Trees.ts

RODO94 · 2024-10-21T09:56:54Z

editor.planx.uk/src/@planx/components/MapAndLabel/test/public.test.tsx

This file was becoming quite long, so it felt right to move the back navigation tests to separate file, and relocate these files to the test folder on the Map and Label component

jessicamcinchak

On the right track - pizza is working more smoothly, but a few questions/comments while reading through the code!

jessicamcinchak · 2024-10-21T14:52:18Z

editor.planx.uk/src/@planx/components/MapAndLabel/Public/Context.tsx

  );

  const [minError, setMinError] = useState<boolean>(false);
  const [maxError, setMaxError] = useState<boolean>(false);
+  const [loadPreviousValues, setLoadPreviousValues] = useState<boolean>(


I don't really understand the addition of these local state variables - why can't we simply ensure addInitialFeaturesToMap is only called if when previouslySubmittedData is present? What's a scenario where we're trying to load initial values more than once?

If we do indeed need this, can we rename it to something like loadPreviousValuesOnMap because it's not actually handling the form tab data (formik.initialValues is, right?)

jessicamcinchak · 2024-10-21T14:53:14Z

editor.planx.uk/src/@planx/components/MapAndLabel/Public/Context.tsx

+    if (loadPreviousValues) {
+      setFeatures(initialFeatures);
+      setLoadPreviousValues(false);
+    }


Again, see above comment - not following this change in particular !

editor.planx.uk/src/@planx/components/MapAndLabel/test/mocks/Trees.ts

jessicamcinchak · 2024-10-21T14:56:46Z

editor.planx.uk/src/@planx/components/MapAndLabel/test/mocks/mockPayload.tsx

+export const previouslySubmittedDoubleData: PreviousData = {
+  auto: false,
+  ...previousMockDoubleFeatureData,
+};


Per your question in the PR description, mocking previouslySubmittedData is how I'd expect to see "back" navigation tested at the component level 👍 This is consistent with other @planx/components tests

jessicamcinchak · 2024-10-21T15:05:15Z

editor.planx.uk/src/@planx/components/MapAndLabel/Public/Context.tsx

-    formik.setFieldValue(`schemaData[${destinationIndex}]`, sourceFeature);
+    formik.setFieldValue(`schemaData[${destinationIndex}]`, {
+      ...sourceFeature,
+      label: `${destinationIndex + 1}`,


Good catch but I don't think this is quite working in all scenarios correctly yet!

For example:

I add two trees on the map

I fill out tree two tab

On tab 1, I "copy from" tree 2

I "continue" & my labels are correct

I come "back" and "remove tree" from tab 1 - this shifts the map & tab labels for previous "tree 2" to 1 now

I "continue" again and my passport data has a single feature with label: 2 when it should be label: 1 to reflect map/tab changes after a point was removed !

jessicamcinchak · 2024-10-21T15:06:57Z

editor.planx.uk/src/@planx/components/MapAndLabel/test/public.backNavigation.test.tsx

+    const secondTabPanel = getByTestId("vertical-tabpanel-1");
+    expect(secondTabPanel).toBeVisible();
+
+    addMultipleFeatures([point1, point2, point3]);


nit: aren't point1 and point2 already on the map? Am we actually mocking adding "multiple" features here or just one? I understand the mock data needs to reflect the full feature collection, but I wonder if there's a way we can make this more clear in helper function naming, a code comment etc?

This is correct, it should just be adding one feature at a time. I'll adjust and switch the functions

Found that this was correct and just adding a single point deletes the others from the map, so I need them added like this. I've added a comment above each line explaining this need

editor.planx.uk/src/@planx/components/MapAndLabel/test/public.backNavigation.test.tsx

refine naming of test mocks remove test.only

jessicamcinchak

Working as expected for me now - thanks for iterating on this one !

fix activeIndex bug and previous data fn

3ab268c

jessicamcinchak reviewed Oct 18, 2024

View reviewed changes

fix: label bug when copy - continue - back

407e57e

move test files to test folder tidy test expects fix: remove unused imports

RODO94 force-pushed the rory/map-back-bug branch from a237a8d to 407e57e Compare October 21, 2024 09:46

RODO94 requested a review from a team October 21, 2024 09:48

RODO94 marked this pull request as ready for review October 21, 2024 09:48

RODO94 commented Oct 21, 2024

View reviewed changes

editor.planx.uk/src/@planx/components/MapAndLabel/test/mocks/Trees.ts Outdated Show resolved Hide resolved

RODO94 commented Oct 21, 2024

View reviewed changes

jessicamcinchak reviewed Oct 21, 2024

View reviewed changes

refactor: new test for label bug and naming refinement

960c1eb

refine naming of test mocks remove test.only

RODO94 force-pushed the rory/map-back-bug branch from 9af3f9f to 960c1eb Compare October 22, 2024 12:51

refine state variable naming

c9e05e2

RODO94 requested a review from jessicamcinchak October 22, 2024 14:36

jessicamcinchak approved these changes Oct 25, 2024

View reviewed changes

RODO94 merged commit 8ef61d3 into main Oct 25, 2024
12 checks passed

RODO94 deleted the rory/map-back-bug branch October 25, 2024 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Map and Label breaking on back action #3825

fix: Map and Label breaking on back action #3825

RODO94 commented Oct 18, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024 •

edited

Loading

jessicamcinchak left a comment •

edited

Loading

RODO94 Oct 21, 2024

jessicamcinchak Oct 21, 2024

RODO94 Oct 21, 2024

jessicamcinchak Oct 25, 2024 •

edited

Loading

RODO94 Oct 21, 2024

jessicamcinchak Oct 21, 2024

RODO94 Oct 21, 2024

RODO94 Oct 22, 2024

RODO94 Oct 21, 2024

jessicamcinchak left a comment

jessicamcinchak Oct 21, 2024

jessicamcinchak Oct 21, 2024

jessicamcinchak Oct 21, 2024

jessicamcinchak Oct 21, 2024

jessicamcinchak Oct 21, 2024 •

edited

Loading

RODO94 Oct 21, 2024

RODO94 Oct 22, 2024

jessicamcinchak left a comment

fix: Map and Label breaking on back action #3825

fix: Map and Label breaking on back action #3825

Conversation

RODO94 commented Oct 18, 2024 • edited Loading

github-actions bot commented Oct 18, 2024 • edited Loading

jessicamcinchak 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

jessicamcinchak Oct 25, 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

jessicamcinchak 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

jessicamcinchak Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicamcinchak left a comment

Choose a reason for hiding this comment

RODO94 commented Oct 18, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024 •

edited

Loading

jessicamcinchak left a comment •

edited

Loading

jessicamcinchak Oct 25, 2024 •

edited

Loading

jessicamcinchak Oct 21, 2024 •

edited

Loading