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

Need an option to filter provisioning entries by date, Fineract-462 #1113

Closed
wants to merge 3 commits into from
Closed

Need an option to filter provisioning entries by date, Fineract-462 #1113

wants to merge 3 commits into from

Conversation

Charulathakarthikeyan
Copy link

@Charulathakarthikeyan Charulathakarthikeyan commented Jun 25, 2020 •
edited by vorburger

FINERACT-462

Description

Describe the changes made and why they were made. Ignore if these details are present on the associated Jira ticket

Checklist

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

Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide

@thesmallstar
Copy link
Member

Failed due to spotless errors, most probably you can fix them with
./gradlew spotlessApply
If not,
Maybe this helps: https://github.com/apache/fineract#checkstyle-and-spotless

@awasum
Copy link
Contributor

awasum commented Jul 4, 2020

Please @Charulathakarthikeyan , will you like to fix conflicts ? and ensure the Travis CI passes?

awasum
awasum reviewed Jul 4, 2020
View reviewed changes
...che/fineract/accounting/provisioning/service/ProvisioningEntriesReadPlatformServiceImpl.java
Date filterdate = null;
if (filterDate != null) {
try {
SimpleDateFormat format = new SimpleDateFormat("dd MMMM yyyy", Locale.ENGLISH);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using Locale.ENGLISH? What if the user sent a different language? I think there is a locale param been sent on almost all request in Fineract..Maybe use that?

awasum
awasum reviewed Jul 4, 2020
View reviewed changes
...ain/java/org/apache/fineract/accounting/provisioning/api/ProvisioningEntriesApiResource.java Outdated
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.Parameter;
import io.swagger.v3.oas.annotations.media.ArraySchema;
import io.swagger.v3.oas.annotations.media.Content;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure all of these changes are in this PR because of the this feature or it was added by mistake from another commit?

@Charulathakarthikeyan
Copy link
Author

Charulathakarthikeyan commented Jul 6, 2020 via email

@vorburger
Copy link
Member

@Charulathakarthikeyan we can't skip out commits in your PR for you.. You have to do this, if you would like your contribution to get code reviewed and merged.

@Charulathakarthikeyan
Copy link
Author

Charulathakarthikeyan commented Jul 14, 2020 via email

vorburger
vorburger reviewed Jul 23, 2020
View reviewed changes
...che/fineract/accounting/provisioning/service/ProvisioningEntriesReadPlatformServiceImpl.java
SimpleDateFormat format = new SimpleDateFormat("dd MMMM yyyy", Locale.ENGLISH);
filterdate = format.parse(filterDate);
} catch (ParseException e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

NOK, please follow https://github.com/apache/fineract/#error-handling-guidelines (use LOG.error("retrieveAllProvisioningEntriesByDate() bad filterDate: {}", filterDate, e);)

@edcable
Copy link
Contributor

edcable commented Aug 13, 2020

@Charulathakarthikeyan Have you been able to address the merge conflicts? I sent an email to your colleagues as well.

@Charulathakarthikeyan
Copy link
Author

Charulathakarthikeyan commented Aug 18, 2020 via email

@github-actions
Copy link

This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.

@github-actions github-actions bot added the stale label Sep 19, 2020
@vorburger
Copy link
Member

@Charulathakarthikeyan are you still interested in getting this merged? You have to git rebase it, resolve merge conflicts, and make sure that it passes the build on Travis CI. We won't be able to do this for you, so you have to follow up on this if you want us to review and merge. The pill request will automatically get closed if you continue to ignore it.

@Charulathakarthikeyan
Copy link
Author

Charulathakarthikeyan commented Sep 23, 2020 via email

@vorburger
Copy link
Member

I recommitted this code with new id contactus@habile.in to avoid this issue. And also I made PR using that id .

I'm not sure what you mean - which PR, where? I don't see any from you on https://github.com/apache/fineract/pulls.

tonic889 pushed a commit to tonic889/incubator-fineract that referenced this pull request Sep 28, 2020
per fineract issue apache#1113 and community-app issue apache#2225, a…
1ba0031 
…dd a validation error when a user attempts a recovery repayment which is greater than the total written off for the loan
tonic889 pushed a commit to tonic889/incubator-fineract that referenced this pull request Sep 28, 2020
per fineract issue apache#1113 and community-app issue apache#2225, a…
f112956 
…dd a validation error when a user attempts a recovery repayment which is greater than the total written off for the loan
@github-actions github-actions bot closed this Oct 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vorburger vorburger vorburger left review comments

@awasum awasum awasum left review comments

Assignees
No one assigned
Labels
stale
Projects
None yet
Milestone
No milestone
5 participants
@Charulathakarthikeyan @thesmallstar @awasum @vorburger @edcable