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

Support export files on partners #153

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

michieldegezelle
Copy link
Contributor

@michieldegezelle michieldegezelle commented Dec 4, 2024

Description

Update to make sure that export files can be handled (imported & updated) on partners as well.
Note that the create commands were not yet added, since we have a separate ticket for that to cover all template types.

Fixes # [Liquid toolchain] - CLI - Support export files on Partners

Type of change

  • Bug fix
  • New feature
  • Breaking change

Checklist

  • README updated (if needed)
  • Version updated (if needed)
  • Documentation updated (if needed)

@michieldegezelle michieldegezelle linked an issue Dec 4, 2024 that may be closed by this pull request
@michieldegezelle michieldegezelle force-pushed the 133-cli-support-export-files-on-partners branch from b15623a to 9a0ea03 Compare December 4, 2024 12:45
bin/cli.js Outdated Show resolved Hide resolved
@michieldegezelle michieldegezelle force-pushed the 133-cli-support-export-files-on-partners branch from 750a6d5 to 8a359aa Compare December 5, 2024 12:56
@michieldegezelle michieldegezelle marked this pull request as ready for review December 5, 2024 13:06
@michieldegezelle michieldegezelle self-assigned this Dec 5, 2024
@delphine-demeulenaere delphine-demeulenaere force-pushed the 133-cli-support-export-files-on-partners branch from 8a359aa to a69adcc Compare December 5, 2024 13:07
@michieldegezelle michieldegezelle force-pushed the 133-cli-support-export-files-on-partners branch from a69adcc to 89bdcb9 Compare December 6, 2024 09:58
Copy link
Contributor

@AgustinSilverfin AgustinSilverfin left a comment

Choose a reason for hiding this comment

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

Updates look really good! 💯
I left some comments here and there.

About async/await: I guess we can keep it without await for now since save method is not async.

You will need to update the CLI version again. And as a simply rule, if we make an update like this, we bump the MINOR number. If we do a bug fix, we bump the PATCH
Considering that v{MAJOR}.{MINOR}.{PATCH}. https://semver.org/#summary

lib/cli/utils.js Show resolved Hide resolved
bin/cli.js Show resolved Hide resolved
bin/cli.js Outdated
@@ -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

bin/cli.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
lib/templates/exportFile.js Show resolved Hide resolved
lib/templates/exportFile.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@AgustinSilverfin AgustinSilverfin left a comment

Choose a reason for hiding this comment

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

I left only one comment regarding partners and update/create all commands to decide what we do.

Besides that, everything looks good for me. Which takes me to the next point. Did someone else from the team did a functional testing? if not, we should do that

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @BenjaminLangenakenSF , but I would prefer to have all this conditions on /bin/cli.js rather than index.js
I see this contrain related to the CLI commands & flags, so we add it there, while keeping the functionality 'clean'

If we ever support them, again we update CLI commands & flags.

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.

Hi @AgustinSilverfin

How would that look like? Something like this?
image

So adding a condition to check on the combination of those options and then a process.exit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.
Or even cleaner (to avoid repeating always the same and adding multiple lines) we create a helper method like cliUtils.checkUniqueOption(["handle", "all"], options) or cliUtils.checkDefaultFirm(options.firm, firmIdDefault) where we pass options

E.g. cliUtils.checkPartnerSupport(options) I believe the condition is options.all and options.type == partner, right?

Choose a reason for hiding this comment

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

Thanks for the explanation Rufo, added a fixup commit 😉
Looks ok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: Support export files on Partners
4 participants