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

fix(catalogue): resource details page not rendering when resource.peopleInvolved is empty #4347

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

@@ -506,7 +506,7 @@ if (route.params.catalogue) {
}

const contributors = computed(() =>
resource.value.peopleInvolved.sort((a, b) => {
resource.value.peopleInvolved?.sort((a, b) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

if peopleInvolved is falsy, the value of the contributors becomes undefined , whereas i would expect an empty list.

also is contributors now 'peopleInvolvedSoretedByRoleAndName' ?

Copy link
Member Author

@mswertz mswertz Oct 14, 2024

Choose a reason for hiding this comment

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

why would you expect empty list? I mean, the list should not be there if there are no values? See line 599 where the content block is conditional. That could instead be conditional on length > 0 is that better?

I will indeed rename the contributors to be more meaningful as you suggest

(and we should discuss with catalogue team if it should be 'contributors' or 'people involved'

Copy link
Contributor

@connoratrug connoratrug left a comment

Choose a reason for hiding this comment

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

looking at sort code i still see several potential null pointer issues (minimumOrderOfRolesB = b.role?.length , minimumOrderOfRolesB is undefined if roles is undefined, this later may result in number - undefined )( did a llm write this ?) but this pr fixes the relevant case in the catalogue
const contributors = computed(() => resource.value.peopleInvolved?.sort((a, b) => { const minimumOrderOfRolesA = a.role?.length ? Math.min(...a.role?.map((role) => role.order ?? Infinity)) : Infinity; const minimumOrderOfRolesB = b.role?.length ? Math.min(...b.role?.map((role) => role.order ?? Infinity)) : Infinity; if (minimumOrderOfRolesA !== minimumOrderOfRolesB) { return minimumOrderOfRolesA - minimumOrderOfRolesB; } else if (a.lastName !== b.lastName) { return a.lastName.localeCompare(b.lastName); } else { return a.firstName.localeCompare(b.firstName); } }) );

@connoratrug
Copy link
Contributor

now that its renamed its easer to follow , a see thats it to separate thing, a sortByRoleAndName function and getter from the query response ,

const peopleInvolved = resource?.peopleInvolved.length ? resource.peopleInvolved.sort(byRoleAndName) : [ ]

Copy link

sonarcloud bot commented Oct 14, 2024

@mswertz mswertz merged commit 005ff2d into master Oct 14, 2024
1 check was pending
@mswertz mswertz deleted the fix/testNetwork1-not-shown-because-of-sort-error branch October 14, 2024 21:05
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.

2 participants