Set device_id and settings_json from MDM

[lognaturel]

Exploration for #6581

This is intended to be a quick spike to explore how MDM works. To set settings_json, make sure to include both general and admin settings blocks.

Out of scope:

• Sending confirmation back to MDM, we can do this as a follow-on item

Todo:

• make sure all error handling is robust -- nothing should happen if invalid values are set

The easiest way I've found to try this is:

• Run Test DPC app
• Run adb shell dumpsys user to get profile info
• Update Android Studio run config to add --user N to Launch Flags where N is the user number for the managed user

In Test DPC, search for "managed configuration", then select Collect and identify all keys from manifest.

Why is this the best possible solution? Were any other approaches considered?

The implementation's structure is the result of our discussions. We decided to listen for changes only in the main menu and the landing screen (when no projects are available). While this isn't perfect (settings may not be applied immediately), it’s a reasonable compromise that avoids introducing too many changes.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

The cases described in the issues are a good reference for what should be tested. Other than that, I can't think of any specific part of the functionality that requires additional testing.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Yes getodk/docs#1923

Before submitting this PR, please make sure you have:

• added or modified tests for any new or changed behavior
• run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
• added a comment above any new strings describing it for translators
• added any new strings with date formatting to DateFormatsTest
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[grzesiek2010]

@lognaturel I can't request your review as you are the creator of this pull request, but feel free to take a look.

[seadowg]

I've just a quick look, but I think there are a few pieces of criteria from the issue that might not work. Apologies if I'm missing something here. For example:

Given that Collect is not running and managed by MDM
When I change the settings_json key in my MDM system and those settings include a URL+username combination that does NOT match an existing project
Then a new project should be created with all of the settings I pushed

I don't think there's any code to cover this? As far as I can see we only deal with MDM configuration with a broadcast receiver, and that's only active while the FirstLaunchActivity or MainMenuActiivty are in a resumed state. The latter also means the following wouldn't work:

Given that Collect is running and managed by MDM
And I'm in the middle of form entry
When I change the device_id key in my MDM system
Then I should see that reflected next time I look at the settings or access deviceid from a form after returning to the main menu

As far as I can tell, we need to listen for broadcasts if the app is running (open or in the background) and the user is on the first launch screen, and we need to check for MDM config (with the RestrictionsManager) when returning to the main menu (and while it's visible).

[grzesiek2010]

@seadowg
We don’t need to constantly listen for broadcasts since they’re only used to trigger actions when we're in the main menu or the first launch screen. If we're not there, the next time we navigate to that screen, we'll get the new config because it's always available when onResume is called: https://github.com/getodk/collect/pull/6582/files#diff-a548976b1bfd075eabc5d36bbdb698d67a7581cf052754c93b2dc18dc893d58aR30

Given that all the criteria mentioned in the issue should be met.

[seadowg]

@grzesiek2010 ah I'd missed the applyConfig call there!

[seadowg]

I've pointed out a few specific things and also wanted to make some broader comments

There's currently no tests that verify that this is all setup. I'd push for having instrumentation tests that check flows like starting the app for the first time on a device with MDM setup and there being a configuration change while the user is on the main menu etc. This initially seems a little awkward, but is pretty doable by managing the RestrictionsManager in Dagger so that it can be swapped out with a stub in tests, and also introducing a BroadcastReceiverRegister interface like this:

interface BroadcastReceiverRegister {
    fun registerReceiver(context: Context, receiver: BroadcastReceiver)
}

That would allow us to have a test implementation that we can manually trigger (like we do with Scheduler).

I also, think there's two refactors that could make these changes a little simpler, but they're just suggestions:

1. Making first launch a Fragment and moving it into MainMenuActivity. This would make switching between two simpler (it could even use Jetpack Navigation) and allows sharing the dependency setup for projects, MDM etc
2. Flipping the relationship with ProjectCreator and ProjectsDataService so that createNewProject is a method on the data service. ProjectsDataService could also have a configureProjectFromJSON method that handles the "UPSERT" configuration and reduce the MDM specific code down to just "parsing" the bundle from broadcast/RestrictionsManager.

[grzesiek2010]

There's currently no tests that verify that this is all setup. I'd push for having instrumentation tests that check flows like starting the app for the first time on a device with MDM setup and there being a configuration change while the user is on the main menu etc.

Done.

[seadowg]

I've still got to have a proper look at MDMConfigHandler, but I'm finishing up for the day, and I thought it made sense to give you the thoughts I do have!

[seadowg]

I think every scenario here should save config and trigger a broadcast right? That's what will happen on device when the config changes (whether Collect is "listening" or not).

[grzesiek2010]

If we're outside the landing screen/main menu, there is no registered broadcast, so there's nothing to trigger. In those cases, it's enough to save the config, and it will be applied in onResume the next time we open those screens.

[seadowg]

Right, but the broadcast still happens. We want to trigger the FakeBroadcastReceiverRegister to make sure that everything behaves as expected. The correct implementation will obviously not have a receiver registered at that point, so settings won't change etc. An incorrect implementation where the receiver was registered would cause a fail however.

[seadowg]

Does this need to be translated?

[lognaturel]

I believe that if the MDM provider supports multiple languages, it will used the localized string. I haven't verified this but the docs implied it.

[seadowg]

As I'd said before, I think it would probably be nicer for us to be interacting with the ProjectsDataService here, but I think there's a solid dividing line between the Android parts (broadcast receiver, lifecycle hooks and restrictions manager) and the domain logic (in MDMConfigHandler) which should let us make a change to something like that later.

[seadowg]

#6582 (comment)
#6582 (comment)

[seadowg]

@grzesiek2010 sorry I hit the wrong button earlier and somehow ended up requesting a review from you rather than leaving one.

[grzesiek2010]

@lognaturel @seadowg
There is one issue you should be aware of. If the current project is updated while we are in the main menu, the new settings are applied immediately. However, the UI does not refresh automatically. This means that if the new configuration includes changes that affect the main menu, such as hiding certain buttons, those changes won't be reflected right away. Updating the UI dynamically in this scenario is not straightforward, so it may not be a major concern, but I wanted to discuss it with you.

[lognaturel]

the new settings are applied immediately. However, the UI does not refresh automatically.

Yes, and it's a strange experience because it refreshes right before changing to another activity. I mentioned this when I demoed it for the Insiders call and Yaw asked about it. We agreed it's ok. That said, I found it surprising that it does refresh right before switching activities. Do you have an explanation for that?

One other thing -- I couldn't get to get this branch as-is to work with TinyMDM. I ended up copying the restrictions block from the module manifest to the app manifest and that worked.

[grzesiek2010]

I found it surprising that it does refresh right before switching activities. Do you have an explanation for that?

For example, when you navigate from the main menu to the list of blank forms? If so, I don't see anything like that in tests.

One other thing -- I couldn't get to get this branch as-is to work with TinyMDM. I ended up copying the restrictions block from the module manifest to the app manifest and that worked.

This should be fixed by 1156ca0

[lognaturel]

This should be fixed by 1156ca0

Confirmed, thanks!

On Galaxy A13 which is fairly slow, notice I tap "Start new form" and the project title changes:

screencapture-1741821440389.mp4

[grzesiek2010]

Ah, it's just the project title. This happens because, in MainMenuFragment, we observe the current project and update the title whenever it changes. The update occurs when navigating to the list of forms, and if the main menu is still visible, it reflects the change.
It's still the same project, so I don't think it should be updating. There might be an issue there. I’ll take a look and file an issue if needed.

[seadowg]

This could be defined later so you wouldn't need the !! later.

[seadowg]

I'm a little confused by this test: the ODKAppSettingsImporter being used here is a mock, so I wouldn't expect any settings to get written anyway?

[seadowg]

@lognaturel @grzesiek2010 I'm thinking we should file an issue around the project details not updating on the main menu. I think this is related to the way we interact with projects not being centralized (as I mention in #6582 (comment)) which is a hangover from before this PR. It'd be good to rework things so we more easily support project changes not being made directly by the user (which allows to just reload things explicitly).