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/01 16:01:04 UTC
[GitHub] [fineract] galovics opened a new pull request, #2560: FINERACT-1694: Integration of external event storage
galovics opened a new pull request, #2560:
URL: https://github.com/apache/fineract/pull/2560
## Description
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] galovics commented on a diff in pull request #2560: FINERACT-1694: Integration of external event storage
Posted by GitBox <gi...@apache.org>.
galovics commented on code in PR #2560:
URL: https://github.com/apache/fineract/pull/2560#discussion_r960994212
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/repository/domain/ExternalEvent.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.event.external.repository.domain;
+
+import java.time.LocalDate;
+import java.time.OffsetDateTime;
+import javax.persistence.Basic;
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.EnumType;
+import javax.persistence.Enumerated;
+import javax.persistence.FetchType;
+import javax.persistence.Table;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.Setter;
+import org.apache.fineract.infrastructure.core.domain.AbstractPersistableCustom;
+import org.apache.fineract.infrastructure.core.service.DateUtils;
+
+@Entity
+@Table(name = "m_external_event")
+@Getter
+@NoArgsConstructor
+public class ExternalEvent extends AbstractPersistableCustom {
Review Comment:
Intentionally didn't use that one since I didn't want auditing, only a creation date.
--
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] galovics commented on a diff in pull request #2560: FINERACT-1694: Integration of external event storage
Posted by GitBox <gi...@apache.org>.
galovics commented on code in PR #2560:
URL: https://github.com/apache/fineract/pull/2560#discussion_r960994427
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/repository/domain/ExternalEvent.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.event.external.repository.domain;
+
+import java.time.LocalDate;
+import java.time.OffsetDateTime;
+import javax.persistence.Basic;
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.EnumType;
+import javax.persistence.Enumerated;
+import javax.persistence.FetchType;
+import javax.persistence.Table;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.Setter;
+import org.apache.fineract.infrastructure.core.domain.AbstractPersistableCustom;
+import org.apache.fineract.infrastructure.core.service.DateUtils;
+
+@Entity
+@Table(name = "m_external_event")
+@Getter
+@NoArgsConstructor
+public class ExternalEvent extends AbstractPersistableCustom {
+
+ @Column(name = "type", nullable = false)
+ private String type;
+
+ @Basic(fetch = FetchType.LAZY)
+ @Column(name = "data", nullable = false)
+ private byte[] data;
+
+ @Column(name = "created_at", nullable = false)
+ private OffsetDateTime createdAt;
+
+ @Enumerated(EnumType.STRING)
+ @Column(name = "status", nullable = false)
+ @Setter
+ private ExternalEventStatus status;
+
+ @Column(name = "sent_at", nullable = false)
Review Comment:
Yep, nice catch. I already changed the Liquibase script to be nullable. ;-)
--
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] galovics merged pull request #2560: FINERACT-1694: Integration of external event storage
Posted by GitBox <gi...@apache.org>.
galovics merged PR #2560:
URL: https://github.com/apache/fineract/pull/2560
--
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 #2560: FINERACT-1694: Integration of external event storage
Posted by GitBox <gi...@apache.org>.
vidakovic commented on code in PR #2560:
URL: https://github.com/apache/fineract/pull/2560#discussion_r961216824
##########
build.gradle:
##########
@@ -279,6 +279,8 @@ configure(project.fineractJavaProjects) {
sourceSets.main.output.resourcesDir = sourceSets.main.java.outputDir
sourceSets.test.output.resourcesDir = sourceSets.test.java.outputDir
+ check.dependsOn('cucumber')
Review Comment:
Not sure if I follow here... doesn't this mean that the cucumber tests kick in when we do linting with Spotless etc.? Linting and testing being two different build lifecycle stages?
--
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] galovics commented on pull request #2560: FINERACT-1694: Integration of external event storage
Posted by GitBox <gi...@apache.org>.
galovics commented on PR #2560:
URL: https://github.com/apache/fineract/pull/2560#issuecomment-1234667341
> I was wondering whether we should have tests to execute the serializers with "real" data to validate their proper behaviour.
The external events feature is enabled during the integration tests and all the serializers are triggered. Note, if any of the serialization logic fails, the test case will also fail since the serializers are executed within the same transaction as the business operation.
--
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] galovics commented on a diff in pull request #2560: FINERACT-1694: Integration of external event storage
Posted by GitBox <gi...@apache.org>.
galovics commented on code in PR #2560:
URL: https://github.com/apache/fineract/pull/2560#discussion_r960995047
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/repository/domain/ExternalEventStatus.java:
##########
@@ -0,0 +1,23 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.event.external.repository.domain;
+
+public enum ExternalEventStatus {
+ TO_BE_SENT, SENT
Review Comment:
Well, not really because if for some reason we won't be able to send the message, for example due to an outage on the message channel, we'll just retry it next time.
--
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] adamsaghy commented on a diff in pull request #2560: FINERACT-1694: Integration of external event storage
Posted by GitBox <gi...@apache.org>.
adamsaghy commented on code in PR #2560:
URL: https://github.com/apache/fineract/pull/2560#discussion_r960979022
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/repository/domain/ExternalEventStatus.java:
##########
@@ -0,0 +1,23 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.event.external.repository.domain;
+
+public enum ExternalEventStatus {
+ TO_BE_SENT, SENT
Review Comment:
Should be there an error status as well?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/repository/domain/ExternalEvent.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.event.external.repository.domain;
+
+import java.time.LocalDate;
+import java.time.OffsetDateTime;
+import javax.persistence.Basic;
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.EnumType;
+import javax.persistence.Enumerated;
+import javax.persistence.FetchType;
+import javax.persistence.Table;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.Setter;
+import org.apache.fineract.infrastructure.core.domain.AbstractPersistableCustom;
+import org.apache.fineract.infrastructure.core.service.DateUtils;
+
+@Entity
+@Table(name = "m_external_event")
+@Getter
+@NoArgsConstructor
+public class ExternalEvent extends AbstractPersistableCustom {
+
+ @Column(name = "type", nullable = false)
+ private String type;
+
+ @Basic(fetch = FetchType.LAZY)
+ @Column(name = "data", nullable = false)
+ private byte[] data;
+
+ @Column(name = "created_at", nullable = false)
+ private OffsetDateTime createdAt;
+
+ @Enumerated(EnumType.STRING)
+ @Column(name = "status", nullable = false)
+ @Setter
+ private ExternalEventStatus status;
+
+ @Column(name = "sent_at", nullable = false)
Review Comment:
It should be nullable, we will send it asynchronously at a later point in time.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/external/repository/domain/ExternalEvent.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.event.external.repository.domain;
+
+import java.time.LocalDate;
+import java.time.OffsetDateTime;
+import javax.persistence.Basic;
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.EnumType;
+import javax.persistence.Enumerated;
+import javax.persistence.FetchType;
+import javax.persistence.Table;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.Setter;
+import org.apache.fineract.infrastructure.core.domain.AbstractPersistableCustom;
+import org.apache.fineract.infrastructure.core.service.DateUtils;
+
+@Entity
+@Table(name = "m_external_event")
+@Getter
+@NoArgsConstructor
+public class ExternalEvent extends AbstractPersistableCustom {
Review Comment:
Please use the AbstractAuditableWithUTCDateTimeCustom and auditing will be handled automatically!
--
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 #2560: FINERACT-1694: Integration of external event storage
Posted by GitBox <gi...@apache.org>.
vidakovic commented on code in PR #2560:
URL: https://github.com/apache/fineract/pull/2560#discussion_r961397600
##########
build.gradle:
##########
@@ -279,6 +279,8 @@ configure(project.fineractJavaProjects) {
sourceSets.main.output.resourcesDir = sourceSets.main.java.outputDir
sourceSets.test.output.resourcesDir = sourceSets.test.java.outputDir
+ check.dependsOn('cucumber')
Review Comment:
alright... interesting.
--
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] galovics commented on a diff in pull request #2560: FINERACT-1694: Integration of external event storage
Posted by GitBox <gi...@apache.org>.
galovics commented on code in PR #2560:
URL: https://github.com/apache/fineract/pull/2560#discussion_r961395027
##########
build.gradle:
##########
@@ -279,6 +279,8 @@ configure(project.fineractJavaProjects) {
sourceSets.main.output.resourcesDir = sourceSets.main.java.outputDir
sourceSets.test.output.resourcesDir = sourceSets.test.java.outputDir
+ check.dependsOn('cucumber')
Review Comment:
Good catch Aleks. The reason I moved the cucumber execution here is because originally if I wanted to execute simple unit tests in fineract-provider, it was first running ALL the cucumber tests. And considering they are Spring tests, it's quite slow. For a single unit test it was taking 30-60 seconds to run which is way too long.
Moving the cucumber execution to depend on the check task means that the unit tests can be easily executed without the cucumber tests being run beforehand.
Also, this change is in line with the official Gradle recommendation:
> check: Aggregate task that performs verification tasks, such as running the tests. Some plugins add their own verification tasks to check. You should also attach any custom Test tasks to this lifecycle task if you want them to execute for a full build. This task is added by the Base Plugin.
Source: https://docs.gradle.org/current/userguide/java_plugin.html
--
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