Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[DC-1205]Store TDR owned Sam Group info in database #1788
[DC-1205]Store TDR owned Sam Group info in database #1788
Changes from 14 commits
bddeff0
e305ccd
1ec7888
86ebc67
ee583f6
49018b3
fd9022c
cc710b1
247c994
d6cba79
22ee075
cbc9818
62aa63a
d45f934
0dab3b2
f781f42
4fd729f
5258850
01a6429
878afc8
3dffe87
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
Do you want to exclude
DELETED
here too? That's what otherget()
methods do.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.
Hm the getById for SnapshotRequestDao doesn't do this. Maybe I should update both methods
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 actually we do want to return deleted requests for the getByCreatedId call. Because when a source snapshot is deleted, all of the snapshot access requests on that snapshot are deleted. However, created snapshots from those requests will still exist as will the sam groups associated with them, stored on the snapshot access request. So, when one of those created snapshots is deleted, we still want to be able to find the request that created it (even if it has been deleted) so that we can delete the sam group stored on it. Also, for the getById endpoint I think we might also want to return deleted requests as well for auditing purposes, but not as sure on this one.
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.
Yes, that's a case where we can't exclude deleted as the caller may want to see a deleted record.
OK, that seems reasonable to me. Since this is an internal API (vs a web API) it's OK to return deleted elements. I wonder if the filtering of deleted items for the API call should be done outside the DAO layer and in the service or API layer. 🤔