[BACK-3478] Add email notifications work system processors.#896
[BACK-3478] Add email notifications work system processors.#896lostlevels wants to merge 10 commits intomasterfrom
Conversation
| @@ -0,0 +1,129 @@ | |||
| package claimaccount | |||
There was a problem hiding this comment.
In order to fit in with the other platform and work implementations, how about we move this to notifications/work/users/claim. And, for the others, notifications/work/users/connections/new and notifications/work/users/connections/issues.
There was a problem hiding this comment.
I went with a middle ground of
notifications/work/claims
*notifications/work/connections/requestsnotifications/work/connections/issues
Not a huge fan of packages where the parent packages are empty. IMO some package named connectionissues gives me pretty good idea of what it deals with, in the same way package stringutil does. When it gets to connections => subcategory is where things can start getting hairy if then sub-subcategories are added.
darinkrauss
left a comment
darinkrauss
left a comment
There was a problem hiding this comment.
A number of quick, minor changes. Also, I didn't highlight every instance, but please replace all fmt.Errorf with errors.Newf or errors.Wrapf, as appropriate. Otherwise, looks good.
|
|
||
| type Metadata struct { | ||
| ClinicID string `json:"clinicId,omitempty"` | ||
| UserId string `json:"userId,omitempty"` |
There was a problem hiding this comment.
Uppercase ID per Golang convention:
| UserId string `json:"userId,omitempty"` | |
| UserID string `json:"userId,omitempty"` |
|
Also, it looks like the build is failing. Try running |
darinkrauss
left a comment
darinkrauss
left a comment
There was a problem hiding this comment.
LGTM, just one question out of curiosity.
| script: | ||
| - export TIMING_CMD='time -p' | ||
| - make plugins-visibility-${PLUGINS_VISIBILITY} && make ci | ||
| - make "plugins-visibility-${PLUGINS_VISIBILITY}" && make ci |
There was a problem hiding this comment.
Was this to silence some linter warning or another?
…sers on specific events.
…itionalnotifications => notifications.
lostlevels
force-pushed
the
jimmy-device-email-notifications
branch
from
bd00c01 to
a0e93d5
Compare
ewollesen
left a comment
ewollesen
left a comment
There was a problem hiding this comment.
Aside from not seeing a matching TidepoolApi PR, all my comments are non-blocking.
Nice work.
| connrequests "github.com/tidepool-org/platform/notifications/work/connections/requests" | ||
| ) | ||
|
|
||
| func NotificationsRoutes() []dataService.Route { |
There was a problem hiding this comment.
I don't see the matching PR to TidepoolApi for these endpoints. Have I missed something?
There was a problem hiding this comment.
These are internal routes, do we usually add those?
|
|
||
| var data claims.Metadata | ||
| if err := request.DecodeRequestBody(req.Request, &data); err != nil { | ||
| request.MustNewResponder(res, req).Error(http.StatusBadRequest, err) |
There was a problem hiding this comment.
Nit
| request.MustNewResponder(res, req).Error(http.StatusBadRequest, err) | |
| responder.Error(http.StatusBadRequest, err) |
|
|
||
| var data connrequests.Metadata | ||
| if err := request.DecodeRequestBody(req.Request, &data); err != nil { | ||
| request.MustNewResponder(res, req).Error(http.StatusBadRequest, err) |
There was a problem hiding this comment.
| request.MustNewResponder(res, req).Error(http.StatusBadRequest, err) | |
| responder.Error(http.StatusBadRequest, err) |
|
|
||
| var data connissues.Metadata | ||
| if err := request.DecodeRequestBody(req.Request, &data); err != nil { | ||
| request.MustNewResponder(res, req).Error(http.StatusBadRequest, err) |
There was a problem hiding this comment.
| request.MustNewResponder(res, req).Error(http.StatusBadRequest, err) | |
| responder.Error(http.StatusBadRequest, err) |
| return errors.Wrap(err, "unable to create clinics client") | ||
| } | ||
| s.clinicsClient = &clnt | ||
| s.clinicsClient = clnt |
| // to group related claim account notifications together so they can all be | ||
| // deleted if the condition to send them is no longer active. For example, if a | ||
| // user has already claimed their account but there is a pending notification | ||
| // that hasn't been processed yetm the processor should delete all work items |
There was a problem hiding this comment.
| // that hasn't been processed yetm the processor should delete all work items | |
| // that hasn't been processed yet the processor should delete all work items |
| } | ||
| create := newWorkCreate(whenToSend, metadata) | ||
| if groupID := pointer.DefaultString(create.GroupID, ""); groupID != "" { | ||
| // Delete any other work items with the same group id because if a new reminder is added, any older ones would be too early since the last reminder of the same group id. |
There was a problem hiding this comment.
Hmm, so what happens if we add a reminder with a WhenToSend of 7 days... Then an hour later we add a reminder with a WhenToSend of 3 days.
In that case, the existing reminder isn't too early, as your comment states.
Maybe you meant to say something like "we maintain only a single reminder (per GroupID) at a time, and the last reminder added to the system wins"?
| return notifications.NewFailingResult(err, wrk) | ||
| } | ||
| if patient == nil { | ||
| return notifications.NewFailingResult(errors.Newf(`unable to find patient with userId "%v"`, data.UserID), wrk) |
There was a problem hiding this comment.
including the clinicId here too is probably useful, since a user could be a member of multiple clinics. Seeing this message, I would look for patient records with that user id, and I might find 1 or more and scratch my head as to why it couldn't be found (not realizing that it's supposed to be scoped to a specific clinic)
3 participants
No description provided.