feat: adding organisations thumbnails to records #511

f-necas · 2023-06-22T14:50:12Z

Organisations thumbnails are now visible in feed and datasets tabs.

If the organization strategy is set to metadata (default behaviour), matching is done on the organization name set in the contact property of the record.

Maybe the IMAGE_URL constant could be common in organisations-from-groups.service.ts and organisations-from-metadata.service.ts.

@fgravin @jahow @tkohr Tell me if this solution would be suitable.
Thanks :)

github-actions · 2023-06-22T14:52:01Z

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

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

fgravin

Looks good to me except the mandatory changes mentioned in the comments.

Overall, I would have some comments about the implementation from @jahow

It's disturbing that we add duplicated contacts within the contacts & resourcesContact properties of the MetadataRecord objects. I wonder if we shouldn't clearly separate this concern with an organization property in the record.
mapContact() could take an optional 3rd argument as the organization, for the group strategy, the group is mapped to the organisation first.
The records should not have the catalog logo by default
I think we should try to maintain a cache because the mapping between contacts & organisation + the hydration would be repeated a lot for nothing. Not in this PR though.

Overall the organisation domain object should raise above contacts/groups, we should just treat with organisations; contacts (for metadata) or groups (for group) should be mapped to an organisation and be forgotten.

fgravin · 2023-06-22T15:10:41Z

libs/feature/catalog/src/lib/organisations/service/organisations-from-metadata.service.ts

