-
Notifications
You must be signed in to change notification settings - Fork 33
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
fix: org thumbnail loading time and contacts overrides #538
Conversation
Affected libs:
|
contactFromOrg, // FIXME: this should go into an organization field | ||
...metadataRecord.resourceContacts, | ||
] | ||
if (org && !record.resourceContacts?.[0].logoUrl && org.logoUrl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should take the email if not present.
const logoUrl = getAsUrl(`${org.logoUrl}`) | ||
metadataRecord.resourceContacts[0].logoUrl = logoUrl // FIXME: this should go into an organization field | ||
metadataRecord.resourceContacts[0].email ??= org.email | ||
metadataRecord.contact.email ??= org.email |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if
statement is not correct for the email set.
I would suggest to keep your mapContactFromOrganisation
and change the name to hydrateContactWithOrg
, and do all the stuff in there.
I would also not touch the record.contact
, you are guessing record.contact
and record.resourceContacts[0]
are the same but it might not be the case.
Note that the group
strategy seems quite different as it adds new contacts within the metadata, which is a bit weird IMO, anyway, the group has the priority in that case so it's ok.
website: metadataRecord.resourceContacts[0].website, | ||
} as MetadataContact | ||
|
||
metadataRecord.resourceContacts = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't add a new contact like for the group
strategy, that was my point in the previous message.
BTW if you do that the name of the function is not meaningful.
The group
strategy add a new contact from the group of the record (which could be different from the recource contact), while the metadata
strategy just hydrates the first existing resource contact.
organisation: 'Ifremer', | ||
}, | ||
{ | ||
email: '[email protected]', | ||
logoUrl: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is wrong.
You should write what you expect first, and code the correct behavior, instead of coding and adapting the test so it can pass 😉
Here what we said
- logo of the contact if it exists
- logo of the organisation
- logo of the record
In your test, it set the logo of the record while it should use the logo of the organisation instead (because resourceContact[0].logoUrl = ''
)
Probably, during the mapping, the resource contact logoUrl is set to the record logo if null in the contact, before the mapping with the organisation, which breaks the priority.
fix: use record logo in last resort
02c24f0
to
4fe24da
Compare
@@ -54,12 +54,25 @@ export const mapLogo = (source: SourceWithUnknownProps) => { | |||
return logo ? getAsUrl(`/geonetwork${logo}`) : null | |||
} | |||
|
|||
export const hydrateWithRecordLogo = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just call it hydrateContactsWithRecordLogo
Do the tests cover this function ? Probably
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed.
Yes this function is covered in both org-from-metadata and org from-groups classes.
const logoUrl = | ||
firstResourceContact.logoUrl || | ||
(organisation.logoUrl ? getAsUrl(`${organisation.logoUrl}`) : null) | ||
const mappedOrg = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we said that the organisation props would only hydrate missing contact props, didn't we ? 🤔
Would mean that the priority is the contact email, the contact logo, the contact name etc...
Organisations observables now use shareReplay to avoid multiple call from toRecord method.
Contact is not overrided anymore to prevent wrong display in metadata view.
Only first resourceContact logoUrl is overrided.