-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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-2079: Rectify query for cashier transactions #3881
base: develop
Are you sure you want to change the base?
Conversation
@wkigenyi Can you please write an integration test which calls the cashier transactions API and calls the cashier transactions with summary API? This way we can be sure, your fix is working and avoid regression in the future? |
@adamsaghy this commit basically replaces the wrong table column in the sql query with the right one. Please guide me on the best way to write this this test. |
First of all thank you for the fix. Like I said in my earlier comment you should write an integration test which call the mentioned APIs and they are not failing (response is HTTP 200). This way we can make sure in the future if the query or the data model got changed we will know about it. You may find plenty example in the "integration-test" directory. Please let me know if you are facing any trouble! |
Thanks Adam, I do understand the importance of the test. I will look into the mentioned directory for examples. |
Hi I believe we dont really have too many test cases for cashier transactions, so i will try to give you some help here:
You can see examples in the LoanTransactionHelper class, example in line 738 |
Thanks Alot Adam. I will at this in next few days. I have a commitment away from my machine. |
Good Morning @adamsaghy. I tried to follow the example in LoanTransactionHelper, how to I run that test individually to see its results (either in intellij or gradlew)? |
could you help me on how to locate this directory, I am using mifos 23.12 and I have failed to locate the suggested file inorder to make the changes (appuser_id) |
@adamsaghy please have a look at this. |
hey @wkigenyi , I am using the mifos 23 not docker version, the one downloaded from sourceforge.net/projects/mifos/ |
@bravosty you can locate the file fineract-provider/src/main/java/org/apache/fineract/organisation/teller/service/TellerManagementReadPlatformServiceImpl.java and make the change made in this commit. |
[image: image.png]
this is the directory of the fineract-provider file in apache tomcat
directory
[image: image.png]
and this is what I find inside the directory, so I can not see the path you
told me to navigate to cause this is the tomcat, local machine installation
…On Mon, May 20, 2024 at 10:24 AM Wilfred Kigenyi ***@***.***> wrote:
@bravosty <https://github.com/bravosty> you can locate the file
fineract-provider/src/main/java/org/apache/fineract/organisation/teller/service/TellerManagementReadPlatformServiceImpl.java
and make the change made in this commit.
—
Reply to this email directly, view it on GitHub
<#3881 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A3E5KA3GALQ4XMK7WFUFNGDZDGQJDAVCNFSM6AAAAABHIBJQ4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZHAZTKNBUGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@bravosty this has to be done in the source code. https://github.com/apache/fineract |
how then will I add this change to the Mifos installation?
…On Mon, May 20, 2024 at 10:43 AM Wilfred Kigenyi ***@***.***> wrote:
@bravosty <https://github.com/bravosty> this has to be done in the source
code. https://github.com/apache/fineract
—
Reply to this email directly, view it on GitHub
<#3881 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A3E5KAZOBPAFWRXAMNKZM53ZDGSQ5AVCNFSM6AAAAABHIBJQ4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZHA3DIMBTG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@bravosty you would have to build your own jar/war. The alternative is to wait for for this PR to be merged and Fineract 1.10 released. |
Ok, thank you.
…On Mon, 20 May 2024, 11:28 am Wilfred Kigenyi, ***@***.***> wrote:
@bravosty <https://github.com/bravosty> you would have to build your own
jar/war. The alternative is to wait for for this PR to be merged and
Fineract 1.10 released.
—
Reply to this email directly, view it on GitHub
<#3881 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A3E5KA5CZNOVJS3Z4ZXOB33ZDGX2VAVCNFSM6AAAAABHIBJQ4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZHE2DCMRUHE>
.
You are receiving this because you were mentioned.Message 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.
LGTM
@@ -0,0 +1,54 @@ | |||
package org.apache.fineract.integrationtests.organization.teller; |
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.
Please add the proper licence header to the newly created files!
(You can copy from any existing file)
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.
- What went wrong:
You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
Execution failed for task ':rat'.
A failure occurred while executing org.nosphere.apache.rat.RatWork
Apache Rat audit failure - 2 unapproved licenses
See file:///home/runner/work/fineract/fineract/build/reports/rat/index.html
Please add the proper licence header to the newly created files!
(You can copy from any existing file)
@adamsaghy license added |
@wkigenyi Please kindly check the failing test case. If you want to test locally, you can run the fineract backend and then configure your intelliJ to run the test case with IntelliJ Runner: |
@adamsaghy I tried to run integration-tests with gradlew :integration-test:test and I was getting this error |
cf27dec
to
463b5dc
Compare
@adamsaghy I improved the test |
fcb818d
to
e9bb776
Compare
well done @wkigenyi, I am excited about the final fineract-provider.war file, I love the developments I am seeing, keep it up! |
@adamsaghy I need some help here. |
Sorry for causing so much trouble for you. In the swagger file you can change it and then the fineract-client will generate the proper response object. You can give a try to remove these annotations, so there will be no defined response object from the TellerApiResourceSwagger class and hopefully it will generate automatically the proper Response object the fineract-client. Alternatively you can change the annotations to point not as an ArraySchema, rather an (regular) schema (just remove the array parts) and change the But i would say it is just easier to remove those annotations and let Fineract automatically generate the proper Response object ;) |
@adamsaghy, done and with this PR I have discovered a lot about Fineract integration testing |
11e1b9f
to
ed7fd70
Compare
@adamsaghy I think I am very close |
@adamsaghy why are the last two checks here taking more than 24 hours ? |
Seems they stucked. I have restarted. I should be finished under 50 mins |
@adamsaghy I have 1/8 failing |
@wkigenyi Seems the cashier transaction API does not working with postgres database. Can you please take a look on it? It might requires different SQL for postgres database. You can find database dependent solution in the From the server logs it looks this exception:
|
@wkigenyi are you planning to fix the PostgreSQL version of the query? |
Yes, though I would have preferred to write something that is dB independent. I am just a bit stuck but will see it through. |
@wkigenyi I agree but it's a good strategy to first get it working and merged. Then as a step 2, you could try to change the query so it's DB independent. |
Okay. Will find time for this and resolve it. |
@wkigenyi any update here? |
I have been a bit swamped. I have time this weekend to work on it. |
@wkigenyi superb, as soon as you're ready, I can review it. |
There are two ways to create a cashier
The second option does not seem to be supported anymore because when you provide values for fields when creating the cashier you get an error that they are not supported, when you leave them out in the request without setting isFullDay to true you will get an error message saying startTime is required. Should I therefore assume that these fields are no longer used when creating the cashier and rewrite the query without considering them? |
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.