[MRXN23-581] inventory panel: features in progress #1649

andresgnlez · 2024-02-12T11:50:16Z

Substitute this line for a meaningful title for your changes

Overview

This PR fetches and handles features at project level that has not being processed yet by the API but we need to display its status in the table.

"Running" features (features being processed) cannot be selected, edited, deleted or visible on map.
"Failed" features cannot be edited nor visible on map. They can be selected and deleted though.

Designs

Link to the related design prototypes (if applicable)

Testing instructions

Please explain how to test the PR: ID of a dataset, steps to reach the feature, etc.

Feature relevant tickets

https://vizzuality.atlassian.net/browse/MRXN23-581

Checklist before submitting

Meaningful commits and code rebased on develop.
If this PR adds feature that should be tested for regressions when
deploying to staging/production, please add brief testing instructions
to the deploy checklist (docs/deployment-checklist.md)
Update CHANGELOG file

vercel · 2024-02-12T11:50:22Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
marxan	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 6:08pm

hotzevzl

thanks for the API part @KevSanchez - it must have been a semi-nightmare to adapt everything to the async-ish way of life 😬

I've added only some minor inline notes about details. I haven't been able to use this in action yet - I'm planning to do this tomorrow, if no other emergencies call me in other projects 😬

hotzevzl · 2024-02-27T16:01:28Z

api/apps/api/src/migrations/api/1708099064649-AddFeatureShapefileApiEvents.ts

+      UPDATE features SET creation_status = 'done'
+      WHERE creation_status = 'created';


I think we should avoid this - the up side of this migration is destructive, and we cannot properly undo this (until now we've been using a mix of done and created, and we have no way to know which of the two was set before up side of the migration run).

I don't think this (being able to restore the original confirmation_status is very important, though (mostly, I think we did not make any real use of this flag until now), so I suggest to maybe just put in a small comment explaining that we cannot reliably roll back the relevant part of the up side of the migration.

hotzevzl · 2024-02-27T16:14:51Z

api/apps/api/src/modules/geo-feature-tags/geo-feature-tags.service.ts

+    /**
+     * @debt ? apparently joins in update queries are currently not supported by typeorm yet, so qe require raw SQL here
+     */
+    await this.geoFeatureTagsRepo.query(
+      `UPDATE project_feature_tags pft
+        SET tag = $1
+        FROM features f
+        WHERE f.id = pft.feature_id and pft.project_id = $2 and pft.tag = $3 and f.creation_status = $4`,
+      [dto.updatedTagName, projectId, dto.tagName, JobStatus.created],
    );


I am not sure about this - yes, we don't want to allow editing of features that are not fully created yet... but on the other hand, this should be (I think) an update of a tag at project level, so maybe we could just let it go through?

If feature A (being created) has tag T set at the start of the upload, and while it's still being created a user decides that tag T should be renamed to S, maybe we can make the assumption that T must be renamed to S across all features, so that when feature A gets to created status, it will have S as tag?

I think this could be open to opinions! I'd suggest to simplify though :/

Hmmm, to add a bit more of confusion, only the feature metadata itself is persisted at the start of the upload, the tag creation is part of the upload transaction, so what would happen if an user uploads a feature with an already existing tag, then in the process it changes that tag at project level? The other tag hasn't been committed and will be persisted as is.
Maybe the tag does get persisted at the start of upload as well and we treat indifferently like you suggest? (I think this doesn't apply to CSV, I recall not having the option to input a tag there)

As we discussed to keep matters simple, and considering that edge case is rather unlikely and not really worth the effort for the complexity it entails, I'm having the PUT/DELETE endpoints for project tags not consider the creationStatus except the GET project tags endpoint.

hotzevzl · 2024-02-27T16:22:32Z

api/apps/api/src/modules/geo-features/geo-features.service.ts

+        // Of all three possible creation status, (fail, running, created) we must also include failure and running (if
+        // indicated in the request) even if they don't intersect with the bounding box (because in those non created
+        // states there is no geospatial data in the geoDB to intersect with)
+        const creationStatusSelection = [
+          JobStatus.failure,
+          ...(info?.params?.includeInProgress ? [JobStatus.running] : []),
+        ];


even though a request may ask for includeInProgress, wouldn't the intention behind this to retrieve features that are not ready to be used, whether this is because they are "in progress" or because their creation failed?

Or put it another way, should the default (give me all the features) not be "all features that are ready to be used"?

hotzevzl · 2024-02-27T16:51:30Z

api/apps/api/src/modules/geo-features/geo-features.service.ts

+      setTimeout(async () => {
+        await this.createFeaturesForShapefile(projectId, data, featureDatas);
+      });
+    } catch (e) {}


🤔 why this empty catch block? IIRC if we throw an error here (could be throw new Error(e)), this should be caught by the NestJS global exception filter which should handle it as a 500 in this case (again IIRC). We should at least log an error with details, and re-throw the exception (or not catch it at all and let the global exception filter step in)

Ah yes, this is because in order to make this run asynchronously, by wrapping the execution in a setTimeout function, an error thrown inside it completely slips by the global exception filter, as the function gets executed from the global context.
I found this while tweaking the CSV tests that checked for failures, whenever an error was thrown it would terminate the test prematurely.

https://nodejs.org/dist/latest-v7.x/docs/api/errors.html#errors_node_js_style_callbacks
https://stackoverflow.com/questions/41431605/how-to-handle-errors-from-settimeout-in-javascript

hotzevzl · 2024-02-27T16:55:17Z

api/apps/api/src/modules/geo-features/import/features-amounts-upload.service.ts

+          projectId,
+          userId,
+        });
+      } catch (e) {}


