-
Notifications
You must be signed in to change notification settings - Fork 69
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(core): Fixing optout flow #292
Conversation
jeyrschabu
commented
Nov 6, 2019
- moved cloud provider opt out actions to handler
- update handler interface optout function
- fix abstraction for optout flow
@aravindmd per our discussion this PR fixes issues we are seeing with the optout flow. |
- moved cloud provider opt out actions to handler - update handler interface optout function - fix abstraction for optout flow
17a0681
to
d782735
Compare
currentStatus = status, | ||
statuses = setOf(optOutState.statuses + status).flatten().toMutableList() | ||
).apply { | ||
resourceStateRepository.upsert(this) |
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.
what about resourceRepository.upsert()
? will this take care of resource upsert ?
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 would we do that while we are ensuring this resource is opted out & umarked?
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.
ok so we don't need to update the resource state , i ask this because the previous optout flow had this call?
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.
That was definitely a bug. resourceRepository.upsert would end up marking the resource. ResourceStateRepository.upsert does update the status to opted out which is what we need
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 like this refactor , makes it much easier to follow.