Arvados

This bug only appears when the user is logged in with admin privileges.

22473-favorites-crash @ 57e4b704d4f284db84b54f66e91003050d641044

developer-run-tests-services-workbench2: #1406

• ✅ All agreed upon points are implemented / addressed.
• - Anything not implemented (discovered or discussed during work) has a follow-up story.
• ✅ Code is tested and passing, both automated and manual, what manual testing was done is described
  • manually tested with my broken link
• - New or changed UX/UX and has gotten feedback from stakeholders.
• - Documentation has been updated.
• ✅ Behaves appropriately at the intended scale (describe intended scale).
• ✅ Considered backwards and forwards compatibility issues between client and server.
  • no change
• ✅ Follows our coding standards and GUI style guidelines.

• The problem was caused by 3 bugs:
1. A malformed link would reset the sidePanelTree to {} in the redux store
2. A malformed link would cause the sidePanelTree rendering cycle to fail, if though the tree is in the correct state in the redux store
3. The creation of a malformed link in the first place

• The first two bugs are now prevented from crashing the sidePanelTree
• We don't actually know what caused the last bug. It occurred after working on another ticket in which several favorites were created and deleted over and over again. There are now checks in place to ensure that all favorites/public favorites are formatted correctly before the create request is sent.

• In favorites-actions and public-favorites-actions, instead of creating an untyped link-like object and using hasCreateLinkProperties to check for fields, and then having the favoriteService.create() create the link object again and submit the API call, I think it would be cleaner to add the check inside the favoriteService.create()
  • You could create a typed link before it calls linkService.create like const link: Partial<LinkResource> = {... and then check for the fields from there.
  • This would allow hasCreateLinkProperties to accept Partial<LinkResource> for validation (or I suppose if you want to be fancy you could create a NewFavoriteLink type that is defined as Pick<LinkResource, ...> and use that - I think linkService.create should accept that since it wants Partial<LinkResource>).
  • This should also help the error handling, as it is now when the check to hasCreateLinkProperties fails it returns a rejected promise, which I think skips the progressIndicatorActions.STOP_WORKING and could cause the indicator to get stuck loading. If the favoritesService.create returns the rejected promise, then it should get caught down on L74 with any other errors.
• It would be nice if we could avoid casting to any in tree.ts mapTree, but I'm not sure how to achieve that - the behavior seems a little weird as mapTree is supposed to be converting Tree<T> to Tree<R>, but if the result is empty, then it's returning Tree<T>. If it wouldn't create a bug to let it return an empty tree (I assume that would happen if all the entries are invalid), then that seems like it would be the more correct thing to do.
• In favorite-service, checkPresenceInFavorites it looks like the additional filter seems redundant since the API query is already filtering for specific head_uuid and link_class=star, if the API still returns items with empty headUuid or incorrect linkClass then we should probably open a ticket for that and add a comment to that additional filter

22473-favorites-crash @ 45f9a1aff5a7824db2867086741f4c044cf50115

developer-run-tests-services-workbench2: #1411

• In favorites-actions and public-favorites-actions, instead of creating...

Done, with fanciness applied

• It would be nice if we could avoid casting to any in tree.ts mapTree...

I was able to get around this by returning createTree<R>(). The result (an empty tree) is the same in either case, but now the type is properly aligned.

• In favorite-service, checkPresenceInFavorites it looks like the additional filter seems redundant...

It was put in place because the API currently does return malformed links. #22541 was created to address that.

Great this lgtm!