FCM client refactor #397
FCM client refactor #397
Conversation
Codecov Report
@@ Coverage Diff @@
## master #397 +/- ##
==========================================
+ Coverage 55.17% 55.18% +0.01%
==========================================
Files 115 115
Lines 7956 7949 -7
==========================================
- Hits 4390 4387 -3
+ Misses 2868 2864 -4
Partials 698 698
Continue to review full report at Codecov.
|
| @@ -133,7 +133,7 @@ func handleSubmit(ctx context.Context, s *web.Server, r *http.Request) (interfac | |||
| if err != nil { | |||
| return nil, http.StatusInternalServerError, errors.Wrapf(err, "error starting transaction for session write") | |||
| } | |||
| sessions, err := models.WriteSessions(ctx, tx, s.RP, oa, []flows.Session{fs}, []flows.Sprint{sprint}, nil) | |||
| sessions, err := models.WriteSessions(ctx, tx, s.RP, nil, oa, []flows.Session{fs}, []flows.Sprint{sprint}, nil) | |||
There was a problem hiding this comment.
surveyor sessions shouldn't be sending android messages
| @@ -237,7 +238,7 @@ func HandleAndCommitEvents(ctx context.Context, db QueryerWithTx, rp *redis.Pool | |||
| } | |||
|
|
|||
| // ApplyModifiers modifies contacts by applying modifiers and handling the resultant events | |||
| func ApplyModifiers(ctx context.Context, db QueryerWithTx, rp *redis.Pool, oa *OrgAssets, modifiersByContact map[*flows.Contact][]flows.Modifier) (map[*flows.Contact][]flows.Event, error) { | |||
There was a problem hiding this comment.
modifiers don't send messages so don't need redis (for talking to courier) or the FCM client (for talking to Android)
|
I started to have my doubts realising how many places need to pass it around but I think I still prefer the functional approach and if we're worried about function argument bloat we could possibly pack things like this into a context structure. |
|
Does FCMClient really have any state that is interesting? Maybe our problem is having that be an object in the first place? |
|
Looking at it it is basically just an http.Client, so it really isn't maintaining much state over just the config. I don't know, it isn't the end of the world for sure, but just really feels wrong to have the Hook stuff even know about FCMClient, especially given how rarely it is used. |
|
What's the difference between hooks knowing about redis.Pool and knowing about fcm.Client ? |
|
To me it is that redis/sql are more generically used by other hooks. They also actually have state beyond just configs (pools for the connections). |
|
Ok so what do you want to do now that this is merged? I don't feel very strongly in favor of this so if you feel it's really wrong then I can revert it. |
|
I approved it so I can live with it. :) |
|
I've reverted it for now so can ponder on it a bit more |
Moves to
Mailroominstance alongside the elastic client, redis pool etc instead of being a global.