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

Jump to bottom

FINERACT-1926: Asset Externalization on Fineract #3350

Merged
merged 1 commit into from
Sep 1, 2023
Merged

FINERACT-1926: Asset Externalization on Fineract #3350

merged 1 commit into from
Sep 1, 2023

Conversation

josehernandezfintecheandomx
Copy link
Contributor

@josehernandezfintecheandomx josehernandezfintecheandomx commented Aug 1, 2023 •
edited
Loading

Description

API for Asset Externalization Feature, in order to analyze and work on the asset external owner details, I need to be able to view filer and search the AEO

FINERACT-1926

Screenshot 2023-08-03 at 21 14 55

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
Copy link
Contributor

@josehernandezfintecheandomx Please add proper FINERACT story number into the PR title and please amend your commit as well as that one should contain the relevant FINERACT story number as well!

adamsaghy
adamsaghy requested changes Aug 2, 2023
View reviewed changes
Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

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

Please see my comments, questions!
Also please cover this functionality with tests!

@adamsaghy
Copy link
Contributor

@josehernandezfintecheandomx please fix the compilation errors!

Execution failed for task ':fineract-investor:compileTestJava'.

> Compilation failed; see the compiler error output for details.

adamsaghy
adamsaghy requested changes Aug 4, 2023
View reviewed changes
Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

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

Please see my comments!

@josehernandezfintecheandomx josehernandezfintecheandomx force-pushed the enhancement/inverstor_search branch 2 times, most recently from 109a516 to c252496 Compare August 8, 2023 12:08
@adamsaghy
Copy link
Contributor

@josehernandezfintecheandomx Please add FINERACT-XXXX story number into the PR and into the commit!

@josehernandezfintecheandomx josehernandezfintecheandomx force-pushed the enhancement/inverstor_search branch 2 times, most recently from 2bcbc60 to 056df58 Compare August 10, 2023 07:59
@josehernandezfintecheandomx josehernandezfintecheandomx changed the title API for Asset Externalization Feature FINERACT-1526: Asset Externalization on Fineract Aug 10, 2023
@josehernandezfintecheandomx josehernandezfintecheandomx changed the title FINERACT-1526: Asset Externalization on Fineract FINERACT-1926: Asset Externalization on Fineract Aug 10, 2023
adamsaghy
adamsaghy reviewed Aug 11, 2023
View reviewed changes
...or/src/main/java/org/apache/fineract/investor/api/ExternalAssetOwnersApiResourceSwagger.java Outdated
public String text;
}

@Schema(description = "GetSearchExternalAssetOwnerTransferResponse")
Copy link
Contributor

Choose a reason for hiding this comment

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

We dont need this. Swagger can automatically generate this. See ExternalAssetOwnersApiResource.getTransfers

adamsaghy
adamsaghy reviewed Aug 11, 2023
View reviewed changes
...or/src/main/java/org/apache/fineract/investor/api/ExternalAssetOwnersApiResourceSwagger.java Outdated
@@ -152,4 +152,57 @@ static final class ExternalAssetOwnerTransferChangesData {
public String purchasePriceRatio;
}
}

@Schema(description = "GetExternalAssetOwnerSearchRequest")
public static final class GetExternalAssetOwnerSearchRequest {
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 not used. not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! removed

adamsaghy
adamsaghy reviewed Aug 11, 2023
View reviewed changes
...or/src/main/java/org/apache/fineract/investor/api/ExternalAssetOwnersApiResourceSwagger.java

@Schema(example = "[2023, 5, 1]")
public LocalDate effectiveFrom;
@Schema(description = "ExternalTransferResponse")
Copy link
Contributor

Choose a reason for hiding this comment

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

you might wanna revert this rearrangement changes.

adamsaghy
adamsaghy reviewed Aug 11, 2023
View reviewed changes
...c/main/java/org/apache/fineract/investor/service/search/ExternalAssetOwnerSearchService.java Outdated
private void validateTextSearchRequest(PagedRequest<ExternalAssetOwnerSearchRequest> searchRequest) {
Objects.requireNonNull(searchRequest, "searchRequest must not be null");

platformUserRightsContext.isAuthenticated();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this authentication check to API layer!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DOne, !t was moved to the API layer

adamsaghy
adamsaghy requested changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

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

Please see my comments!

@josehernandezfintecheandomx josehernandezfintecheandomx force-pushed the enhancement/inverstor_search branch 2 times, most recently from b88555c to 19ee736 Compare August 15, 2023 04:43
josehernandezfintecheandomx
josehernandezfintecheandomx commented Aug 15, 2023
View reviewed changes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

...or/src/main/java/org/apache/fineract/investor/api/ExternalAssetOwnersApiResourceSwagger.java Outdated
@@ -152,4 +152,57 @@ static final class ExternalAssetOwnerTransferChangesData {
public String purchasePriceRatio;
}
}

@Schema(description = "GetExternalAssetOwnerSearchRequest")
public static final class GetExternalAssetOwnerSearchRequest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! removed

@adamsaghy
Copy link
Contributor

@josehernandezfintecheandomx Please let me know when all the comments are replied / resolved. Thanks! ;)

@josehernandezfintecheandomx josehernandezfintecheandomx force-pushed the enhancement/inverstor_search branch 2 times, most recently from 843c09d to bfea74d Compare August 21, 2023 23:47
adamsaghy
adamsaghy requested changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

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

  1. HttpMessageNotReadableErrorController is okay, however i reckon we need an erorr handler to handle DateTimeParseException and provide back a valid error message with a hint about the problem. Something like this:
    HTTP 400 - Bad request
    Error message: Invalid date was provided. The date must match with the “yyyy-MMMM-dd” date format.
    Can you please write an ErrorMapper for the DateTimeParseException?

  2. ExternalAssetOwnersApiResourceSwagger.java -> Please undo the changes, they are not required. , Spring will generate automatically the swagger

  3. ExternalAssetOwnersApiResource.java -> searchInvestorData method @ApiResponses annotation can be removed as well

  4. Please enhance your tests cases:

  • assertNotNull(response); is not enough… Please write assertion for each value and check the value is matching with the expectations
  • Test case when submitted on from date of search request date format is invalid -> I dont see this test case to be covered!

@josehernandezfintecheandomx
Copy link
Contributor Author

  1. HttpMessageNotReadableErrorController is okay, however i reckon we need an erorr handler to handle DateTimeParseException and provide back a valid error message with a hint about the problem. Something like this:
    HTTP 400 - Bad request
    Error message: Invalid date was provided. The date must match with the “yyyy-MMMM-dd” date format.
    Can you please write an ErrorMapper for the DateTimeParseException?
  2. ExternalAssetOwnersApiResourceSwagger.java -> Please undo the changes, they are not required. , Spring will generate automatically the swagger
  3. ExternalAssetOwnersApiResource.java -> searchInvestorData method @ApiResponses annotation can be removed as well
  4. Please enhance your tests cases:
  • assertNotNull(response); is not enough… Please write assertion for each value and check the value is matching with the expectations
  • Test case when submitted on from date of search request date format is invalid -> I dont see this test case to be covered!

Done those changes and PR updated please review it, Thanls

adamsaghy
adamsaghy reviewed Sep 1, 2023
View reviewed changes
...he/fineract/integrationtests/investor/externalassetowner/ExternalAssetOwnerTransferTest.java
import org.junit.jupiter.api.BeforeAll;

@Slf4j
public class ExternalAssetOwnerTransferTest {
Copy link
Contributor

@adamsaghy adamsaghy Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This class have 0 tests...

Probably the class name is not the best...

adamsaghy
adamsaghy approved these changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

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

LGTM

@adamsaghy adamsaghy merged commit 4a2aeff into apache:develop Sep 1, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamsaghy adamsaghy adamsaghy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@josehernandezfintecheandomx @adamsaghy