MIG-85: implemented changes for partial migrations and fixed broken tests

[davideaquaro]

Unit tests will be added once the current approach is confirmed

[nahueld]

given that we are adding this option to the CLI too may be we can call it manifest instead of manifestObject to make it consistent with the manifest option in mdctl env export|import command , what do you think?

[davideaquaro]

hi @nahueld I had that one initially, but weirdly it was somehow causing issues when generating the final manifest, so I decided to adopt a different name to avoid this

[nahueld]

I'd add that this is only valid during export command, and it can only contain object instances, other org config objects can be exported through regular mdctl env export command

[davideaquaro]

hi @nahueld , I added that in the "Notes" section but it makes sense to have it as part of the explanation of this flag. Thanks for that

[nahueld]

IMO this function should be more of a responsibility of the study.js module, this will decoupled "parsing"/"reading" the manifest that is ultimately an option passed to the CLI command, the buildManifestAndDependencies should receive the manifest as a plain javascript object directly, validations on reading the file or performing a JSON.parse() can be done outside to fail first. what is your take on this?

[nahueld]

In the other hand this validation could be left here

 if (!manifestJSON.object || manifestJSON.object !== 'manifest') {
        throw Fault.create('kInvalidArgument', { reason: 'The argument is not a valid manifest' })
      }

So basically:

• reading/parsing to the upper layer (CLI)
• making sure that that json is a manifest looks ok in here

[davideaquaro]

That makes sense, I will move the function in study.js and make sure the manifest is ok in here. Thanks for the advise ;)

[nahueld]

I think this one can be improved slightly, overall it should work but we can make more secure

1. Notice that arrays are also considered objects so it will pick up the "objects" from the manifest if passed through, this should not happen but it may be a good idea to account for incorrect usage
2. I'd add part of this to validateManifest and may be rename it to validateAndCleanManifest

For example, inside validateAndCleanManifest:

return Object.keys(manifestJSON)
   .filter(key => availableObjectNames.includes(key))
   .reduce((acc,curr) => ({ ...acc, [curr]: manifestJSON[curr] })

The variable availableObjectNames can be this array that is somewhere else actually:

['c_study', 'c_task', 'c_visit_schedule', 'ec__document_template', 'c_group',
              'c_anchor_date_template', 'c_fault', 'c_dmweb_report', 'c_site', 'c_task_assignment', 'c_participant_schedule',
              'c_patient_flag', 'c_looker_integration_record', 'int__vendor_integration_record', 'int__model_mapping',
              'int__pipeline', 'orac__studies', 'orac__sites', 'orac__forms', 'orac__form_questions', 'orac__events']

In the end we should only support granular export of these

[nahueld]

Finally if you decide to go with the approach above, the calculation of ignoreKeys can be simplified to:

ignoreKeys = Object.keys(manifestJSON)
        .map(key => this.mapObjectNameToPlural(key))

[nahueld]

By applying the refactors mentioned above, this line would be:

manifestKeys = study ? this.availableObjectNames : Object.keys(manifestObject)

[nahueld]

Changes are good to go