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

FINERACT-2081: Fetch configurations by name #4057

Conversation

adamsaghy
Copy link
Contributor

Description

Describe the changes made and why they were made.

Ignore if these details are present on the associated Apache Fineract JIRA ticket.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Write the commit message as per https://github.com/apache/fineract/#pull-requests

  • Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.

  • Create/update unit or integration tests for verifying the changes made.

  • Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.

  • Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes

  • Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

@adamsaghy adamsaghy force-pushed the FINERACT-2081/fetch_configurations_by_name branch 4 times, most recently from ce6aa1c to db4740f Compare September 13, 2024 22:31
@@ -658,14 +658,17 @@ protected void verifyRepaymentSchedule(Long loanId, Installment... installments)

protected void runAt(String date, Runnable runnable) {
try {
GlobalConfigurationHelper.updateEnabledFlagForGlobalConfiguration(requestSpec, responseSpec, 42, true);
GlobalConfigurationHelper.updateIsBusinessDateEnabled(requestSpec, responseSpec, TRUE);
// globalConfigurationHelper.updateGlobalConfiguration("is-interest-to-be-recovered-first-when-greater-than-emi",
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch!

GlobalConfigurationHelper.updateEnabledFlagForGlobalConfiguration(requestSpec, responseSpec, 42, false);
globalConfigurationHelper.updateGlobalConfiguration("enable_business_date",
new PutGlobalConfigurationsRequest().enabled(false));
// globalConfigurationHelper.updateGlobalConfiguration("is-interest-to-be-recovered-first-when-greater-than-emi",
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment.

@@ -113,12 +110,14 @@ public void setup() {
this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
this.datatableHelper = new DatatableHelper(this.requestSpec, this.responseSpec);
GlobalConfigurationHelper.updateIsAutomaticExternalIdGenerationEnabled(this.requestSpec, this.responseSpec, true);
globalConfigurationHelper.updateGlobalConfiguration("enable-auto-generated-external-id",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these names should go into a class and have these as constants. Can't we do it in fineract-provider and use it from there?


// TODO: Would be better to support string based identifier in Commands and resolve the entity by name in the
// service
final GlobalConfigurationPropertyData configurationData = this.readPlatformService.retrieveGlobalConfiguration(configName);
Copy link
Contributor

Choose a reason for hiding this comment

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

There could be a concurrency issue here since the retrieval and the update are not running in the same transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but it is not an issue. We just resolve the id... that is not changing...

// service
final GlobalConfigurationPropertyData configurationData = this.readPlatformService.retrieveGlobalConfiguration(configName);

final CommandWrapper commandRequest = new CommandWrapperBuilder() //
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say we should write a new service method that can handle the update directly based on the name, not by the ID.

Copy link
Contributor Author

@adamsaghy adamsaghy Sep 17, 2024

Choose a reason for hiding this comment

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

Agree, hence the TODO comment above. But i dont think that should be part of this PR.

@adamsaghy adamsaghy force-pushed the FINERACT-2081/fetch_configurations_by_name branch 2 times, most recently from 34e7cea to 87215ae Compare September 25, 2024 15:14
@adamsaghy
Copy link
Contributor Author

adamsaghy commented Sep 25, 2024

@galovics I have done the requested changes, except the one to enhance the command service to accept the string based identifier. I think for now we are on the safe-side to resolve the id first and use that. There is no concurrency issue or any other as id cannot be changed. It is planned to enhance the command service to accept string based identifier, but that should be out of scope of this enhancement.

@adamsaghy adamsaghy force-pushed the FINERACT-2081/fetch_configurations_by_name branch from 87215ae to be1e0b2 Compare October 1, 2024 13:10
@adamsaghy adamsaghy force-pushed the FINERACT-2081/fetch_configurations_by_name branch from be1e0b2 to 6c3a986 Compare October 1, 2024 13:48
@adamsaghy adamsaghy merged commit 39d95f3 into apache:develop Oct 1, 2024
9 checks passed
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.

3 participants