Save shape updates #141

mstone121 · 2022-10-18T17:54:13Z

Overview

This PR adds functionality that saves a boundary when it is updated.

Updating is debouced with a three second interval. Each update is patched immediately and directly into the RTK query cache. After the three second interval, the latest update is sent to the server. If the server fails, changes are rolled back to the last saved state.

Closes #130

Notes

The map is re-rendering on update. I believe this is because a component above the map is using the boundary details query. When the shape is updated, this query is updated, which triggers a re-render. This should be addressed here if possible or in a later card.

Testing Instructions

Log in as [email protected]
Go to http://localhost:4545/draw/1
Make a few adjustments to the shape, wait 3 seconds, and reload the page
- Ensure updates were saved
Repeat with [email protected] and http://localhost:4545/draw/3
- Ensure updates were saved

Checklist

fixup! commits have been squashed
CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines
README.md updated if necessary to reflect the changes
CI passes after rebase

src/app/src/hooks.js

src/django/api/views/boundary.py

rajadain

This is generally looking good!

Let's take away the Validator's ability to change the shape.

Also, it would be nice to fix the map flashing. Would recommend that be given a shot in this PR, and only deferred if too complex.

src/django/api/views/boundary.py

rajadain · 2022-10-19T17:21:20Z

src/django/api/serializers/shape.py

+        if not ShapeSerializer.coordinates_are_closed(coordinates):
+            coordinates = ShapeSerializer.get_closed_coordinates(coordinates)
+
+        return Polygon(*coordinates)


This may be opposed to / need to be overridden when #127 is implemented.

rajadain · 2022-10-19T17:21:39Z

src/django/api/exceptions.py

+class BadRequestException(APIException):
+    status_code = 400
+    default_detail = 'There was a problem with your request.'
+    default_code = 'bad_request'


Nice job adding clear exceptions

src/django/api/views/boundary.py

mstone121 · 2022-10-19T19:24:37Z

src/app/src/components/DrawTools/DrawTools.js

@@ -45,6 +76,18 @@ export default function DrawTools({ details }) {
    );
 }

