Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ME]: Add coverage field (toggle) #983

Merged
merged 8 commits into from
Sep 9, 2024
Merged

Conversation

Angi-Kinas
Copy link
Collaborator

@Angi-Kinas Angi-Kinas commented Sep 2, 2024

Description

This PR introduces an addition to the spatial extent field which is the coverage field. This field can be toggled between predefined values (as Keywords). For now we have "national" and "regional" as options.
When a value is selected, the corresponding Keyword will be added to the list of Keywords in the record.

Note

Update: No need to add the thesauri. Having the keywords in the fields config should be enough.
I'm not sure if this is testable without me changing the dump, or manually adding the two new thesauri in geonetwork.
The urls for them are:
https://inspire.ec.europa.eu/metadata-codelist/SpatialScope/regional
https://inspire.ec.europa.eu/metadata-codelist/SpatialScope/national

And for adding them in geonetwork you need to provide the url to the RDF/XML:
national: RDF/XML
regional: RDF/XML

Screenshots

image

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

Copy link
Contributor

github-actions bot commented Sep 2, 2024

Affected libs: feature-editor, ui-inputs, feature-dataviz, feature-record, feature-router, feature-search, feature-map, ui-elements, feature-notifications, feature-catalog, ui-catalog, ui-search, ui-layout,
Affected apps: metadata-editor, datafeeder, demo, metadata-converter, datahub, webcomponents, search, map-viewer,

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

Copy link
Contributor

github-actions bot commented Sep 2, 2024

📷 Screenshots are here!

@coveralls
Copy link

coveralls commented Sep 2, 2024

Coverage Status

coverage: 81.448% (-0.3%) from 81.781%
when pulling 29da5ca on ME-coverage-record-field
into 6f312e6 on main.

Copy link
Collaborator

@tkohr tkohr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Angi-Kinas ! I've left some suggestions. The logic seems to work fine. I was not able to save a metadata record yet, but this is likely an issue on my side.

url: new URL(
'http://localhost:8080/geonetwork/srv/api/registries/vocabularies/external.place.national.en'
),
type: 'place',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be of type theme?

})
describe('#onSpatialScopeChange', () => {
it('removes all existing spatial scope keywords and add the selected one', async () => {
const spatialScopes = [{ label: 'national' }, { label: 'regional' }]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't the labels expected to be uppercase?

@@ -98,12 +108,32 @@ export class FormFieldSpatialExtentComponent {
shareReplay(1)
)

switchToggleOptions$: Observable<SwitchToggleOption[]>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we could add a pipe on allKeywords$ here directly, so we wouldn't even need the ngOnInit with a subscription...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that makes more sense! Thanks for the suggestion. It was not easy for me to assign the switchToggleOptions$ initially based on the keywords, but now I have a solution that works. Maybe you can have another look?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, it looks good to me. If you move the switchToggleOptions$ behind the allKeywords$ you could even reuse the latter and only need one map like:

switchToggleOptions$: Observable<SwitchToggleOption[]> =
  this.allKeywords$.pipe(
    map((keywords) =>
...

constructor(
private platformService: PlatformServiceInterface,
private editorFacade: EditorFacade,
private translateService: TranslateService
) {}

ngOnInit(): void {
this.editorFacade.record$.subscribe((data) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we subscribe here we should also unsubscribe in the ngOnDestroy I think, but maybe we can even get rid of the ngOnInit entirely. WDYT?

@@ -10,7 +10,7 @@ import { MatButtonToggleModule } from '@angular/material/button-toggle'

export type SwitchToggleOption = {
label: string
value: string
value: any
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way we could still type this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it completely since it will be of different value for each component that will use the SwitchToggleComponent. And it is also not used anymore inside the component. WDYT?

Copy link
Collaborator

@tkohr tkohr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the adaption @Angi-Kinas. I was also able to retest it on my side and it works fine. I just see that there are some conflicts to be resolved before merging.

to use the label as identifier for the selected
value. Make value of type any to allow for
flexibility in the value type (e.g. Keyword)
as Keywords for the spatial scope / coverage
field which adds/removes a Keyword that can be
specified in the config file. For now it is a
toggle between "national" or "regional"
for spatial scope in form-field-spatial-extent
component
component as it can be of any type and is not
necessary.
chore(editor): Update spatial extent form field
to not use ngOnit and subscribe funtionalities
and assign options directly to the Observable,
adapt tests accordingly.
@Angi-Kinas Angi-Kinas merged commit 8b395cf into main Sep 9, 2024
14 checks passed
@Angi-Kinas Angi-Kinas deleted the ME-coverage-record-field branch September 9, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants