feat(plugin): a data set instance content replacement slot [skip size] - #5163
Merged
Merged
Conversation
The plugin system had one injection point, and it was additive: PluginSlot renders every registered plugin for a slot, stacked, inside host UI that stays put. Nothing was ever replaced, and no plugin was ever scoped to a particular object. DATA_SET_INSTANCE_CONTENT is the first replacement slot. A plugin takes over the body of the data set instance screen - the panes, section tabs and table - for the data sets an administrator configures, and the host keeps everything framing it: the top bar with its title, back and sync actions, the save button, the bottom bar with validation, completion and the non-editable reason, and the snackbar. A data set no plugin claims keeps the default table. Three things this needed that the additive slot did not. Per-object targeting. PluginMetadata gains slotConfig, a map keyed by injection point whose values are opaque JSON objects, each slot owning its own schema. Keeping them unparsed at the envelope and decoding them in the slot is what makes a new slot additive: it brings a configuration type and an arguments type and changes nothing else. A flat list of UIDs would have answered today's question and been the wrong shape the moment a slot needs a program plus its stages. InjectionPoint records whether a slot is meaningless unconfigured, so a replacement slot renders nowhere until it is told what it applies to rather than taking over every data set on the server. Somewhere for the plugin to learn which instance is open. SlotArguments, read through LocalSlotArguments, with the matching rule (appliesTo) on the arguments themselves - so the host's render path asks "does this plugin claim this occurrence" without knowing what a data set is. Exactly-one-wins with a fallback. selectReplacement takes the first plugin in configuration order and logs the rest; stacking two full-screen layouts is not an option, and falling back to the host would let one admin's typo disable another team's plugin. :aggregates gains no dependency on the plugin modules - it is KMP with a desktop target and :plugin is Android-only. DataSetInstanceScreen takes one nullable parameter instead, and :app, which already depends on both, decides. The parameter is nullable rather than a decorator plus a flag because the decision has two consequences - what renders, and whether the view model builds the tables at all - and they must not be able to disagree. Skipping that table build is worth having. loadDataSet was already two phases: the details, sections and rendering config the chrome needs, then one section query plus a data value query per table group to build tables nothing would render. Only the second is skipped. Everything else stays, and stays correct over data the plugin wrote itself, because validation, completion and the save flow all query the SDK rather than the host's in-memory table. Also fixes a hazard the new enum value would otherwise have introduced: the config was decoded in one go, so an injection point an installed app had never heard of failed the whole parse and took every other plugin down with it. Entries are now decoded one at a time, and unknown slot names are dropped from a plugin's list rather than failing its entry - so rolling out a new slot does not disable an administrator's existing plugins on devices that have not been updated yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Move plugin resolution for data set instances from `DataSetInstanceActivity` to a new `DataSetInstanceBodyProvider` interface. This decoupling allows the `:aggregates` module to support UI replacements without a direct dependency on the plugin system, while enabling the `:app` module to wire them together via Koin. Key changes: - Introduce `DataSetInstanceBodyProvider` and `DataSetInstanceBody` interfaces in `:aggregates`. - Implement `PluginDataSetInstanceBodyProvider` in `:app` to bridge the `PluginRegistry` with the aggregates UI, moving selection logic out of the Activity. - Update `DataSetTableViewModel` to query the provider during initialization. If a replacement body is present, the view model skips building the default tables, avoiding expensive and redundant SDK queries. - Refactor `DataSetInstanceScreen` to resolve body replacements via DI rather than taking a nullable Composable parameter. - Simplify documentation and comments across `PluginMetadata`, `PluginRegistry`, and `SlotArguments` to focus on rendering behavior rather than access control. - Update `docs/plugin-system.md` to document the new wiring architecture between `:app`, `:aggregates`, and `:plugin`. - Add unit tests for `PluginDataSetInstanceBodyProvider` to verify replacement selection logic.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



What
DATA_SET_INSTANCE_CONTENT, the plugin system's second injection point and its first replacement slot: a plugin takes over the body of the data set instance screen for the data sets an administrator configures, and the host keeps everything framing it — the top bar with its title, back and sync actions, the save button, the bottom bar with validation, completion and the non-editable reason, and the snackbar. A data set no plugin claims keeps the default table.The existing slot is additive —
PluginSlotstacks every registered plugin inside host UI that stays put. Replacement needed three things it never did: per-object targeting, a way for the plugin to learn which instance is open, and exactly-one-wins with a fallback.Server configuration
PluginMetadatagainsslotConfig, keyed by injection point, values opaque:{ "id": "org.myorg.nutrition-form", "version": "1.0.0", "entryPoint": "org.myorg.nutrition.NutritionPlugin", "downloadUrl": "https://example.com/nutrition-1.0.0.zip", "checksum": "sha256:abc…", "injectionPoints": ["DATA_SET_INSTANCE_CONTENT"], "slotConfig": { "DATA_SET_INSTANCE_CONTENT": { "dataSetUids": ["lyLU2wR22tC", "BfMAe6Itzgt"] } } }Each slot owns its own schema, so the value is kept unparsed at the envelope and decoded by the slot that understands it. That is what makes a new slot additive — it brings a configuration type and an arguments type and changes nothing here. A flat UID list would have answered today's question and been the wrong shape the moment a slot needs a program plus its stages.
It is a rendering filter and says so in its KDoc: it decides where the host asks a plugin to draw and narrows nothing about what it can read or write.
allowedDataSetUidswas removed in 86bd5af for promising enforcement that did not exist, and this must not be mistaken for its return.InjectionPointnow recordsrequiresConfiguration, so a replacement slot renders nowhere until it is told what it applies to, rather than taking over every data set on the server because someone pasted an entry. An empty list doubles as a kill switch that does not require deleting the entry.The seam
:aggregatesgains no dependency on the plugin modules — it is KMP with a desktop target and:pluginis Android-only.DataSetInstanceScreentakes one nullable parameter and:app, which already depends on both, decides:Nullable rather than a decorator plus a
loadTablesflag because the decision has two consequences — what renders, and whether the view model builds the tables at all — and they must not be able to disagree.contentPaddingcarries the space the save button occupies (FabContainerHeightis private to the screen, so:appcannot compute it).onHostRefreshis the view model'sloadDataSet, passed out through the lambda because it only exists inside the screen.DataSetBodySurfaceis extracted and made public so a replacement sits in the same rounded, raised frame as the default table instead of every plugin re-implementing it.Skipping the table build
loadDataSetwas already two phases: the details, sections and rendering config the chrome needs, then one section query plus a data value query per table group. Only the second is skipped, andonSectionSelectedgoes inert. Everything else stays — and stays correct over data a plugin wrote itself, becauseRunValidationRules,CheckCompletionStatusandCompleteDataSeteach take only the four identifiers and query the SDK, never the host's in-memory table. Persistence is the plugin's, validation is the host's, and they meet in the SDK's database.A hazard this closes
The config was decoded in one go, so an injection point an installed app had never heard of failed the whole parse — and a device that had not been updated yet would lose the administrator's existing home-screen plugin too, for a slot that could not have rendered there anyway. Entries are now decoded one at a time, and unknown slot names are dropped from a plugin's list rather than failing its entry.
Note this changes one test's expectation on purpose:
a plugin entry missing a required field is reported as a failurebecomes…is skipped, not fatal. The mistake is still surfaced, in the log, naming the entry.LoadPluginsUseCasealready promised per-plugin failure isolation; the config parse was the one step that broke it.Tested
./run_tests.sh— 1839 tests, 0 failures.:aggregates:desktopTestpasses, which is the guard that the seam stayed KMP-clean.Verified on a device against a local DHIS2 instance, with the template plugin from
dhis2-android-plugin-sampleconfigured for one data set:That covers what only the real app can exercise: registry selection on
slotConfig, the composition local crossing the class-loader boundary, resource resolution, Koin isolation and Compose version skew.Reviewing
DataSetTableScreen.ktis most of the diff and nearly all of it is one indent level: wrapping the body region in a lambda reindents ~150 lines, which git counts twice. The pre-existing double-paddingValuesinset on the single-paneContentLoadingbranch is left alone so "no behaviour change" stays literally true.Happy to split this into the three commits it was planned as — the
:aggregatesseam, the plugin contract and render path, then docs and the Gradle snippet — if that reviews better.Known limitations, documented in §7
LocalHostRefreshis a pull, so a plugin that forgets to call it leaves the chrome showing a stale completion status.Loaded. Previously hidden behindContentLoading, now visible for a beat over filled plugin content. Fixing it means touching the view model's state modelling.🤖 Generated with Claude Code