-
Notifications
You must be signed in to change notification settings - Fork 42
MMT-4089: Fix inconsistencies with Collection Association #1418
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1418 +/- ##
==========================================
- Coverage 98.13% 98.10% -0.04%
==========================================
Files 422 422
Lines 6812 6796 -16
Branches 1436 1432 -4
==========================================
- Hits 6685 6667 -18
- Misses 126 128 +2
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cgokey
left a comment
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.
PR looks good to me. Added a couple of minor suggestions if you find the refresh just still isn't working right, as something you can try.
|
|
||
| // Handles deleting selected collection | ||
| // if no collections selected, returns an error notification | ||
| const handleDeleteAssociation = () => { |
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.
May want to surface an error if can't delete -- const handleDeleteAssociation = () => {
| const handleDeleteAssociation = () => { | |
| const handleDeleteAssociation = () => { | |
| setIsDeleting(true); | |
| deleteAssociationMutation({ | |
| variables: { | |
| conceptId, | |
| associatedConceptIds: collectionConceptIds | |
| } | |
| }).catch((error) => { | |
| // Handle error | |
| console.error('Failed to delete association:', error); | |
| addNotification({ | |
| message: 'Failed to delete association', | |
| variant: 'danger' | |
| }); | |
| }).finally(() => { | |
| setIsDeleting(false); | |
| }); | |
| };``` |
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.
This is done in deleteAssociationMutation's onError
| setCollectionConceptIds([]) | ||
|
|
||
| // Gives time for CMR to update data | ||
| setTimeout(() => { |
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.
One pattern I found for handling this that could be implemented is below:
| setTimeout(() => { | |
| setTimeout(() => { |
const pollForChanges = (attempts = 0) => {
if (attempts > 5) {
addNotification({
message: 'Unable to confirm update. Please refresh manually.',
variant: 'warning'
})
return
}
refetch().then((result) => {
if (/* check if data is updated */) {
// Data updated successfully
} else {
setTimeout(() => pollForChanges(attempts + 1), 1000)
}
})
}
onCompleted: () => {
setShowDeleteModal(false)
addNotification({
message: 'Collection Associations Deleted Successfully!',
variant: 'success'
})
setCollectionConceptIds([])
pollForChanges()
},
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.
In 10 attempts, the setTimeout appears to work each time. Though I do think we should consider polling for future CMR lag issues.
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.
Yeah I think we can probably come up with a generic component too
| }) | ||
| }) | ||
|
|
||
| describe('when rendering the CollectionAssociationForm', () => { |
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.
why do we remove the test here?
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.
We don't pass props to CollectionAssociationForm anymore.
| th:first-child, | ||
| td:first-child { | ||
| position: sticky; | ||
| z-index: 1000; |
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.
we should ensure this doesn't break other things as this is applied to alot of components if we are going to use this change alternatively we could bump up the Z index for a specific table
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 looked and didn't see any issues there
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 looked around but was unable to find a situation in which the top-left cell of a table needed to be at a z-index of 1000.
Overview
What is the feature?
What is the Solution?
1 & 2: Created an isDeleting state variable that toggles on and off for user feedback as well as for initiating a page refresh
3: ManageCollectionAssociation page now has a fetch policy of 'network-only'. This eliminates the need to mess around with caches as this data is changed.
4. Now using ControlledPagination
5. Similar with 3, using a 'network-only' fetch policy.
What areas of the application does this impact?
CollectionAssociationForm
ManageCollectionAssociation
Testing
Reproduction steps
Attachments
N/A
Checklist