+  ): MetadataContact {
+    const logoUrl = getAsUrl(`${organisation.logoUrl}`)
+    return {
+      name: organisation.name[lang3],


contacts or not multilingual yet
multilingual index fields ends with Object eg organizationObject

fgravin · 2023-06-22T15:37:26Z

libs/feature/catalog/src/lib/organisations/service/organisations-from-metadata.service.ts

+    ]).pipe(
+      map(([record, organisations]) => {
+        const org = organisations.filter(
+          (o) => o.name === record.contact.organisation


resourceContact[0]

fgravin · 2023-06-22T15:40:10Z

libs/feature/catalog/src/lib/organisations/service/organisations-from-metadata.service.ts

+        )[0]
+        if (org) {
+          const lang3 = LANG_2_TO_3_MAPPER[this.translateService.currentLang]
+          record.contact = this.mapContactFromOrganisation(org, lang3)


avoid object mutation

jahow

Just looked through quickly but I think the approach is good since it's only touching the orgs-from-metadata implementation 🙂

jahow · 2023-06-22T16:27:03Z

It's disturbing that we add duplicated contacts within the contacts & resourcesContact properties of the MetadataRecord objects. I wonder if we shouldn't clearly separate this concern with an organization property in the record.

Yes clearly the record format is awkward on that part. I think we should address that in the pivot format and not overthink it with the current MetadataRecord format though.

mapContact() could take an optional 3rd argument as the organization, for the group strategy, the group is mapped to the organisation first.

I feel like simply adding a 3rd optional argument would be a work around for something missing elsewhere, but not really sure about this.

The records should not have the catalog logo by default

I think that's the point of this PR, display the thumbnail if any and display the contact logo otherwise. Or am I missing something? Maybe we shouldn't overwrite any existing logo on the record?

I think we should try to maintain a cache because the mapping between contacts & organisation + the hydration would be repeated a lot for nothing. Not in this PR though.

Why not but in my understanding the contacts in the raw metadata can have many different forms and the method used to match with the groups use several fields. We can't just maintain a map between an org email and a group for example; the mapping could use a combination of the org fields (name, email...). But I also think that this concern would be "premature optimization": the hydration happens maybe at most twenty times in a row when showing the records list, and I don't think this is going to be noticeable in any way.

fgravin · 2023-06-22T18:07:33Z

I think that's the point of this PR, display the thumbnail if any and display the contact logo otherwise. Or am I missing something? Maybe we shouldn't overwrite any existing logo on the record?

ATM it uses the metadata.logo which is the catalog logo, and not the organization logo, so yes it could make sense as the third choice for the thumbnail.
Actually i haven't checked where the metadata logo comes from, probably from the source, which means that geo2france should have logos instead of blank images (because they use harvester sources) ... anyway

fgravin · 2023-06-23T07:54:44Z

I feel like simply adding a 3rd optional argument would be a work around for something missing elsewhere, but not really sure about this.

Thinking about this and the issue is more about the confusion between contact & orgs.
The mapper and atomic-operations are just responsible for mapping the index to our model, at least it should be. Once we are hydrating this model from other sources (other than the index, eg. groups/orgs), then we overextend the responsibility of the mapper IMO.
And the question raised again: should we extend metadata informations from other sources (eg contacts from orgs/groups), or shall we add new properties in the record (eg organisations).

f-necas · 2023-06-23T08:09:38Z

And the question raised again: should we extend metadata informations from other sources (eg contacts from orgs/groups), or shall we add new properties in the record (eg organisations).

For me, I think it would be clearer if we edit the MetadataRecord interface with something like :

export interface MetadataRecord {
  id: string
  ...
  contact?: MetadataContact
  resourceContacts?: MetadataContact[]
  organisation?: Organisation
  ...
}

but this doesn't match XML format ...

What if we create an ExtendedMetadateRecord interface ?

export interface ExtendedMetadataRecord {
  record: MetadataRecord
  organisation?: Organisation
}

remove untranslated organisation name fields, improving addOrganisationToRecordFromSource without forkJoin, alter mapLogo to not use the record logo for a contact

fgravin · 2023-07-11T09:42:24Z

It's not clear in the US, but to me the rules are

use record thumbnail if present
if not, use organization logo if present
if not, use placeholder

f-necas · 2023-07-11T10:37:37Z

It's not clear in the US, but to me the rules are

use record thumbnail if present

if not, use organization logo if present

if not, use placeholder

It's conflicting what @jahow told me : Keeping catalog logo if org logo is not present for the feed.
For the feed, organisation logo is set with org.logo or record.logo (which is catalog logo in the index).
For the search, we take record thumbnail or organisation logo ( so it takes catalog logo if none present ).

@fgravin Can you confirm that we won't use record logo (catalog's logo) for organisation logo ? It will change actual display of logos in the feed preview

fgravin · 2023-07-11T11:51:36Z

@fgravin Can you confirm that we won't use record logo (catalog's logo) for organisation logo ? It will change actual display of logos in the feed preview

Ok, to me this is the kind of things to be discussed with Geo2France/MEL, but you can go as @jahow told you for now.
Keep using catalog logo as a fallback.
Thanks

fgravin

Thanks for adding the missing behavior of hydrating the records with the organisation details.
Works well, but it lacks of tests. You should test the hydration.

I still have a weird behavior (which is expected), i have an org with a logo in the organization tab, when i click on it, the metadata in the result list has the catalog logo instead of the org one (because the organisation is the second resourceContact). Actually, having several resourceContact breaks a bit the approch we have, because the record does not have only one organisation, anyway.

Feel free to merge after adding a test.
Thanks, great work

check if record collect the right logo/thumbnail

f-necas requested review from fgravin, tkohr and jahow June 22, 2023 14:50

f-necas force-pushed the org-thumbnail branch from 6c26a85 to 5255d25 Compare June 22, 2023 14:52

fgravin requested changes Jun 22, 2023

View reviewed changes

jahow reviewed Jun 22, 2023

View reviewed changes

f-necas force-pushed the org-thumbnail branch from 1a1ad20 to 8d832c0 Compare June 28, 2023 09:12

f-necas requested review from fgravin and jahow June 29, 2023 12:00

f-necas force-pushed the org-thumbnail branch from 8d832c0 to 2ed6a2a Compare July 3, 2023 08:49

fgravin added enhancement Proposal for a new feature minor feature labels Jul 5, 2023

f-necas force-pushed the org-thumbnail branch 2 times, most recently from 6ad11ad to 844c970 Compare July 6, 2023 09:21

f-necas added 2 commits July 6, 2023 11:32

feat: adding organisations thumbnails to records

a39b383

fix: translations, improve method, alter mapLogo

70e0784

remove untranslated organisation name fields, improving addOrganisationToRecordFromSource without forkJoin, alter mapLogo to not use the record logo for a contact

f-necas force-pushed the org-thumbnail branch from 844c970 to 4e0fbbb Compare July 6, 2023 09:32

f-necas marked this pull request as ready for review July 6, 2023 09:32

f-necas force-pushed the org-thumbnail branch from 4e0fbbb to 70e0784 Compare July 11, 2023 11:21

feat: getting record logo as fallback if it is set in contact

0b6a439

fgravin self-assigned this Jul 13, 2023

fgravin approved these changes Jul 13, 2023

View reviewed changes

fgravin assigned f-necas Jul 13, 2023

test: add tests for organisations with matching group

8c212f6

check if record collect the right logo/thumbnail

f-necas merged commit 0c09970 into main Jul 17, 2023
8 checks passed

f-necas deleted the org-thumbnail branch July 17, 2023 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding organisations thumbnails to records #511

feat: adding organisations thumbnails to records #511

f-necas commented Jun 22, 2023

github-actions bot commented Jun 22, 2023 •

edited

Loading

fgravin left a comment

fgravin Jun 22, 2023

fgravin Jun 22, 2023

fgravin Jun 22, 2023

jahow left a comment

jahow commented Jun 22, 2023

fgravin commented Jun 22, 2023

fgravin commented Jun 23, 2023

f-necas commented Jun 23, 2023 •

edited

Loading

fgravin commented Jul 11, 2023

f-necas commented Jul 11, 2023

fgravin commented Jul 11, 2023

fgravin left a comment

feat: adding organisations thumbnails to records #511

feat: adding organisations thumbnails to records #511

Conversation

f-necas commented Jun 22, 2023

github-actions bot commented Jun 22, 2023 • edited Loading

fgravin left a comment

Choose a reason for hiding this comment

fgravin Jun 22, 2023

Choose a reason for hiding this comment

fgravin Jun 22, 2023

Choose a reason for hiding this comment

fgravin Jun 22, 2023

Choose a reason for hiding this comment

jahow left a comment

Choose a reason for hiding this comment

jahow commented Jun 22, 2023

fgravin commented Jun 22, 2023

fgravin commented Jun 23, 2023

f-necas commented Jun 23, 2023 • edited Loading

fgravin commented Jul 11, 2023

f-necas commented Jul 11, 2023

fgravin commented Jul 11, 2023

fgravin left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 22, 2023 •

edited

Loading

f-necas commented Jun 23, 2023 •

edited

Loading