feat(#566): implement auto-inclusion of task and target and contact s… - #784
feat(#566): implement auto-inclusion of task and target and contact s…#784inromualdo wants to merge 23 commits into
Conversation
|
@inromualdo sorry for the delay! Very interested in the feature you have added here! Before I get too far into reviewing the code, can you help me understand a bit better how this is expected to work? Here is how I think things go and maybe you can correct me if I get anything wrong:
There are some tradeoffs here between clarity and convenience. (There is more complexity when we are merging files behind the scenes and it adds another level of abstraction between what is seen in the config files and what is actually uploaded to the server. On the other hand, making configuration more reusable has clear benefits as well.) I think I see how this works for just dropping new files into the project directory. How will this work for config coming from re-usable NPM libs like https://github.com/medic/cht-stock-monitoring-workflow? Would I still have to manually copy files from that repo into my local config? |
There was a problem hiding this comment.
Okay, thanks for your patience @inromualdo! I have been doing a ton more thinking about this and digging through the existing code along with your changes here. I think this is a really exciting opportunity to step into a new era of CHT configuration. What you have here does a good job being passive and minimal in terms of the changes it envisions. Instead, I would like to propose we go all-in on this and expand our approach to just set a new standard! To summarize, I am proposing:
- We remove all remaining support for "non-declarative" configuration for tasks/targets/contact-summary.
- Add support for dynamically reading configuration from files in the
./contact-summary,./tasks, and./targetsdirectories (using directories to group the files instead of your current approach with the.contact-summary/tasks/targets.jsprefix).- Deprecate the
contact-summary.templated.js,tasks.js, andtargets.jsfiles, but retain support for reading config from them for backwards compatibility.
- Deprecate the
- Add support for not including contact-summary/tasks/targets configuration in a project (just default to empty config instead).
With this approach, a typical basic configuration structure would look something like:
app_settings/base_settings.json
contact-summary/base.js
targets/base.js
tasks/base.js
You can expand the contact-summary/tasks/targets configuration by adding additional files to the associated directory. Running the compile-app-settings action will pull everything together into the ./app_settings.json file.
What do you think? I am trying to actually reduce complexity by just standardizing along the intended go-forwards code paths. Do you think this will be useful/maintainable? Do you have any ideas on how to improve this (or achieve the same objective more cleanly)?
I don't think implementing what I am going for here would take a huge amount of additional effort. You have already done the most difficult design work of figuring out the details for merging the config. After poking around the code a bit, here is my thoughts on the implementation details, but we might be able to come up with a better approach....
- contact-summary:
- Remove support for "freeform" contact summary configuration (aka the
contact-summary.jsfile). Only keep support for "structured" (declarative) contact-summary config.- Going forwards, we will only support reading the declarative contact summary config from
contact-summary.templated.jsfile andcontact-summary/*.jsfiles.
- Going forwards, we will only support reading the declarative contact summary config from
- Move the contact-summary merge functionality out of
contact-summary/lib.jsintocontact-summary-emitter.jsso that it takes an array ofcontactSummariesinstead of the currentcontactSummaryparameter. - Remove the
contact-summary/lib.jsfile and replace it with a file generated at compile-time in thecompile-contact-summary.jslogic (similar tocht-cards-extensions-shim.js).- The generated file should only:
- Require the
contact-summary.templated.jsfile and all the variouscontact-summary/*.jsfiles, - Use the emitter to emit the contact summary config.
- Require the
- The logic should just treat the
contact-summary.templated.jslike the othercontact-summary/*.jsfiles, but it should be the "most preferred".- Also can log a message that
contact-summary.templated.jsis deprecated and should switch tocontact-summary/base.js
- Also can log a message that
- The generated file should only:
- Add support for not having any
contact-summary.templated.jsfile andcontact-summary/*.jsfiles (and just adding empty config into the app_settings)
- Remove support for "freeform" contact summary configuration (aka the
- tasks/targets:
- Remove support for
rules.nools.jsconfig (nools). Only keep support for declarative config.- Going forwards we will only support reading the declarative config from
tasks.js,targets.js,tasks/*.js, andtargets/*.jsfiles.
- Going forwards we will only support reading the declarative config from
- Remove the
nools/lib.jsand replace it with a file generated at compile-time in thecompile-tasks-and-targets.jslogic (similar to how the current*extensions-shim.jsfiles are being generated).- The generated file should only:
- Require the
tasks.js,targets.js,tasks/*.js, andtargets/*.jsfiles. - Use the emitters to emit the tasks and targets.
- Require the
- The logic should treat the
tasks.jsandtargets.jslike the othertasks/*.js, andtargets/*.jsfiles, but thetasks.jsandtargets.jsshould be "most preferred".- Also can log a message that
tasks.jsandtargets.jsare deprecated and should switch totasks/base.js, andtargets/base.js
- Also can log a message that
- The generated file should only:
- Add support for not having any
tasks.js,targets.js,tasks/*.js, andtargets/*.jsfiles (and just adding empty config into the app_settings)
- Remove support for
|
@inromualdo did you manage to apply @jkuester's feedback? Are you still planning to continue working on this issue? |
Add compile-app-settings scenarios for the directory layout and for a project that omits all declarative config. Regenerate expected outputs for tasks.isDeclarative (now always true) and remove the obsolete "both legacy and declarative nools" test now that nools is unsupported. BREAKING CHANGE: removes support for freeform contact-summary.js and nools rules.nools.js. Projects must use declarative config in the contact-summary/, tasks/ and targets/ directories (or the deprecated contact-summary.templated.js / tasks.js / targets.js single files).
- validate cross-file uniqueness of target id / task name (per-file joi .unique() could not catch duplicates spanning directory files), which would otherwise ship duplicate targets/tasks with colliding ids at runtime - validate every config file instead of short-circuiting on the first failure, so all schema errors surface in one pass - write each generated webpack entry to a unique mkdtemp dir so concurrent compiles can no longer clobber each other's entry file - normalise targets.json + targets/*.js merge through the same field whitelist and tolerate a legacy top-level-array targets.json
- warn when the removed rules.nools.js or freeform contact-summary.js is present, so upgraders get a clear migration message instead of silent ignore - extract the shared "deprecated base file first, then directory files" discovery (with optional deprecation/info logging) into auto-include collectConfigFiles, consumed by both compile steps and the schema validator - extract the generated-entry tmp-dir/write + require-statement building into a shared generated-entry helper used by both compile steps - delete the dead webpack aliases for the deprecated single files in package-lib - use readdirSync withFileTypes instead of a per-file statSync, and drop the redundant fileExists wrapper in favour of the shared helper
parse-targets previously kept its own hand-maintained TARGET_FIELDS list of
properties to copy into app_settings.tasks.targets, which could (and had)
drift from TargetSchema: percentage_count_translation_key was valid per the
schema but missing from the whitelist, so it was silently stripped.
Tag the static display-metadata properties on TargetSchema with
.meta({ appSettingsTarget: true }) and derive the whitelist from those tags,
making the schema the single source of truth. Adding a tagged property is now
the only step needed for it to flow into app_settings; runtime/logic
properties stay untagged. Fixes the percentage_count_translation_key drop.
- contact-summary-emitter: replace var with const/let in the merge code and extract mergeContext to drop mergeContactSummaries cognitive complexity to 5 - parse-targets: use object spread instead of Object.assign, and throw TypeError (not Error) for the non-array type check - validate-declarative-schema spec: assert each runtime field is excluded individually instead of the uncertain .to.not.have.members assertion
Extract normalizeEntry so the merge loop has a single branch, dropping the flagged Cognitive Complexity below the SonarCloud threshold.
targets.json is read only by parse-targets (never the compile/collectConfigFiles path), so it was the one legacy format without a deprecation nudge. Warn once when it is present; targets.js keeps its warning from the compile step to avoid double-warning.
|
@jkuester , I implemented the directory based config design from your review... Tasks, targets, and contact-summary can now be split across multiple files in tasks/, targets/, and contact-summary/ directories, compiled and merged at build time The diff looks big 😔, so here is a summary 😊 :
the other files are tests and fixtures |
3 participants
Description
Ability to compile and upload tasks and contact summary from reusable componnent. This PR auto include any file end by
.tasks.js,.targets.jsand.contact-summary.js#566
Code review items
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.