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.

Sign up for GitHub

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

Support export files on partners #153

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
+1,010 −931
Open

Support export files on partners #153

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
12239e6
Activate options in documentation to read and update export files fro…
michieldegezelle Nov 15, 2024
6c42250
Adjust functions in index.js file
michieldegezelle Dec 4, 2024
a886199
update parameters documentation of export class
michieldegezelle Dec 4, 2024
8a8090e
Update all underlying dependencies + update CLI version
michieldegezelle Dec 4, 2024
89bdcb9
Fix issue where partner id is not stored after importing from partners
michieldegezelle Dec 4, 2024
8a889a0
fixup! Update all underlying dependencies + update CLI version
michieldegezelle Dec 9, 2024
b2d0e69
Remove partnerSupported parameter from runCommandChecks function
michieldegezelle Dec 9, 2024
c7b7778
Allow publishAll commands for partner as well
michieldegezelle Dec 9, 2024
47556ef
Improve all warning messages by referring to type and envId
michieldegezelle Dec 9, 2024
91db732
Move getTemplateId function to Utils and fetch it where necessary
michieldegezelle Dec 9, 2024
584a103
Update packages to make sure that tests are running
michieldegezelle Dec 9, 2024
f653750
Make sure that the PublishAll functions do not work for partners
michieldegezelle Dec 11, 2024
3a7a97c
Add error text in case of trying update all templates in partner env
BenjaminLangenakenSF Dec 18, 2024
3d20f38
Merge branch 'main' into 133-cli-support-export-files-on-partners
michieldegezelle Dec 19, 2024
f58ce4a
fixup! Add error text in case of trying update all templates in partn…
BenjaminLangenakenSF Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 23 additions & 14 deletions bin/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ program
.command("import-export-file")
.description("Import export file templates")
.option("-f, --firm <firm-id>", "Specify the firm to be used", firmIdDefault)
.option("-p, --partner <partner-id>", "Specify the partner to be used")
.option("-n, --name <name>", "Import a specific export file by name")
.option("-i, --id <id>", "Import a specific export file by id")
.option("-a, --all", "Import all existing export files")
Expand All @@ -158,7 +159,7 @@ program
["name", "id", "all", "existing"],
options,
firmIdDefault,
false
delphine-demeulenaere marked this conversation as resolved.
Show resolved Hide resolved
true // Partner supported
);

if (options.name) {
Expand All @@ -168,11 +169,15 @@ program
options.name
);
} else if (options.id) {
toolkit.fetchExportFileById(settings.type, settings.envId, options.id);
toolkit.fetchExportFileById(
settings.type,
settings.envId,
options.id
);
} else if (options.all) {
toolkit.fetchAllExportFiles(settings.type, settings.envId);
} else if (options.all) {
toolkit.fetchExistingExportFiles(options.firm);
AgustinSilverfin marked this conversation as resolved.
Show resolved Hide resolved
} else if (options.existing) {
toolkit.fetchExistingExportFiles(settings.type, settings.envId);
}
});

Expand All @@ -181,6 +186,7 @@ program
.command("update-export-file")
.description("Update an existing export file template")
.option("-f, --firm <firm-id>", "Specify the firm to be used", firmIdDefault)
.option("-p, --partner <partner-id>", "Specify the partner to be used")
.option("-n, --name <name>", "Specify the export file to be used (mandatory)")
.option("-a, --all", "Update all export files")
.option(
Expand All @@ -194,8 +200,8 @@ program
["name", "all"],
options,
firmIdDefault,
false,
true // Message required (added for later)
true, // Partner supported
true // Message required
);

if (options.name) {
Expand All @@ -207,7 +213,7 @@ program
);
} else if (options.all) {
toolkit.publishAllExportFiles(
settings.type,
"firm",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inline all publicAll... commands. But I'm not sure why we don't support that for Partners tbh.
Maybe something to tackle together with the create- commands ticket?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the error when running silverfin update-<any-template-type> --all --partner <id> --message "Update all is not clear.

It is not saying it is not supported, it just says missing IDs.

I would vote for change it now, replace "firm" with type and support it for every update- command. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the index.js file, we seemed to use the type already in the underlying functions, so is it correct that I only needed to change the "firm" to settings.type (4 times -> Reconciliations, export files, account templates, shared parts).

Since we are supporting it, we won't have to adjust the error either, I assume?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I mentioned to remove the partner check, but later we decided to not support update-all and create-all on partners.

And I wonder if we should do that still in this PR, because if not we could still be releasing something not fully working?

keeping the fixed "firm" parameter is not going to prevent going through the entire process when you select partners -> it will fail because of the wrong id (the firm id is not the partner id)

But I think we should do the right thing now, and implement the block for Partners in update/create all in here.
So a way to check if the option is partner and the command is update or create, and the flag is set to all. We block the process, show an error message, and don't run any further methods.

What do you think

settings.envId,
options.message
);
Expand Down Expand Up @@ -237,7 +243,7 @@ program
if (options.name) {
toolkit.newExportFile("firm", options.firm, options.name);
} else if (options.all) {
toolkit.newAllExportFiles("firm", options.firm, options.name);
michieldegezelle marked this conversation as resolved.
Show resolved Hide resolved
toolkit.newAllExportFiles("firm", options.firm);
}
});