same as for the earlier empty catch block

Same reason as previous one, as well

api/apps/api/test/projects/project-feature-tags/project-tags.e2e-spec.ts

hotzevzl · 2024-02-27T17:21:45Z

api/apps/api/test/upload-feature/upload-feature.fixtures.ts

@@ -264,6 +268,70 @@ export const getFixtures = async () => {
        .send();
    },
    // ASSERT
+    ThenWaitForApiEvent: (topic: string, kind: API_EVENT_KINDS) => {


just a tiny naming note: for this and for all the following fixture functions that wait for specific API events to be fired, I think they are used as something inbetween acting and asserting, basically holding off until we can do the core assertions... (you've placed the functions within the assert section here, but often within the act section within tests).
No need to do anything, but my suggestion would be to treat them as assertions throughout, with names such as ThenA<kind>EventIsSubmitted, but well, yes, it's hair-splitting on naming, so up to you 😄

…hronous

andresgnlez added the Frontend Everything related frontend label Feb 12, 2024

andresgnlez self-assigned this Feb 12, 2024

vercel bot deployed to Preview February 12, 2024 11:50 View deployment

andresgnlez changed the title ~~[MRXN23-581]: inventory: features in progress~~ [MRXN23-581] inventory panel: features in progress Feb 12, 2024

inventory: features in progress

8805e10

KevSanchez force-pushed the MRXN23-581-api-uploading-features-may branch from c4f971c to 8805e10 Compare February 19, 2024 17:57

vercel bot deployed to Preview February 19, 2024 18:00 View deployment

vercel bot deployed to Preview February 21, 2024 12:59 View deployment

KevSanchez force-pushed the MRXN23-581-api-uploading-features-may branch from 88831b4 to 014df41 Compare February 21, 2024 18:55

vercel bot deployed to Preview February 21, 2024 18:57 View deployment

KevSanchez force-pushed the MRXN23-581-api-uploading-features-may branch from 014df41 to 6633d5e Compare February 22, 2024 09:49

vercel bot deployed to Preview February 22, 2024 09:52 View deployment

KevSanchez force-pushed the MRXN23-581-api-uploading-features-may branch from 6633d5e to 40bc71c Compare February 22, 2024 14:28

vercel bot deployed to Preview February 22, 2024 14:30 View deployment

KevSanchez marked this pull request as ready for review February 22, 2024 17:12

KevSanchez requested a review from hotzevzl February 22, 2024 17:12

hotzevzl reviewed Feb 27, 2024

View reviewed changes

KevSanchez force-pushed the MRXN23-581-api-uploading-features-may branch from 40bc71c to 5578410 Compare March 1, 2024 17:36

vercel bot deployed to Preview March 1, 2024 17:38 View deployment

fix(Features): Makes the upload of features (shapefile and csv) async…

197ba50

…hronous

KevSanchez force-pushed the MRXN23-581-api-uploading-features-may branch from 5578410 to 197ba50 Compare March 1, 2024 18:06

vercel bot deployed to Preview March 1, 2024 18:08 View deployment

KevSanchez merged commit 186cadb into develop Mar 1, 2024
61 checks passed

KevSanchez deleted the MRXN23-581-api-uploading-features-may branch March 1, 2024 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRXN23-581] inventory panel: features in progress #1649

[MRXN23-581] inventory panel: features in progress #1649

andresgnlez commented Feb 12, 2024 •

edited

Loading

vercel bot commented Feb 12, 2024 •

edited

Loading

hotzevzl left a comment

hotzevzl Feb 27, 2024

hotzevzl Feb 27, 2024

KevSanchez Feb 29, 2024

KevSanchez Mar 1, 2024

hotzevzl Feb 27, 2024

hotzevzl Feb 27, 2024

KevSanchez Feb 29, 2024

hotzevzl Feb 27, 2024

KevSanchez Feb 29, 2024

hotzevzl Feb 27, 2024

		UPDATE features SET creation_status = 'done'
		WHERE creation_status = 'created';

[MRXN23-581] inventory panel: features in progress #1649

[MRXN23-581] inventory panel: features in progress #1649

Conversation

andresgnlez commented Feb 12, 2024 • edited Loading

Substitute this line for a meaningful title for your changes

Overview

Designs

Testing instructions

Feature relevant tickets

Checklist before submitting

vercel bot commented Feb 12, 2024 • edited Loading

hotzevzl 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

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

andresgnlez commented Feb 12, 2024 •

edited

Loading

vercel bot commented Feb 12, 2024 •

edited

Loading