🔁 Add task to resend messages #420
Conversation
Codecov Report
@@ Coverage Diff @@
## main #420 +/- ##
==========================================
+ Coverage 54.47% 54.67% +0.20%
==========================================
Files 115 116 +1
Lines 8110 8164 +54
==========================================
+ Hits 4418 4464 +46
+ Misses 2990 2984 -6
- Partials 702 716 +14
Continue to review full report at Codecov.
|
rowanseymour
force-pushed
the
resend_msgs
branch
from
4761c06 to
98c09fa
Compare
core/models/msgs.go
Outdated
| @@ -458,12 +514,8 @@ func UpdateMessage(ctx context.Context, tx Queryer, msgID flows.MsgID, status Ms | |||
| return nil | |||
| } | |||
|
|
|||
| // MarkMessagesPending marks the passed in messages as pending | |||
| func MarkMessagesPending(ctx context.Context, db Queryer, msgs []*Msg) error { | |||
There was a problem hiding this comment.
not sure it's worth having a separate function for each status
There was a problem hiding this comment.
Why are we cloning the message? This feels like some remnant of the previous implementation that just adds complication. I'd rather we just re-queue it and make sure any kind of double sending cache or whatnot is cleared so that works. Cloning just seems odd especially given how often this occurs.
|
Good question I was just copying what we had in RapidPro.. maybe we did it this way to ensure it uses another topup? Will rework this to just requeue. |
|
Was thinking about this on the dog walk and I think it may have to do with the relayer, though that sounds like something perhaps worth revisiting. |
|
@nicpottier in order for mailroom to clear the double-send protection, it would have to know the specifics of courier's date-based redis sets we use for that... maybe mailroom queues it to courier with a |
|
Ya, that makes sense, can look at that today. |
|
@nicpottier reworked to reset message state rather than clone messages |
| // mark message as being a resend so it will be queued to courier as such | ||
| msg.m.Status = MsgStatusPending | ||
| msg.m.QueuedOn = dates.Now() | ||
| msg.m.SentOn = dates.ZeroDateTime |
There was a problem hiding this comment.
not sure about this.. I think it'll put a zero time in the json queued to courier.. but that doesn't matter?
There was a problem hiding this comment.
Does omit_empty leave it out if it is a zero value? Feels risky / weird having a 0 value there.
There was a problem hiding this comment.
Apparently not but aren't we always queuing sent_on="0001-01-01T00:00:00Z" to courier? I need to verify but I can't see how the regular queuing code would be any different.
There was a problem hiding this comment.
Ok now I've checked and we always queue to courier with sent_on="0001-01-01T00:00:00Z" and next_attempt="0001-01-01T00:00:00Z" because those are non-null in mailroom's model
| // mark message as being a resend so it will be queued to courier as such | ||
| msg.m.Status = MsgStatusPending | ||
| msg.m.QueuedOn = dates.Now() | ||
| msg.m.SentOn = dates.ZeroDateTime |
There was a problem hiding this comment.
Does omit_empty leave it out if it is a zero value? Feels risky / weird having a 0 value there.
|
Ya, if that's what we were doing then fine. Maybe this is an argument to add |
|
Just realized when looking at this further that both |
Part of #177 which would let us remove last of the topup and message sending code in RP