-
Notifications
You must be signed in to change notification settings - Fork 190
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
feat(compass-saved-aggregations-queries): update namespace for saved aggregations/queries (COMPASS-7665) #5462
Conversation
8831c62
to
0ce0217
Compare
}); | ||
} else if (selectedItem.type === 'query') { | ||
await queryStorage?.updateAttributes(id, { _ns: newNamespace }); | ||
} |
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 wonder if we shouldn't dispatch some 'itemUpdated' action here. probably not necessary, it just feels weird that there would be no trace of this change happening in redux.
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.
Hmmm, I'd say that as in this case there is no effect on the actual reducer and we already dispatch an action that you can trace back to this handler, it's okay not to do this. Maybe if there was even more logic involved and the state of the update was part of the reducer, this would make more sense to do. Good point though!
packages/compass-saved-aggregations-queries/src/components/open-item-modal.tsx
Outdated
Show resolved
Hide resolved
packages/compass-saved-aggregations-queries/src/components/open-item-modal.tsx
Outdated
Show resolved
Hide resolved
className={checkbox} | ||
checked={updateItemNamespace} | ||
onChange={(event) => { | ||
dispatch(updateItemNamespaceChecked(event.target.checked)); |
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.
Super nit, feel free to ignore: as we are already using connect
and mapDispatch
here, it might be a bit cleaner to stick with the pattern for now
}); | ||
} else if (selectedItem.type === 'query') { | ||
await queryStorage?.updateAttributes(id, { _ns: newNamespace }); | ||
} |
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.
Hmmm, I'd say that as in this case there is no effect on the actual reducer and we already dispatch an action that you can trace back to this handler, it's okay not to do this. Maybe if there was even more logic involved and the state of the update was part of the reducer, this would make more sense to do. Good point though!
…aggregations/queries (COMPASS-7665) (#5462)
Description
If a collection is renamed, any queries or aggregations associated with that namespace are not updated. Users have the ability to open a query or aggregation by selecting the namespace, but they have no way of permanently re-associating a namespace with their queries/aggregations.
This PR adds the ability to permanently update the name space associated with an aggregation or query when they use they open it from the "select namespace" modal. This scenario may become more common after the "rename collection" feature is released.
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes