Page MenuHomePhabricator

[lib][native] Pass CSAT to Reports endpoint
ClosedPublic

Authored by bartek on Feb 2 2024, 1:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 26, 4:44 AM
Unknown Object (File)
Fri, Nov 8, 10:03 PM
Unknown Object (File)
Fri, Nov 8, 7:15 PM
Unknown Object (File)
Fri, Nov 8, 7:15 PM
Unknown Object (File)
Fri, Nov 8, 7:15 PM
Unknown Object (File)
Fri, Nov 8, 1:36 PM
Unknown Object (File)
Oct 3 2024, 11:47 PM
Unknown Object (File)
Oct 3 2024, 11:47 PM
Subscribers

Details

Summary

Final part of ENG-6289.
Passed AuthMetadata to sendReports() calls if possible.

Depends on D10930

Test Plan
  • Sent multimedia messages - checked Reports service - reports had user ID set
  • Triggered a crash with CSAT unset - user ID was unknown in generated report
  • Sent a message with CSAT set - user ID was set in generated report

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Feb 2 2024, 2:33 AM
bartek added a reviewer: varun.
bartek added inline comments.
native/crash.react.js
163 ↗(On Diff #36579)

This works here, but I'm wondering if calling commCoreModule.getCommServicesAuthMetadata() directly from here wouldn't be better in case of a crash

I wonder if there's a more generic way we could handle this... I could imagine defining this similar to keyserver actions, where "Comm service actions" are defined as functions that take functions as input. The inner function would be something like callAuthenticatedCommService, and it would allow the "Comm service action" to abstract away the details of authentication. Then we would have a hook (eg. useCommServiceCall) that takes the "Comm service action" function as input, and binds in the callAuthenticatedCommService function.

I wonder if there's a more generic way we could handle this... I could imagine defining this similar to keyserver actions, where "Comm service actions" are defined as functions that take functions as input. The inner function would be something like callAuthenticatedCommService, and it would allow the "Comm service action" to abstract away the details of authentication. Then we would have a hook (eg. useCommServiceCall) that takes the "Comm service action" function as input, and binds in the callAuthenticatedCommService function.

I've proposed something less generic but also less complicated in https://phab.comm.dev/D10912#316146

native/crash.react.js
163 ↗(On Diff #36579)

If this component is rendered below the provider, it should work the same. On the other hand, we can't be sure what caused the crash, which means that reducing the number of layers we're using here is beneficial. Up to you.

This revision is now accepted and ready to land.Feb 6 2024, 1:44 AM
native/crash.react.js
163 ↗(On Diff #36579)

On the other hand, we can't be sure what caused the crash, which means that reducing the number of layers we're using here is beneficial.

Agree with this. We should keep things simple here

  • Used hook abstraction as proposed
  • Used direct commCoreModule call in crash component