feat(#474): generic templates and dynamic placeholder vars

[ChinHairSaintClair]

Description:

This feature introduces the support for:

• multiple templates
• conditional edit form creation
• dynamic placeholder variables

Multiple templates

In addition to PLACE forms, users can now define multiple PERSON forms templates. As outlined in the original issue, this enables handling situations where multiple groups of contact forms share a similar structure:

Nation, Region, District -> PLACE_TYPE_1
Supervisor Area, CHW Area -> PLACE_TYPE_2
National admin, Regional admin, District admin -> PERSON_TYPE_1
Supervisor, CHW -> PERSON_TYPE_2

Conditional edit form creation

The CONTACT_TYPES.json config supports an optional templateEdit property. If this property is omitted, no edit form will be created. This could reduce form maintenance overhead, where separate create/edit forms are unnecessary. In such cases, consolidating logic into a single form could help avoid drift and missed updates during ongoing maintenance.

Dynamic placeholder variables

Building on the existing PLACE_TYPE and PLACE_NAME placeholders, users can now define additional custom variables for each form. To avoid unintended replacements, all custom placeholders must use the __cht_var- prefix.
For example, in a template it could be used to keep a spot for:

• database lookup type, or
• perhaps a primary contact type.

Questions:

• Should we prefix template file names with a keyword?
  If a contact-types.json file is not provided, the system cannot distinguish templates from standard form files. As a result, all files are processed as regular forms, which IIRC differs from the original behaviour. One possible solution is to introduce a file name prefix, for example:
  CONTACT-<template_name>.xlsx

• Should template config filenames be case- and format-insensitive? E.g:

  • place-types.json
  • PLACE-TYPES.json
  • place_types.json
  • PLACE_TYPES.json

  Aligning this with the template xlsx naming convention (PLACE_TYPE-create.xlsx) could improve consistency.

• Is there a need to support templates for app forms?
  The feature should be able to support that. However, the content in these forms tend to differ considerably, so it may not provide much value. That said, I'm happy to add it support if there's a use case I might have missed.

• Should we purge template sections that are not relevant for certain types/forms?
  For example, consider a project where PLACES largely share the same form structure except for one level. The additional fields for that level can be included as conditional section(s) in the template. While these sections (or fields) can be hidden at runtime based on type, their content will still exist in each copied form and must be loaded and evaluated every time the form is opened. We could instead purge these unnecessary sections/fields on template -> form creation.

Issue: 804

Forum discussion:
https://forum.communityhealthtoolkit.org/t/place-type-behaviour-clarification/5514

Code review items

• Readable: Concise, well named, follows the style guide, documented if necessary.
• Documented: Configuration and user documentation on cht-docs
• Tested: Unit and/or integration tests where appropriate
• Backwards compatible: Works with existing data and configuration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

[jkuester]

Okay, first of all, sorry for the delayed review! 😓 Things have been crazy busy.

I finally got several hours this afternoon to sit down and work though this functionality thoroughly. I think before getting into the details of the code review, I want to start with a few high-level thoughts we can discuss:

Multiple templates

Your approach to switching from the place-types.json to contact-types.json makes sense and it nice and incremental. One thing I felt was annoying about the PLACE_TYPE templates, though, was this mixing of template form files, real form files, and generated form files all in the forms/contact directory (and then the place-types.json was thrown in the mix just to make things more fun...). If you knew exactly how everything worked it could make sense, but for someone not familiar with the templating, it is really confusing to grok.

One alternative structuring I have been considering is to have a forms/contact/templates directory. That would at least move the template files out of the main forms/contact directory and create a clearer delineation between which are the actual contact form and which are the templates.

Also, instead of having the contact-types.json file at all, in the forms/contact/templates directory we could have .properties.json files for each of the template forms with a template_contact_types property (or something like that) which would define which contact forms should be generated from that template. So, in forms/contact/templates you could have a patient-create.xlsx file and a patient-create.properites.json file that contains the template_contact_types of household_head and household_member. Then you have a user-create.xlsx file and a user-create.properites.json file that contains the template_contact_types of chw and chw_supervisor. The end result of convert-contact-forms should be that in forms/contact you have the files:

• household_head-create.xlsx
• household_head-create.xml
• household_member-create.xlsx
• household_member-create.xml
• chw-create.xlsx
• chw-create.xml
• chw_supervisor-create.xlsx
• chw_supervisor-create.xml

You also get full control over which of the create/edit forms you actually create. Not saying we have to go this way if you don't think it is a good idea, but I am just looking for ways to make this work in the least surprising way (while still being feasible to maintain).

One possible challenge this creates is that it makes it even more work for cht-conf to understand which of the forms are generated from templates (and so need to have CONTACT_TYPE and CONTACT_NAME replaced in the xml). I think we could maybe simplify things a lot here by just also generating (or updating if it already exists) a .properties.json for each of the generated forms where we can make sure the CONTACT_TYPE and CONTACT_NAME values are set in the placeholder_vars object. Then, when we go to replace the form placeholders, we do not need any custom logic for generated forms.

One side-point that I think we should do either way is that IMHO we should change the logic around replacing existing copies of the generated xlsx files. With the place-types.json, when we generate an xlsx file from the template, it is not saved if a file with the same name already exists on the disk. (I assume the original intent was that someone might have edited the existing xlsx and we didn't want to blow that away when regenerating from the template. 🤷) Regardless, that behavior seems surprising and confusing to me. When converting from xlsx > xml we always blow away any existing xml content. Why not do the same for the xlsx? Also, it just makes no sense that I can run the convert with the template and generate the xlsx files. Then I update the template and run the convert again, but I do not get new versions of the xlsx files... (And if you really want to customize/keep the generated xlsx, then just remove it from the template config...)

I think we need to leave things as they are for the place-types.json config so we don't create any breaking change (until we just totally remove support for that). But for the stuff generated from the new config, I think we can replace the existing xlsx files.

Dynamic placeholder variables

I love this! I think there are a number of very interesting and powerful use-cases that this can enable both in contact and app forms! What I am not a big fan of is editing stringified xml data with Regex.... 💀 😭 I have slowly been trying to drag us away from that practice and back to a more sane world where we edit the xml Document via the standard apis and with the help of xpath queries. You can see it in action in all the handle- files I added to do some janky "massaging" of the form structure/content. In a perfect world, I would love to see us do this placeholder replacement in a similar handle- file. 🤔

The main advantage of not using regex against the whole string is that I think we could make it work without requiring the placeholders to be prefixed with __cht_var-. Instead we should be able to intelligently replace any configured value. The downside is that it is going to be more complex to do this against the xml Document. 😬

After poking around for awhile, I think the main steps for the conversion would look something like:

1. Get all the nodes named with the placeholder: //*[local-name()='PLACEHOLDER']
   • Rename these nodes (have to create a new node as a child of the parent, copy attributes/children, delete old node)
   • Should be able to scope this to just descendants of the primary instance node.
2. Get any appearance attributes that contain the placeholder as a "type": //@appearance[contains(., 'type-PLACEHOLDER')]
   • Update the appearance value to replace the placeholder.
   • This is a specific edge case to support dynamic contact types being used with the select-contact functionality.
3. Spin through all the attributes in the doc where the placeholder is being used in a path: //@*[contains(., '/PLACEHOLDER')])`
   • When we rename a node, we also need to update any path references to that node (either relative or absolute).
   • We can use some regex in the attribute value to make sure we only replace the placeholder when it exists in a path expression: /\/PLACEHOLDER($|[\s*@:/\[\]])/
4. Get any nodes who's value is the placeholder: //*[text()='PLACEHOLDER']
   • Update the text value for these nodes

I think that logic is pretty close to complete when it comes to doing what we need. I can't say I am 100% convinced the extra complexity is worth it just to avoid the prefix, but it does also let us avoid editing xml with regex AND it keeps things simpler for all consumers going forwards since you will not have to worry about using the prefix when authoring the forms...

---

Should we purge template sections that are not relevant for certain types/forms?

IMHO we should not try to do this in the current iteration.

---

One last thing, as we add the new functionality to replace all the existing place-types.json, PLACE_TYPE, and PLACE_NAME stuff it would be convenient if we can handle that legacy functionality via the new logic. However, if that is going to add significant complexity (and lots of branching cases) my vote would be to leave the the existing place-types.json, PLACE_TYPE, and PLACE_NAME functionality as it is. We deprecate it, but don't try to refactor it. We can remove it in the next major version.

[ChinHairSaintClair]

No stress @jkuester, I've seen some of the PR assignment requests pouring in. Can only imagine what that onslaught of reviews must be like. My turn to apologies for the late response, I was away for a bit and then had to catch up with some of our instance's fixes.

Template content separation (sub dir):

I agree on having a sub directory contain all the new template related forms config. The separation makes sense and it'll also make sure that we don't accidentally process templates as normal forms (ignore everything in ./templates). This'll address item one in PR description question section.

.properties (template_contact_types) vs contact-types.json:

I think, while it differs a little from the general form setup, there's less cognitive load, better maintainability, and simpler for the code to consume when keeping the template gen config in the contact-types.json file. Similar to the base_setting.json it's easier to set up, and refer back to, which contacts are linked which generated forms. If there are any gaps or overlap. All in one place, rather than spread between files. Additionally, since our template related files will now be in a sub directory, the purpose of the contact-type.json file is a bit clearer. This file is the template creation driver, the rest of the content is used to meet the need. Checks can be put in place to ensure if ./template/ exists the contact-types.json has to exist as well, else throw.

Support template .properties files:

The current impl does not consider .properties files as part of the template content. It should be trivial to add that functionality. The form names and property file names are the same save for the suffix.

Generated form behaviour (override behaviour):

I agree, the behaviour where updating templates do not update the generated files automatically also confused me. As you've mentioned, it does seem the original intent was to protect manually created/updated forms. In the original PR code, I tried to say aligned with what was already there. I thought there must've been a good reason for that impl, perhaps not every adopter uses version control and loss of changes could be a real problem. Manually deleting a generated form is a clear indication that a new one is required. That said, your point on removing the entry signalling form generation for a specific contact also makes sense. The one is just more of an opt-in approach, while the other is an opt-out. While both will require the user to remember to do something, the one is potentially destructive while the other one is more of an inconvenience. Just playing devil's advocate. You're definitely better placed to speak to about how config is set up in the CHT community, so I'll go with whatever you decide is best.

Tracking generated files (CONTACT_NAME / CONTACT_TYPE):

IIUC this is maybe motivated by the assumption that only templates will need to have placeholder vars. That may not be the case, so the replacement function needs to run over every form. Generated forms already have their file names set, perhaps it makes sense to also set the form_id, as they need to match. In manual files the user can just set the placeholder themselves if need be. Sorry if I'm missing something.

Regex vs xml document:

Yeah, that makes sense. It especially hits home as I went with this regex approach after creating the variable restriction PR that uses the XML document code you're referring to. In a handle file no less.
You're right though, it is quite a bit more complex, so do appreciate the outlined steps to try and ease the load. That said, I do need to balance the time spent on these contributions against priorities in our space. I'll have a stab at it, but in the meantime can I ask that we try and get the var restriction PR in? It's much less complex.

---

Thank you for the heads up. Famous last words, but I think I'll be able to satisfy both the PLACE and CONTACT cases using the same code. Good to know I can back out if need be.

[ChinHairSaintClair]

@jkuester Tweaked the PR based on the discussions above:

• process contact templates from a templates subdir
• moved variable replacement to the convert folder as a handle- file

However, the PR still uses regex based variable replacement and the __cht_var- prefix. I'm also not the biggest fan of the prefix, but it does give us a deterministic lookup point and makes it possible to check whether any variables were missed during replacement (checkForStragglers).

The prefix may also help avoid ambiguity around what will actually be treated as a dynamic placeholder inside the form.

I have also left the overwrite as-is pending the discussion above.