Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rmayore opened this issue Jan 8, 2024 · 2 comments
Open

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

rmayore opened this issue Jan 8, 2024 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@rmayore
Copy link

rmayore commented Jan 8, 2024

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
Copy link

@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.js and 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
Copy link
Author

rmayore commented Jan 10, 2024

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

@Marina-Kenf Marina-Kenf added the bug Something isn't working label Jan 19, 2024
@inromualdo inromualdo self-assigned this Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants