modify care team confirmation context for alerting #129
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ps2
reviewed
Sep 8, 2023
ewollesen
force-pushed
the
eric/alerting-context
branch
from
cb2144f
to
d5687dc
Compare
ewollesen
force-pushed
the
eric/alerting-context
branch
2 times, most recently
from
ecacd3c
to
cafe1af
Compare
ewollesen
force-pushed
the
eric/alerting-context
branch
3 times, most recently
from
5af4513
to
735397c
Compare
toddkazakov
reviewed
Sep 27, 2023
| a.sendModelAsResWithStatus(res, statusErr, http.StatusBadRequest) | ||
| return | ||
| } | ||
| setPerms, err := a.gatekeeper.SetPermissions(inviteeID, invitorID, ctc.Permissions) |
There was a problem hiding this comment.
I think permissions must include the new "follow" permission and validate that the new permission is present in the request body if alertsConfig is not empty.
There was a problem hiding this comment.
That's fair. In theory, the alerts config that existed would be ignored without the permissions to match it, but there's also no sense in storing the alerts config if they don't have the permission. I'll make the change.
There was a problem hiding this comment.
Also worth noting, that the new alerting permission isn't added yet. That happens in #126. I'm making a note to address it there.
There was a problem hiding this comment.
I've now addressed this in #126.
ewollesen
force-pushed
the
eric/alerting-context
branch
from
735397c
to
606f8d8
Compare
Care team invitation/confirmation contexts are currently a
clients.Permissions object. To support alerting, the context is extended to
include a alerts.Config (defined in platform).
The custom JSON unmarshaler allows flexibility in parsing the following
received contexts:
1. The existing permissions only context
2. A hybrid context with old-style permissions plus an alertsConfig
3. A future context with a "Permissions" and an alertsConfig as sibling
properties
When the API is migrated to scenario #3 above, the custom marshaler can be
removed.
Bringing in platform as a dependency brought many vendor changes as well.
Part of BACK-2500
ewollesen
force-pushed
the
eric/alerting-context
branch
from
606f8d8
to
20eebcc
Compare
This struct property was changed in platform. This is the corresponding change. BACK-2500
toddkazakov
approved these changes
Oct 17, 2023
|
Closing this, as it's been consumed by #126 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Care team invitation/confirmation contexts are currently a clients.Permissions object. To support alerting, the context is extended to include a models.AlertsConfig.
The custom JSON unmarshaler allows flexibility in parsing the following received contexts:
When the API is migrated to scenario #3 above, the custom marshaler can be removed.
Pulling in platform as a dependency brought a lot of vendor changes with it.
Part of BACK-2500