-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
FINERACT-2081: Fetch configurations by name #4057
Conversation
ce6aa1c
to
db4740f
Compare
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment.
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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() // |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
...ava/org/apache/fineract/infrastructure/configuration/api/GlobalConfigurationApiResource.java
Show resolved
Hide resolved
34e7cea
to
87215ae
Compare
@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. |
...act/integrationtests/investor/externalassetowner/InitiateExternalAssetOwnerTransferTest.java
Show resolved
Hide resolved
...org/apache/fineract/infrastructure/configuration/exception/GlobalConfigurationException.java
Outdated
Show resolved
Hide resolved
87215ae
to
be1e0b2
Compare
be1e0b2
to
6c3a986
Compare
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.