You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2022/09/28 09:13:52 UTC
[GitHub] [fineract] ruchiD opened a new pull request, #2623: FINERACT-1694 Categorization for Events
ruchiD opened a new pull request, #2623:
URL: https://github.com/apache/fineract/pull/2623
## Description
Changes for introducing event categorization for business events.
Events are categorized based on domain objects on which events occur.
Why:
There might be downstream consumers who are interested in all - for example - Loan related events but they don’t want to specify it down to fine-grained level events like Loan Approval, Loan Disbursement.
Changes made:
Introduced category column for events table.
For each event type category must be provided.
External Events are saved with category information.
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on a diff in pull request #2623: FINERACT-1694 Categorization for Events
Posted by GitBox <gi...@apache.org>.
vidakovic commented on code in PR #2623:
URL: https://github.com/apache/fineract/pull/2623#discussion_r982662129
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/business/domain/BulkBusinessEvent.java:
##########
@@ -30,4 +30,9 @@ public BulkBusinessEvent(List<BusinessEvent<?>> value) {
public String getType() {
return "BulkBusinessEvent";
Review Comment:
I know, a bit picky, but could we add somewhere `static final String` variables with these kind of values (if in doubt then put it in class `BulkBusinessEvent.java` as a private variable. Like it is now the string will always be created new when the function `getType()` is called. Having a variable is also a good practice in terms of avoiding "magic strings".
Another suggestion: why not use `BulkBusinessEvent.class.getSimpleName()` (="BulkBusinessEvent")? In that case this whole thing would be immune against refactoring errors...
If I were you I would do this:
```
public class BulkBusinessEvent imiplements BusinessEvent {
private static final String TYPE = getClass().getSimpleName();
...
@Override
public String getType() {
return TYPE;
}
...
}
```
And finally: as the function `getType()` is required by all `BusinessEvent` classes you could create an abstract class and move above code there. Benefits:
- only have to write this code once
- completely type-safe
- avoiding all magic strings (aka refactoring works like a charm)
- ... and memory usage is a tiny bit better (no "new strings" on every call to `getType()`)
This could look like this:
```
public class AbstractBusinessEvent imiplements BusinessEvent {
protected static final String TYPE = getClass().getSimpleName();
...
@Override
public String getType() {
return TYPE;
}
...
}
public class BulkBusinessEvent extends AbstractBusinessEvent {
...
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on a diff in pull request #2623: FINERACT-1694 Categorization for Events
Posted by GitBox <gi...@apache.org>.
vidakovic commented on code in PR #2623:
URL: https://github.com/apache/fineract/pull/2623#discussion_r982664849
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/business/domain/BulkBusinessEvent.java:
##########
@@ -30,4 +30,9 @@ public BulkBusinessEvent(List<BusinessEvent<?>> value) {
public String getType() {
return "BulkBusinessEvent";
}
+
+ @Override
+ public String getCategory() {
+ return "Bulk";
Review Comment:
Similar situation here as with `getType()` in my last comment. To avoid the "magic string" and avoid creating a new string on every call to that function I'd rearrange the code like this:
```
public class BulkBusinessEvent imiplements BusinessEvent {
private static final String CATEGORY = "Bulk";
...
@Override
public String getCategory() {
return CATEGORY;
}
...
}
```
Rinse repeat for similar cases...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic merged pull request #2623: FINERACT-1694 Categorization for Events
Posted by GitBox <gi...@apache.org>.
vidakovic merged PR #2623:
URL: https://github.com/apache/fineract/pull/2623
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org