+function getDrawMode({ status, userRole }) {
+    if (userRole === ROLES.VALIDATOR && status === BOUNDARY_STATUS.IN_REVIEW) {


This used to be:

[BOUNDARY_STATUS.SUBMITTED, BOUNDARY_STATUS.IN_REVIEW].includes( details.status ) && user.role === ROLES.VALIDATOR

I'm thinking that the distinction between IN_REVIEW and SUBMITTED is the presence of a row in the Reviews database table. If a Validator clicks "Review" on a boudary, an entry should be added to the Reviews table before taking them to the /draw page.

Otherwise the Review creation logic would happen the first time a review is saved. This feels more complicated to me, however, it's something that could be handled entirely by the back-end.

This understanding is correct.

DRAFT = Row in Submission table, with submitted_at = NULL
SUBMITTED = That row has a value for submitted_at
IN_REVIEW = Row in Review table, with reviewed_at = NULL
NEEDS REVISIONS = That row has a value for reviewed_at
APPROVED = Row in Approval table, with a value for approved_at

I'm not sure how much we'll end up using IN_REVIEW in practice.

mstone121 · 2022-10-19T19:30:16Z

src/app/src/pages/Draw.js

    return (
        <Flex>
            <Sidebar />
            <Box flex={1} position='relative'>
                <Map>
-                    <Layers mode={mode} />


Is Layers going to need access to mode? If so, maybe boundary details should be loaded in a separate component above DrawTools and Layers, but below Map.

maybe boundary details should be loaded in a separate component above DrawTools and Layers, but below Map.

I like that arrangement.

Alternatively, if Layers uses the same RTKQuery hook to get the boundary details, will it not get a cached value? i.e. it should be fetched only once for the different components that request it, right?

Alternatively, if Layers uses the same RTKQuery hook to get the boundary details, will it not get a cached value? i.e. it should be fetched only once for the different components that request it, right?

Yep, but then we would also need to handle the various states of the request (isFetching, error, success) in that component as well. I like to have one parent component handling all this and passing the non-null data to the children.

Good point.

rajadain

Playing with this a bit, I think the reverting on fail to save is too harsh. For rural areas that may have flaky internet, a slight drop in connectivity can make the user lose their work:

2022-10-19.15.33.33.mp4

Failures here should be silent, and the shape on the map should remain unchanged.

When the contributor clicks "Save and back" or "Review and submit", we should also send the shape then. If there are internet connectivity issues then, we should let the user know, and stop them from navigating away from this page and losing their work. All this should be done as part of #132.

For this card, making the save failures silent in the background is enough.

Does that make sense?

rajadain · 2022-10-19T18:35:16Z

src/django/api/views/boundary.py

+        boundary.latest_submission.shape = serializer.validated_data
+        boundary.latest_submission.save()
+
+        return Response()


Consider returning an HTTP 204 NO CONTENT here.

rajadain · 2022-10-19T18:36:57Z

src/app/src/pages/Draw.js

@@ -19,7 +19,7 @@ const DRAW_MODES = {

 export default function Draw() {
    const user = useSelector(state => state.auth.user);
-    const { id } = useParams();
+    const id = useBoundaryId();


mstone121 · 2022-10-19T21:06:17Z

Does that make sense?

Yep. See changes here: 833ad59.

I'm not going to fixup this commit because it can serve as a refence for undoing manual cache updates.

rajadain

+1 tested this is looking good! Nice work guiding this in through all the different reviews!

src/app/src/hooks.js

There are two reasons to do this: The first is that it's less ambiguous than using `const { id } = useParams()`. This is especially important in deeply nested components where there are other elements with their own IDs. Second, if this logic were to change at a later point (perhaps it is moved into a Context), then it can be updated in one place.

This does not include the back-end changes which will follow in a later commit.

This could be useful in many components.

When the query was outside of the <Map> component, it would cause re-render of the map each time the query data was updated.

These changes are intentionally not fixed up into ff73200 because it can serve as a reference for how to revere manaual cache updates.

When #141 was merged, it updated the polygon to read from the RTK query cache. This broke the add polygon feature and is addressed here.

jacobtylerwalls reviewed Oct 18, 2022

View reviewed changes

src/app/src/hooks.js Outdated Show resolved Hide resolved

jacobtylerwalls reviewed Oct 18, 2022

View reviewed changes

src/django/api/views/boundary.py Outdated Show resolved Hide resolved

rajadain requested changes Oct 19, 2022

View reviewed changes

mstone121 force-pushed the ms/save-shape-changes branch from fea16bc to 9a11845 Compare October 19, 2022 19:15

mstone121 commented Oct 19, 2022

View reviewed changes

rajadain requested changes Oct 19, 2022

View reviewed changes

rajadain approved these changes Oct 20, 2022

View reviewed changes

src/app/src/hooks.js Show resolved Hide resolved

Matt Stone added 8 commits October 20, 2022 12:39

Update draw page to save polygon

e0bf015

This does not include the back-end changes which will follow in a later commit.

Add update boundary shape route

b5ad762

Update CHANGELOG.md

8d3772f

Add useEndpointToastError hook

c824157

This could be useful in many components.

Add LoadingModal

2b9520a

This could be useful in many components.

Move boundary details query inside <Map>

551e61d

When the query was outside of the <Map> component, it would cause re-render of the map each time the query data was updated.

Don't revert shape edits on save failure

5c56803

These changes are intentionally not fixed up into ff73200 because it can serve as a reference for how to revere manaual cache updates.

mstone121 force-pushed the ms/save-shape-changes branch from 889da3d to 5c56803 Compare October 20, 2022 17:44

mstone121 merged commit d79070c into develop Oct 20, 2022

mstone121 deleted the ms/save-shape-changes branch October 20, 2022 17:51

mstone121 pushed a commit that referenced this pull request Oct 24, 2022

Fix add polygon

d32750e

When #141 was merged, it updated the polygon to read from the RTK query cache. This broke the add polygon feature and is addressed here.

mstone121 pushed a commit that referenced this pull request Oct 25, 2022

Fix add polygon

56e581d

When #141 was merged, it updated the polygon to read from the RTK query cache. This broke the add polygon feature and is addressed here.

mstone121 pushed a commit that referenced this pull request Oct 25, 2022

Fix add polygon

eee5cac

When #141 was merged, it updated the polygon to read from the RTK query cache. This broke the add polygon feature and is addressed here.

mstone121 pushed a commit that referenced this pull request Oct 26, 2022

Fix add polygon

acda19a

When #141 was merged, it updated the polygon to read from the RTK query cache. This broke the add polygon feature and is addressed here.

mstone121 pushed a commit that referenced this pull request Oct 27, 2022

Fix add polygon

77c0f59

When #141 was merged, it updated the polygon to read from the RTK query cache. This broke the add polygon feature and is addressed here.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save shape updates #141

Save shape updates #141

mstone121 commented Oct 18, 2022 •

edited

Loading

rajadain left a comment

rajadain Oct 19, 2022

rajadain Oct 19, 2022

mstone121 Oct 19, 2022 •

edited

Loading

rajadain Oct 19, 2022

mstone121 Oct 19, 2022

rajadain Oct 19, 2022 •

edited

Loading

mstone121 Oct 19, 2022

rajadain Oct 19, 2022

rajadain left a comment

rajadain Oct 19, 2022

mstone121 Oct 19, 2022

rajadain Oct 19, 2022

mstone121 commented Oct 19, 2022

rajadain left a comment

Save shape updates #141

Save shape updates #141

Conversation

mstone121 commented Oct 18, 2022 • edited Loading

Overview

Notes

Testing Instructions

Checklist

rajadain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstone121 Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajadain Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajadain 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

mstone121 commented Oct 19, 2022

rajadain left a comment

Choose a reason for hiding this comment

mstone121 commented Oct 18, 2022 •

edited

Loading

mstone121 Oct 19, 2022 •

edited

Loading

rajadain Oct 19, 2022 •

edited

Loading