Feature: DESENG-443/UI implementation #2413

NatSquared · 2024-03-11T15:50:43Z

Issue #: https://github.com/bcgov/met-public/issues/

Description of changes:

Task: Remove "default_values" from metadata taxa.
Replace with "preset values", metadata entries that are not assigned to an engagement.
Task: Update authorization documentation in the API blueprint. Update
metadata management to rely on normal authorization check functions.
Task: Clean up metadata management code and tests.
Task: Add endpoint for updating metadata by taxon in bulk
Feature: Add editor for metadata taxa (admin only). 🎟️DESENG-443
Feature: Add editor for metadata entries (available to anyone who can
edit an engagement). 🎟️DESENG-443

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).

* DESENG-452 : Applying pending migrations * Updating unit test * Updated changelog * Fixed lint issue

* DESENG-484: Adding max age for cors (#2377)

codecov-commenter · 2024-03-11T22:19:12Z

Codecov Report

Attention: Patch coverage is 94.68085% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 74.71%. Comparing base (8366029) to head (60cda11).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2413      +/-   ##
==========================================
+ Coverage   74.65%   74.71%   +0.06%     
==========================================
  Files         537      537              
  Lines       18555    18557       +2     
  Branches     1349     1347       -2     
==========================================
+ Hits        13852    13865      +13     
+ Misses       4496     4485      -11     
  Partials      207      207

Flag	Coverage Δ
metapi	`85.73% <94.68%> (+0.06%)`	⬆️

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

Files	Coverage Δ
met-api/src/met_api/auth.py	`100.00% <100.00%> (ø)`
met-api/src/met_api/resources/__init__.py	`100.00% <100.00%> (ø)`
...t-api/src/met_api/resources/engagement_metadata.py	`83.18% <100.00%> (+2.57%)`	⬆️
met-api/src/met_api/resources/metadata_taxon.py	`88.23% <100.00%> (+0.53%)`	⬆️
met-api/src/met_api/schemas/engagement_metadata.py	`82.00% <100.00%> (+2.00%)`	⬆️
met-web/src/apiManager/endpoints/index.ts	`100.00% <ø> (ø)`
met-web/src/components/common/Dragdrop/index.tsx	`100.00% <ø> (ø)`
...b/src/components/engagement/form/ActionContext.tsx	`58.46% <ø> (+0.30%)`	⬆️
...ormTabs/AdditionalDetails/AdditionalTabContent.tsx	`78.57% <ø> (-1.43%)`	⬇️
.../form/EngagementFormTabs/EngagementTabsContext.tsx	`70.44% <ø> (-0.19%)`	⬇️
... and 7 more

Baelx

Great work! It's been a really exciting feature to watch take shape.

Could you please just:

Remove some non-error console.log calls (maybe just one?)
Remove some commented-out code, I've left comments indicating where

Optionally, could you consider removing Promise.resolve, reject calls in async functions? I'm pretty sure they're not necessary. Also optionally, could you double-check that we need to add error messages to default context values? I feel like since we're setting those values in the provider right away, they shouldn't be necessary.

Baelx · 2024-03-12T22:26:39Z

met-api/src/met_api/resources/engagement_metadata.py

@@ -93,9 +101,27 @@ def post(engagement_id: int):
        except (ValueError, ValidationError) as err:
            return str(err), HTTPStatus.BAD_REQUEST

+    @staticmethod
+    @cross_origin(origins=allowedorigins())
+    @API.doc(security='apikey')


Oh so this was the method to add documentation to the API route? What does the security kwarg do here? Does it just add an additional key-value pair to the outputted json?

Here's a couple screenshots of http://localhost:5000/api/swagger.json showing the generated output. apikey and tenant are auth methods that can be referenced on endpoints at will:

"/engagement_metadata/taxa": { "patch": { "responses": { ... }, "summary": "Reorder the tenant's metadata taxa", "operationId": "patch_metadata_taxa", "parameters": [ ... ], "security": [ { "apikey": [] }, { "tenant": [] } ], "tags": [ ... ] },

They show up in the SecurityDefinitions as header parameters, like so:

"securityDefinitions": { "apikey": { "type": "apiKey", "in": "header", "name": "Authorization" }, "tenant": { "type": "apiKey", "in": "header", "name": "tenant-id" } },

This is achieved on line ~40 when we use the imported security definitions and apply them to our Namespace

Baelx · 2024-03-12T22:33:04Z

met-api/src/met_api/resources/engagement_metadata.py

+    @cross_origin(origins=allowedorigins())
+    @API.doc(security='apikey')
+    @API.expect(metadata_bulk_update_model, validate=True)
+    @API.marshal_list_with(metadata_return_model)


I don't think we're really doing response marshalling anywhere else, right?

Baelx · 2024-03-12T22:33:46Z

met-api/src/met_api/resources/engagement_metadata.py


 @cors_preflight('GET,PUT,DELETE')
-@API.route('/<metadata_id>')  # /api/engagements/{engagement.id}/metadata/{metadata.id}


Thanks for keeping this comment up to date!

Baelx · 2024-03-12T22:42:55Z

met-api/src/met_api/resources/metadata_taxon.py

@@ -131,7 +132,7 @@ def post(tenant: Tenant):

    @staticmethod
    @cross_origin(origins=allowedorigins())
-    @API.expect({'taxon_ids': fields.List(fields.Integer(required=True))})
+    @API.expect(taxon_ids_model)


So many new flast_restx methods to learn! Would you mind explaining what this is doing? Can this handle full-on request validation?

This is basically a shortcut to sidestep a lot of validation I'd be doing manually. This particular endpoint is expecting a list of tenant IDs, and it puts them in the order that they appear in the list.
the @API.expect part means that the request will be automatically rejected with proper error messages if:

the mimetype is not application/json

the json is invalid

the json does not have a key taxon_ids containing a list of integers (in this case)

I'm not sure exactly what counts as "full-on" validation - this mostly handles data structure, not values. For example, if a valid integer that didn't correspond to a taxon ID was passed in, this decorator would not catch it.

Thanks for breaking that down! I was just curious what level of validation this decorator could provide and how it was being used alongside the Marshmallow or JSON validation we're doing elsewhere

Baelx · 2024-03-12T22:51:43Z

met-api/src/met_api/services/engagement_metadata_service.py

@@ -145,6 +138,37 @@ def update(metadata_id: int, value: str) -> dict:
        metadata.value = value
        return dict(EngagementMetadataSchema().dump(metadata, many=False))

+    @staticmethod
+    @transactional()
+    def update_by_taxon(engagement_id: int, taxon_id: int, values: List[str]) -> List[dict]:


Great type annotations and documentation!

Baelx · 2024-03-13T00:17:22Z

met-web/src/components/metadataManagement/TaxonEditForm.tsx

+                        </Collapse>
+                    </Grid>
+                    <Grid item container alignItems="center" justifyContent="flex-start">
+                        <Button variant="outlined" onClick={() => setSelectedTaxonId(-1)}>


On the subject of accessibility, if you were a non-sighted person using a keyboard, would it be clear what "cancel" on its own means? Is there enough context on this button for someone not relying on sight to understand what it does? If not, we should add an aria-label with more context.

I'm going to speak to Steve about what's expected of the backend of the app in terms of web accessibility. No need to rush to update/change this or anything

Baelx · 2024-03-13T00:19:39Z

met-web/src/components/metadataManagement/TaxonEditor.tsx

+            }}
+        >
+            <Box sx={{ padding: '2.1em', width: '100%' }}>
+                {/* <MetHeader2>Metadata Management</MetHeader2> */}


