[Vortex] Add PushNotificationDeferredResponseInterface for JPush reponse
Review Request #953 — Created May 26, 2023 and submitted
Information | |
---|---|
b.stoop | |
Lunr.Vortex | |
deferred/interface | |
|
|
50d3781... | |
Reviewers | |
lunr | |
Based on release/0.7.x
Add PushNotificationDeferredResponseInterface for JPush reponse
unit tests
-
-
src/Lunr/Vortex/JPush/JPushResponse.php (Diff revision 1) I don't like the disconnect between endpoints and deferred batches. It's gonna be rather annoying to verify that the data matches.
All the endpoints marked as
DEFERRED
need to be passed to the Job fetching the status, which then needs to fetch a status for each of them. But the additional info is passed along indexed bymessage_id
(which isn't generic, BTW, and thus might cause issues when doing the same for FCM/whatever).
The Job needs to iterate over every endpoint, since it needs to be make every endpoint gets a new status. It also needs to iterate over the deferred batch, to check for overlap, extract that matching message IDs and request once per message ID the status of all relevant endpoints, and then loop again over all endpoints to assign the correct status.That's an awful lot of house keeping :-/
IMHO, the info needed to fetch the deferred status should be attached to the endpoints themselves.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+237 -22) |
Change Summary:
Changed return type for get_batch() function
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+237 -22) |
-
-
-
src/Lunr/Vortex/JPush/JPushResponse.php (Diff revision 3) what if the
batch
index doesn't exist? What if$this->statuses[$endpoint]
is not an array?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+264 -23) |
-
-
-
src/Lunr/Vortex/PushNotificationDeferredResponseInterface.php (Diff revision 4) I think the terminology here isn't quite right. For us it's a
batch
, but for the provider it should just be a message. There's no way to link multiple batches to the same notification on the provider side.With this in mind, I think
get_message_id()
might be better.Also, I don't think we can guarantee that message IDs are always integers. They might contain characters or they might be a uuid. So I think the method should return
?string
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+264 -23) |
-
-
src/Lunr/Vortex/JPush/JPushResponse.php (Diff revision 5) Why the string cast? What does changing
NULL
to""
solve here?
Summary: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 6 (+269 -23) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+269 -23) |