[Vortex] Add PushNotificationDeferredResponseInterface for JPush reponse

Review Request #953 — Created May 26, 2023 and submitted

b.stoop
Lunr.Vortex
deferred/interface
951
50d3781...
lunr

Based on release/0.7.x

Add PushNotificationDeferredResponseInterface for JPush reponse

unit tests

  • 0
  • 0
  • 6
  • 0
  • 6
Description From Last Updated
pprkut
pprkut
  1. 
      
  2. 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 by message_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.

  3. 
      
b.stoop
b.stoop
pprkut
  1. 
      
  2. src/Lunr/Vortex/JPush/JPushResponse.php (Diff revision 3)
     
     
     
     
     
     

    Needs updating?

  3. 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?

  4. 
      
b.stoop
pprkut
  1. 
      
  2. src/Lunr/Vortex/JPush/JPushResponse.php (Diff revision 4)
     
     
     
     
     

    Doesn't match the type of $this->statuses

  3. 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

  4. 
      
b.stoop
pprkut
  1. 
      
  2. src/Lunr/Vortex/JPush/JPushResponse.php (Diff revision 5)
     
     

    Why the string cast? What does changing NULL to "" solve here?

    1. You changed the implementation, but you didn't answer the question :)
      Why the string cast?

    2. It wasn't my intention to change the NULL to string, I only wanted to cast the integer to a string. I think I just overlooked the fact that it returned NULL too

    3. That's fine, but why do you want to cast the ints to string? :)

    4. https://reviews.lunr.nl/r/953/#comment2289. Like you said we can't guarantee that message IDs are always integers so it would be better to return ?string

    5. For int values returned by the BatchResponse, if we want the array types to be strictly adhered to, something like this would be better IMHO:

      $message_id === NULL ? NULL : (string) $message_id
      
  3. 
      
b.stoop
b.stoop
pprkut
  1. Ship It!
  2. 
      
b.stoop
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release/0.7.x (50d3781)
Loading...