Baelx · 2024-03-13T00:21:00Z

met-web/src/components/metadataManagement/TaxonEditor.tsx

+                                                />
+                                            );
+                                        })}
+                                    {isLoading && [...Array(9)].map(() => <TaxonCardSkeleton />)}


Love this way of quickly spinning up nine components

Baelx · 2024-03-13T00:23:52Z

met-web/src/components/metadataManagement/types.ts

+    trigger: (name?: string | string[] | undefined) => Promise<boolean>;
+    errors: Partial<
+        FieldErrorsImpl<{
+            [x: string]: any;


This should handled with a type or linter exception if needed

Baelx · 2024-03-13T00:25:33Z

met-web/src/services/engagementMetadataService/index.ts

    const url = replaceUrl(Endpoints.EngagementMetadata.GET_BY_ENG, 'engagement_id', String(engagementId));
    if (!engagementId || isNaN(Number(engagementId))) {
-        return Promise.reject('Invalid Engagement Id ' + engagementId);
+        return Promise.reject(new Error('Invalid Engagement Id ' + engagementId));


Just an FYI, when the async keyword is appended to a function, it'll automatically return a promise so explicit calls to Promise.resolve, Promise.reject shouldn't be necessary. To have the promise resolve, simply return something. To reject, just throw a new Error

…eview - Alex)

sonarqubecloud · 2024-03-13T21:03:27Z

Quality Gate passed

Issues
0 New issues
6 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

NatSquared and others added 21 commits February 9, 2024 12:27

Engagement Metadata Management - API changes

7be03fc

DESENG-452 : Applying pending migrations (#2378)

f8bc363

* DESENG-452 : Applying pending migrations * Updating unit test * Updated changelog * Fixed lint issue

DESENG-484: Adding max age for cors (#2379)

f764cb1

* DESENG-484: Adding max age for cors (#2377)

Engagement Metadata Management - API changes

6c946ba

edit migration order to merge with migration changes

21ba1e2

Backend changes for DESENG-443

4ff5307

Frontend changes for deseng-443

9871f57

Remove metadata references from frontend unit tests

202ade8

Update Changelog

95b095c

Update unit tests to handle metadata as an array

85f7233

Clean up unused imports from frontend files

3fd43ea

Add ticket links to changelog

942a009

Merge branch 'main' into feature/deseng-443/ui-implementation

c3c5041

Adapt metadata to changes from main

b933803

Format and save date properly

09462ed

Save changes stably without re-renders

4e2bfcc

Merge changes from main

abb99d1

Python: fix lint errors

a2e6162

Apply suggestions from JS linter; update migration file

331a540

More lint errors fixed

43127af

Hopefully the final lint errors fixed

b649ae7

NatSquared marked this pull request as ready for review March 11, 2024 19:09

String conversion hotfix

9c303f7

NatSquared added 4 commits March 11, 2024 15:54

Flake8 validation - attempt 1

bcbdddb

Flake8 validation - attempt 2

d9bc58b

Patch jest tests

60cda11

Patch jest tests again (sorry)

31bcd8b

Baelx requested changes Mar 13, 2024

View reviewed changes

Fix lint warnings, remove commented code, enhance accessibility (PR r…

3fb3746

…eview - Alex)

NatSquared added 3 commits March 13, 2024 13:06

Merge branch 'main' into feature/deseng-443/ui-implementation... again

3dfb1da

Fix sonarcloud linting; aria accessibility tweaks

bf6abb1

Ensure proper sequence of flask migration files

1db888f

Baelx approved these changes Mar 13, 2024

View reviewed changes

NatSquared merged commit 0796978 into main Mar 13, 2024
15 checks passed

NatSquared deleted the feature/deseng-443/ui-implementation branch March 27, 2024 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: DESENG-443/UI implementation #2413

Feature: DESENG-443/UI implementation #2413

NatSquared commented Mar 11, 2024

codecov-commenter commented Mar 11, 2024 •

edited

Loading

Baelx left a comment

Baelx Mar 12, 2024

NatSquared Mar 13, 2024 •

edited

Loading

Baelx Mar 12, 2024

NatSquared Mar 13, 2024

Baelx Mar 12, 2024

Baelx Mar 12, 2024

NatSquared Mar 13, 2024 •

edited

Loading

Baelx Mar 13, 2024

Baelx Mar 12, 2024

Baelx Mar 13, 2024

Baelx Mar 13, 2024

Baelx Mar 13, 2024

Baelx Mar 13, 2024

Baelx Mar 13, 2024

sonarqubecloud bot commented Mar 13, 2024


		@cors_preflight('GET,PUT,DELETE')
		@API.route('/<metadata_id>') # /api/engagements/{engagement.id}/metadata/{metadata.id}

Feature: DESENG-443/UI implementation #2413

Feature: DESENG-443/UI implementation #2413

Conversation

NatSquared commented Mar 11, 2024

codecov-commenter commented Mar 11, 2024 • edited Loading

Codecov Report

Baelx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NatSquared Mar 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

NatSquared Mar 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

sonarqubecloud bot commented Mar 13, 2024

Quality Gate passed

codecov-commenter commented Mar 11, 2024 •

edited

Loading

NatSquared Mar 13, 2024 •

edited

Loading

NatSquared Mar 13, 2024 •

edited

Loading