Expand Down Expand Up @@ -488,12 +494,11 @@ program
)
.option("--yes", "Skip the prompt confirmation (optional)")
.action((options) => {
const partnerSupported = options.exportFile ? false : true;
const settings = runCommandChecks(
["sharedPart", "all"],
options,
firmIdDefault,
partnerSupported
true // Partner supported
);

if (options.sharedPart) {
Expand Down Expand Up @@ -560,12 +565,11 @@ program
)
.option("--yes", "Skip the prompt confirmation (optional)")
.action((options) => {
const partnerSupported = options.exportFile ? false : true;
const settings = runCommandChecks(
["handle", "exportFile", "accountTemplate"],
options,
firmIdDefault,
partnerSupported
true // Partner supported
);

if (options.handle) {
Expand Down Expand Up @@ -886,6 +890,7 @@ program
.command("get-export-file-id")
.description("Fetch the ID of an export file from Silverfin")
.option("-f, --firm <firm-id>", "Specify the firm to be used", firmIdDefault)
.option("-p, --partner <partner-id>", "Specify the partner to be used")
.option("-n, --name <name>", "Fetch the export file ID by name")
.option("-a, --all", "Fetch the ID for every export file")
.option("--yes", "Skip the prompt confirmation (optional)")
Expand All @@ -894,7 +899,7 @@ program
["name", "all"],
options,
firmIdDefault,
false
delphine-demeulenaere marked this conversation as resolved.
Show resolved Hide resolved
true // Partner supported
);

if (options.name) {
Expand All @@ -905,7 +910,11 @@ program
options.name
);
} else if (options.all) {
toolkit.getAllTemplatesId(settings.type, settings.envId, "exportFile");
toolkit.getAllTemplatesId(
settings.type,
settings.envId,
"exportFile"
);
}
});

Expand Down
70 changes: 49 additions & 21 deletions index.js
View file
Open in desktop
AgustinSilverfin marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,13 @@ async function newAllReconciliations(type, firmId) {
async function fetchExportFileByName(type, envId, name) {
try {
const template = await SF.findExportFileByName(type, envId, name);

if (!template) {
consola.error(`Export file "${name}" wasn't found`);
process.exit(1);
}
const saved = await ExportFile.save(type, envId, template);

const saved = ExportFile.save(type, envId, template);
if (saved) {
consola.success(`Export file "${name}" imported`);
}
Expand All @@ -270,8 +272,18 @@ async function fetchExportFileById(type, envId, id) {
process.exit(1);
}

ExportFile.save(type, envId, template);
consola.success(`Export file "${template.name}" imported`);
const saved = ExportFile.save(type, envId, template);
if (saved) {
consola.success(
`Export file "${template.name}" imported from ${type} ${envId}`
);
}

return {
type,
envId,
template,
}
} catch (error) {
consola.error(error);
process.exit(1);
Expand All @@ -280,25 +292,44 @@ async function fetchExportFileById(type, envId, id) {

async function fetchAllExportFiles(type, envId, page = 1) {
const templates = await SF.readExportFiles(type, envId, page);

if (templates.length == 0) {
if (page == 1) {
consola.error(`No export files found in firm ${firmId}`);
consola.error("No export files found in firm");
michieldegezelle marked this conversation as resolved.
Show resolved Hide resolved
}
return;
}

templates.forEach(async (template) => {
fetchExportFileById(type, envId, template.id);
const saved = ExportFile.save(type, envId, template.id);
michieldegezelle marked this conversation as resolved.
Show resolved Hide resolved

if (saved) {
consola.success(`Export file "${template.name}" imported`);
}
});
fetchAllExportFiles(type, envId, page + 1);
}

async function fetchExistingExportFiles(firmId) {
async function fetchExistingExportFiles(type, envId) {
const templates = fsUtils.getAllTemplatesOfAType("exportFile");
if (!templates) return;
if (!templates) {
consola.warn("No export files found");
michieldegezelle marked this conversation as resolved.
Show resolved Hide resolved
return;
}

templates.forEach(async (name) => {
const templateConfig = fsUtils.readConfig("exportFile", name);
if (!templateConfig || !templateConfig.id[firmId]) return;
await fetchExportFileById(firmId, templateConfig.id[firmId]);
let templateId =
type == "firm"
? templateConfig?.id?.[envId]
: templateConfig?.partner_id?.[envId]
michieldegezelle marked this conversation as resolved.
Show resolved Hide resolved

if (!templateId) {
errorUtils.missingExportFileId(name);
return false;
}

await fetchExportFileById(type, envId, templateId);
});
}

Expand Down Expand Up @@ -383,6 +414,7 @@ async function newExportFile(type, firmId, name) {
const template = await ExportFile.read(name);
if (!template) return;
template.version_comment = "Created through the Silverfin CLI";

const response = await SF.createExportFile(firmId, template);

// Store new id
Expand Down Expand Up @@ -461,7 +493,11 @@ async function fetchAllAccountTemplates(type, envId, page = 1) {

async function fetchExistingAccountTemplates(type, envId) {
const templates = fsUtils.getAllTemplatesOfAType("accountTemplate");
if (!templates) return;
if (!templates) {
consola.warn("No account templates found");
michieldegezelle marked this conversation as resolved.
Show resolved Hide resolved
return;
}

templates.forEach(async (name) => {
const templateConfig = fsUtils.readConfig("accountTemplate", name);
let templateId =
Expand Down Expand Up @@ -506,7 +542,7 @@ async function publishAccountTemplateByName(

consola.debug(`Updating account template ${name}...`);

const template = AccountTemplate.read(name);
const template = await AccountTemplate.read(name);
if (!template) return;

// Add API-only required fields
Expand Down Expand Up @@ -782,8 +818,8 @@ async function newAllSharedParts(type, firmId) {
}

/** This function adds a shared part to a template. It will make a POST request to the API. If the ID of one of the templates is missing, it will try to fetch it first by making a GET request. In case of success, it will store the details in the corresponding config files.
*
* @param {Number} firmId
* @param {String} type - Options: `firm` or `partner`
michieldegezelle marked this conversation as resolved.
Show resolved Hide resolved
* @param {Number} envId
* @param {string} sharedPartName
* @param {string} templateHandle
* @param {string} templateType has to be either `reconciliationText`, `exportFile`or `accountTemplate`
Expand All @@ -797,14 +833,6 @@ async function addSharedPart(
templateType
) {
try {
// Add a check for export files that are not supported
if (type == "partner" && templateType == "exportFile") {
consola.warn(
"Adding shared parts to export files on partner is not supported. Skipping."
);
return false;
}

michieldegezelle marked this conversation as resolved.
Show resolved Hide resolved
let templateConfig = await fsUtils.readConfig(templateType, templateHandle);
let sharedPartConfig = await fsUtils.readConfig(
"sharedPart",
Expand Down
8 changes: 1 addition & 7 deletions lib/cli/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,7 @@ function runCommandChecks(
messageRequired = false
) {
if (options.partner) {
// To delete once export files are supported
if (!partnerSupported) {
consola.error(
`The option "--partner" is not supported when using "--export-file"`
);
process.exit(1);
} else if (messageRequired && !options.message) {
AgustinSilverfin marked this conversation as resolved.
Show resolved Hide resolved
if (messageRequired && !options.message) {
consola.error(
`Message required when updating a partner template. Please use "--message"`
);
Expand Down
6 changes: 2 additions & 4 deletions lib/templates/exportFile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ class ExportFile {

/**
* Process the response provided by the Silverfin API and store every detail in its corresponding file (liquid files, config file, etc)
* @param {number} firmId
* @param {string} type firm or partner
* @param {number} envId The id of the firm or partner environment where the template is going to be imported from
* @param {object} template
*/
static save(type, envId, template) {
AgustinSilverfin marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -46,9 +47,6 @@ class ExportFile {
...existingConfig?.partner_id,
...addNewId(type, "partner", envId, template),
},
partner_id: {
...existingConfig?.partner_id,
},
michieldegezelle marked this conversation as resolved.
Show resolved Hide resolved
...configDetails,
};
fsUtils.writeConfig(this.TEMPLATE_TYPE, name, configContent);
Expand Down
Loading
Loading