medic / cht-stock-monitoring-workflow

GNU Affero General Public License v3.0
4 stars 2 forks source link

Add Support For The Old Configuration Style (Non-declarative). #33

Open rmayore opened 10 months ago

rmayore commented 10 months ago

We are actively exploring modularizing some of our CHT configs, so this package is a very good starting point for learning. Unfortunately, in some of our deployments we use the old config style (the non-declarative format, with the rules.nools.js file).

Describe the Issue

Initializing the package with the command npm exec -- cht-stock-monitoring-workflow init returns the following error message: Not a CHT app

I think this is because the main.js init script searches for an app_settings directory which is not present in the old-style configs.

Describe the improvement you'd like

Explore adding support for the old style configs. I say this with the realization that this package probably is completely incompatible with the old style configs. In that case, a note in the description that the configs have to be of declarative format will suffice.

Describe alternatives you've considered

None

SheilaAbby commented 10 months ago

@rmayore I have been playing around with the stock monitoring package source code to see if we can come up with possible workarounds to make the package compatible with the old-style configs.

2 major issues I noted. Issue no. 1 - the package is looking for an app settings file inside an app settings folder(declarative configs have a base settings file inside an app settings folder). Therefore, updating the specific package files(main.jsand src/common.js) to also check for an app_settings.json file not within an app_settings directory fixes issue no. 1.

main.js

function isChtApp() {
  const processDir = process.cwd();
  const formDir = path.join(processDir, 'forms');
  const baseSettingDir = path.join(processDir, 'app_settings');

  // Construct the paths to 'app_settings' file for apps without app_settings folder
  const appSettingsFilePath = path.join(processDir, 'app_settings.json');

  // Check if the 'app_settings' file exists and is a regular file
  const hasAppSettingsFile = fs.existsSync(appSettingsFilePath) && fs.statSync(appSettingsFilePath).isFile();

  if ((fs.existsSync(formDir) && fs.existsSync(baseSettingDir)) || (fs.existsSync(formDir) && hasAppSettingsFile) ) {
    return true;
  }
  return false;
}

src/common.js

function getAppSettings() {
  try {
    const processDir = process.cwd();
    // const baseSettingFile = path.join(processDir, 'app_settings', 'base_settings.json');

    const appSettingsPath = path.join(processDir, 'app_settings.json');
    const baseSettingsPath = path.join(processDir, 'app_settings', 'base_settings.json');

    const baseSettingFile = fs.existsSync(baseSettingsPath) ? baseSettingsPath : appSettingsPath;

    const rawSettings = fs.readFileSync(baseSettingFile, {
      encoding: 'utf-8'
    });
    const settings = JSON.parse(rawSettings);
    settings.locales = settings.locales.filter((locale) => locale.disabled !== true);
    return settings;
  } catch (err) {
    console.log('Error reading app settings', err);
    return null;
  }
}

Issue no. 2 - locales definition in the app_settings.json file. The solution here might be to update the definition here from "locales": null to "locales": [{ "code": "en", "name": "English"}] our apps default lang is English so I think it makes sense to define the locales this way instead of setting it to null.

with the above updates npm exec -- cht-stock-monitoring-workflow init runs successfully

rmayore commented 10 months ago

Thanks for this @SheilaAbby... now to see if we can have the stock monitoring worflows/tasks/targets/cs deployed to an instance alongside the other non-declarative configs