Skip to content

Conversation

@LPoin
Copy link

@LPoin LPoin commented Mar 19, 2025

Description

This PR fix the filter of links section disply in record "reuse".
The application should display the links if they exist in the record, reuse or not, as it worked before v2.4
Linked to issue-1169

Architectural changes

Filter modified in mdview-facade.ts to get links in reuse record.
If needed, the filter can be remove but this implies a change in the typing of the elements returned, and therefore deeper changes in the code.

Screenshots

Before

image

After

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

How to test

Open a interactiveMap or map record, see the links displayed in the right section


@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

allLinks$ = this.metadata$.pipe(
map((record) =>
record.kind === 'dataset' && 'onlineResources' in record
(record.kind === 'dataset' || record.kind === 'reuse') && 'onlineResources' in record
Copy link
Member

Choose a reason for hiding this comment

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

I think you can just remove the condition on the record kind; this was there for historical reasons but now all records have an onlineResources property

Copy link
Member

Choose a reason for hiding this comment

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

Also could you please add a test for this? thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Yes as I said, allLinks$ return a Observable<DatasetOnlineResource[]> but if I remove record.kind === 'dataset', it return a Observable<DatasetOnlineResource[] | ServiceOnlineResource[]>.
So everywhere it used it create a typing error.
I could fix that but It's likely to change the code to an extent I'm not familiar with.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right... Then ok your change makes sense. 🙂 Please let us know if you need help for the test

Copy link
Author

Choose a reason for hiding this comment

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

Thanks I'll see it and let you know !

Copy link
Author

Choose a reason for hiding this comment

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

Hello @jahow I get back to you to find out what type of test to add.
In mdview.facade.spec.ts existing tests seems to cover all requirements and passes correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants