[ME] spatial extents field #941

Angi-Kinas · 2024-07-15T12:26:17Z

Description

This PR introduces the form field for the spatial extents.

The tests have no been written yet and the PR depends on #944.

For some reason the translation keys are not being applied.

Architectural changes

There are some changes in the record-form.component in the way we render this field. This is the first field that needs input from two sources of the field config (place keywords and spatial extents). That's why we made it specific here. If there will be more fields like this, we can think about making this more generic.

Screenshots

Quality Assurance Checklist

Commit history is devoid of any merge commits and readable to facilitate reviews
If new logic ⚙️ is introduced: unit tests were added
If new user stories 🤏 are introduced: E2E tests were added
If new UI components 🕹️ are introduced: corresponding stories in Storybook were created
If breaking changes 🪚 are introduced: add the breaking change label
If bugs 🐞 are fixed: add the backport <release branch> label
The documentation website 📚 has received the love it deserves

This work is sponsored by Organization ABC.

github-actions · 2024-07-15T12:27:31Z

Affected libs: api-metadata-converter, api-repository, feature-catalog, feature-record, feature-router, feature-search, feature-map, feature-dataviz, feature-editor, feature-auth, ui-search, common-domain, common-fixtures, ui-elements, feature-notifications, ui-catalog, util-shared, ui-widgets, ui-inputs, ui-layout, ui-map, ui-dataviz,
Affected apps: metadata-editor, metadata-converter, datahub, demo, webcomponents, map-viewer, search, datafeeder, data-platform,

🚀 Build and deploy storybook and demo on GitHub Pages
📦 Build and push affected docker images

github-actions · 2024-07-15T12:35:47Z

📷 Screenshots are here!

Angi-Kinas

Thanks @LHBruneton-C2C for your help on this!

Please feel free to review and leave comments.
I already left some comments to not forget what still needs to be done.
Another ting that I was unsure about, is where to place the components that have been created. Namely "GenericFormFieldKeywordsComponent" and "FormFieldMapContainerComponent" and about their naming.

Thanks in advance!

libs/common/domain/src/lib/model/record/metadata.model.ts

libs/api/repository/src/lib/gn4/platform/gn4-platform.mapper.ts

...rd-form/form-field/form-field-keywords-generic/form-field-keywords-generic.component.spec.ts

.../record-form/form-field/form-field-keywords-generic/form-field-keywords-generic.component.ts

Angi-Kinas · 2024-07-31T07:35:55Z

...s/record-form/form-field/form-field-map-container/form-field-map-container.component.spec.ts

+
+import { FormFieldMapContainerComponent } from './form-field-map-container.component';
+
+describe('FormFieldMapContainerComponent', () => {


To do: write tests for this component

Maybe this could be moved to feature/editor/src/lib too, to split things a bit more.

...record-form/form-field/form-field-spatial-extent/form-field-spatial-extent.component.spec.ts

libs/feature/editor/src/lib/components/record-form/record-form.component.ts

libs/feature/map/src/lib/utils/map-utils.service.ts

...onents/record-form/form-field/form-field-map-container/form-field-map-container.component.ts

LHBruneton-C2C · 2024-07-31T14:26:03Z

...onents/record-form/form-field/form-field-map-container/form-field-map-container.component.ts

+  @Input() keywordsWithSpatialExtents: {
+    [key: string]: {
+      placeKeyword?: Keyword
+      spatialExtents?: DatasetSpatialExtent
+    }
+  }[]


Not critical, but this component doesn't need to know anything about keywords, so it would be better if you only gave it an array of spatial extents as input. This could be done by reducing/flattening your keywordsWithSpatialExtents from the parent component before sending it to the child component.

...ents/record-form/form-field/form-field-spatial-extent/form-field-spatial-extent.component.ts

LHBruneton-C2C · 2024-07-31T14:43:57Z

...onents/record-form/form-field/form-field-map-container/form-field-map-container.component.ts

+  bboxCoordsToGeometry(bbox: [number, number, number, number]): Geometry {
+    const geometry = new Polygon([
+      [
+        [bbox[2], bbox[3]],


This is original, why not start with 0 - 1?

...ents/record-form/form-field/form-field-spatial-extent/form-field-spatial-extent.component.ts

LHBruneton-C2C

Nice work already! Not tested yet, will try tomorrow.

coveralls · 2024-08-07T14:46:28Z

coverage: 83.147% (+1.3%) from 81.886%
when pulling 2e2cd38 on ME-geographical-extent
into dd042ce on main.

jahow · 2024-08-22T21:40:43Z

@LHBruneton-C2C @Angi-Kinas I finally managed to rework this, I have rewritten the commit history to make it more readable and hopefully easier to review.

Things I added to the original work:

make it so that the form-field-spatial-extent component directly reads from and writes to the editor facade; this makes it not necessary to have an elaborate logic in the record-form component because this field works with two properties (keywords and extents) instead of one
fixed the missing translation bug (was due to an erroneous import of TranslateModule.forRoot() in ui-map which caused another translate store to be instanced)
reworked the form-field-spatial-extent to make it more straightforward: the link between extents and keywords is now done directly on the keyword (by storing it in a _linkedExtent property); this works because the user only interacts with keywords so then when one is added/removed we can add and remove extents accordingly
wrote missing tests
added a searchKeywordsByThesaurus in platform service; this is useful because keywords coming from the XML record do not have a bbox or key attached; this information has to be read from the thesaurus API
added a missing ol.css file

This is ready for review, I hope I didn't miss anything.

edit: Coveralls is down which explains the failure in the CI, but the rest works

…urus

this component will be used in the geographical coverage as well

Tests are moved to ng-mocks for convenience

This required adding TranslateModule to some tests in return

This fixes an issue where we can't match a deleted keywords with the array of shown keywords because the array would be recreated on each subscription The generic-keywords component is also not mutating its input array (this was causing a bug as well)

LHBruneton-C2C

Looking great! Thanks @Angi-Kinas and @jahow for this huge feature.
Tested draft and publishing, working as it should.

LHBruneton-C2C · 2024-08-26T14:57:33Z

libs/api/repository/src/lib/gn4/platform/gn4-platform.service.spec.ts

+        },
+      ])
+    })
+    it('returns an empty array of the thesaurus is unknown', async () => {


Suggested change

it('returns an empty array of the thesaurus is unknown', async () => {

it('returns an empty array if the thesaurus is unknown', async () => {

LHBruneton-C2C · 2024-08-26T15:08:46Z

...record-form/form-field/form-field-spatial-extent/form-field-spatial-extent.component.spec.ts

+      .overrideComponent(FormFieldSpatialExtentComponent, {
+        set: {
+          changeDetection: ChangeDetectionStrategy.Default,
+        },
+      })


I don't think this is necessary anymore with the MockBuilder.

LHBruneton-C2C · 2024-08-26T15:11:40Z

libs/feature/editor/src/lib/components/record-form/form-field/form-field.component.spec.ts

-        TranslateModule.forRoot(),
-        HttpClientTestingModule,
-      ],
-      schemas: [NO_ERRORS_SCHEMA],


Can't wait to see more of those disappear!

LHBruneton-C2C · 2024-08-26T15:12:24Z

libs/feature/editor/src/lib/models/editor-config.model.ts

@@ -17,6 +17,7 @@ export interface FormFieldConfig {
 }

 export interface EditorField {
+  id?: string


This is not needed anymore, since you used the facade to retrieve multiple fields.

LHBruneton-C2C · 2024-08-26T15:12:52Z

libs/feature/editor/src/test-setup.ts

+
+ngMocks.globalKeep(CommonModule, true)
+ngMocks.globalKeep(BrowserModule, true)
+ngMocks.globalKeep(RouterModule, true)


Not sure the Router should be kept in libs.

Angi-Kinas

Thank you very much @jahow for making this work! It was a really big and complicated feature.

I tested the feature a bit and I think we still need to filter out the place keywords from the "other" keywords that we display on the 3rd page of the editor.
Otherwise we get issues like this:

And when we delete the keyword "Africa" and go back to the first page, we see:

WDYT?

jahow · 2024-08-27T12:30:33Z

@Angi-Kinas yes you're right... could this be done in another PR? because I honestly don't feel like adding anything else to this one 😱

Angi-Kinas · 2024-08-27T12:35:33Z

@jahow yes you are right, I totally understand! Let's do it in another PR.

Angi-Kinas force-pushed the ME-geographical-extent branch 2 times, most recently from 0226510 to 6c9291c Compare July 31, 2024 07:26

Angi-Kinas commented Jul 31, 2024

View reviewed changes

Angi-Kinas changed the title ~~Draft: [ME] geographical extent~~ Draft: [ME] spatial extents Jul 31, 2024

Angi-Kinas changed the title ~~Draft: [ME] spatial extents~~ Draft: [ME] spatial extents field Jul 31, 2024