[Vortex] FCM: Send fcm notifications in batches

Review Request #1126 — Created Feb. 29, 2024 and submitted

Information
b.stoop
Lunr.Vortex
fcm/batchsending
1090
1136
5ae6af0...
Reviewers
lunr 
Based on master


FCM: Send fcm notifications in batches
Github actions run: https://github.com/brianstoop/lunr.vortex/actions/runs/8891789477
  • 0
  • 0
  • 4
  • 1
  • 5
Description From Last Updated
pprkut
pprkut 
  1. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    is it really a permutation matrix between those 4 values? i.e. a+c, a+d, b+c and b+d?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      No, it could only be something like array<string,Response|RequestsException>|array{0,Response}.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Then that would be the better type :)
      
 
      

      3. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    This sounds wrong. Whenever Unknown shows up in logs it should be considered a bug in the status mapping logic. Are we sure we don't know what the status here is and need more information to fix this later? If so, please add a comment to clarify this in the code.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I looked at the old code and how we mapped this, see https://github.com/lunr-php/lunr.vortex/blob/release/0.8.x/src/Lunr/Vortex/FCM/FCMDispatcher.php#L174 https://github.com/lunr-php/lunr.vortex/blob/release/0.8.x/src/Lunr/Vortex/FCM/FCMDispatcher.php#L210 and https://github.com/lunr-php/lunr.vortex/blob/release/0.8.x/src/Lunr/Vortex/FCM/FCMBatchResponse.php#L150 Here we can see that when we don't set a status code it will be mapped to UNKNOWN, which is the same as what is happening here. Nothing changed here AFAIK.
      
 
      

      2. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    hmm, not really, no? Unknown would mean "we sent it, but haven't heard back". For that to be true we'd need to check whether it's in $this->endpoints too and only return Unknown if it is.
    

    If it isn't in $this->endpoints it wasn't part of the batch. That should be an exception IMHO.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      src/Lunr/Vortex/FCM/FCMBatchResponse.php
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    This is a correct use of Unknown, for example. Whenever we reach this, we know the switch above is missing a case statement.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 



 

  

   




b.stoop




   

   

  

 



 















 





 

  

   




pprkut




   

   







  

 



 

  


   

   

    
 
pprkut


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      src/Lunr/Vortex/FCM/FCMBatchResponse.php
      


       (Diff revisions 1 - 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    correct array shape syntax would be array{0: Response}
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




b.stoop




   

   

  

 



 















 





 

  

   




pprkut




   

   







  

 



 

  


   

   

    
 
pprkut


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




pprkut




   

   







  

 



 

  


   

   

    
 
pprkut


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 







  2. 
 
    
  
  
    
  
   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Needs a rebase
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 













  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




b.stoop




   

   

  

 



 















 



 

  

   




b.stoop




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to master (5ae6af0)





 


 










  

 









     

    

   

  

  

   
   Loading...