Page MenuHomePhabricator

Implement isStaffRelease method in PlatformSpecificTools
ClosedPublic

Authored by marcin on Aug 14 2023, 3:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Apr 7 2024, 6:22 PM
Unknown Object (File)
Apr 7 2024, 6:22 PM
Unknown Object (File)
Apr 7 2024, 6:22 PM
Unknown Object (File)
Apr 7 2024, 6:22 PM
Unknown Object (File)
Apr 7 2024, 6:22 PM
Unknown Object (File)
Apr 7 2024, 6:22 PM
Unknown Object (File)
Apr 7 2024, 6:22 PM
Unknown Object (File)
Apr 7 2024, 6:20 PM

Details

Summary

This differential introduces isStaffRelease in PlatformSpecificTools class. Method is callable from C++, Java and Objective-C code including NSE code. Ther reason for this functionality is that currently only JavaScript code can be landed
directly to the master and switched on only for staff users (using native/staff-utils.js). If developer wants to introduce some changes in native languages that sholud be active only for staff users they have to directly colaborate with release-maker.
With changes introduced in this diff developer can land staff-only changes directly to master by hiding them behind PlatformSpecificTools#isStaffRelease and release-maker only has to change two methods to return true inseated of false.

Test Plan
  1. Apply this diff: https://gist.github.com/marcinwasowicz/38435924b5850515649253a2e490572e (CommCoreModule, CommNotificationsHandler, NSE)
  2. Build iOS and Android app.
  3. Log in on both - observe logs from CommCoreModule.
  4. Send notification to both: Observe logs from CommNotificationHandler and NSE (this one requires usage of console app)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Rebase to put the diff at the bottom of branch

Is it necessary to have two places that we need to change? Could the isStaffRelease function have been placed in a shared CPP file, so it only needs to be updated in one place for both iOS and Android?

ashoat requested changes to this revision.Aug 15 2023, 8:44 AM

Talked about this with @marcin – it should be possible to keep this flag in just one place that is accessible from iOS and Android

This revision now requires changes to proceed.Aug 15 2023, 8:44 AM
  1. Implement the method in C++ and call from Java instead of implementing in Java and exporting to C++
atul added inline comments.
native/cpp/CommonCpp/Tools/StaffUtils.cpp
5 ↗(On Diff #29946)

So this is the value that'll need to be flipped for staff releases, correct?

We should probably document in the Mobile releases doc: https://www.notion.so/commapp/Mobile-releases-62f3ed6eed33413f9e2b09fcfc797e82

Shouldn't there be some change to the .pbxproj file for iOS? Are the new iOS files showing up in Xcode?

ashoat requested changes to this revision.Aug 16 2023, 1:03 PM
ashoat added a reviewer: jon.
ashoat added a subscriber: jon.

Requesting changes for .pbxproj. Adding @jon to confirm CMakeLists.txt updates are correct

This revision now requires changes to proceed.Aug 16 2023, 1:03 PM
native/cpp/CommonCpp/Tools/StaffUtils.cpp
5 ↗(On Diff #29946)

So this is the value that'll need to be flipped for staff releases, correct?

Exactly - staff releases must have this value set to true to unlock staff-only C++/Java/Objective-C code.

We should probably document in the Mobile releases doc: https://www.notion.so/commapp/Mobile-releases-62f3ed6eed33413f9e2b09fcfc797e82

Looks like there is no entry for staff-utils.js file that also gets updated on every staff release. Perhaps both of those places could be mentioned together in the same section?

CMake changes look good to me

This revision is now accepted and ready to land.Aug 17 2023, 8:33 AM