Details

Reviewers

atul
kamil
bartek
ashoat
• jon

Commits

rCOMMda5391ff5719: Implement isStaffRelease method in PlatformSpecificTools

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

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

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Aug 14 2023, 3:01 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 14 2023, 3:01 AM

marcin edited the test plan for this revision. (Show Details)Aug 14 2023, 3:05 AM

Rebase to put the diff at the bottom of branch

marcin added a child revision: D8795: Implement synchronization mechanisms to address process, thread and class level concurrency..Aug 14 2023, 3:10 AM

Harbormaster completed remote builds in B21725: Diff 29853.Aug 14 2023, 3:19 AM

Harbormaster completed remote builds in B21726: Diff 29854.Aug 14 2023, 3:27 AM

marcin requested review of this revision.Aug 14 2023, 3:27 AM

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 mentioned this in D8730: Implement displaying error message notification for cases that might cause empty notification on Android.Aug 15 2023, 8:42 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

Implement the method in C++ and call from Java instead of implementing in Java and exporting to C++

Harbormaster completed remote builds in B21788: Diff 29942.Aug 16 2023, 5:00 AM

bartek accepted this revision.Aug 16 2023, 5:24 AM

Add missing newlines

Harbormaster completed remote builds in B21791: Diff 29946.Aug 16 2023, 6:02 AM

atul accepted this revision.Aug 16 2023, 10:53 AM

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?

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

Necessary XCode chages

Harbormaster completed remote builds in B21822: Diff 29994.Aug 17 2023, 1:33 AM

ashoat accepted this revision.Aug 17 2023, 6:35 AM

marcin added inline comments.Aug 17 2023, 7:54 AM

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?