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/05/02 16:36:52 UTC
[GitHub] [fineract] logoutdhaval opened a new pull request, #2310: FINERACT-1609: Added Pagination to reports
logoutdhaval opened a new pull request, #2310:
URL: https://github.com/apache/fineract/pull/2310
## 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!
- [x] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [x] 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.
- [x] Create/update unit or integration tests for verifying the changes made.
- [x] 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 closed pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
galovics closed pull request #2310: FINERACT-1609: Added Pagination to reports
URL: https://github.com/apache/fineract/pull/2310
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864039095
##########
integration-tests/src/test/java/org/apache/fineract/integrationtest/stretchyreports/StretchyReportHelper.java:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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.integrationtest.stretchyreports;
+
+import com.google.gson.Gson;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportHelper {
+
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+
+ private String nameOfReport = "report_test";
+ private String reportType = "Table";
+ private String reportSQL = null;
+ private String useReport = null;
+ List<String> reportParameters = Collections.<String>emptyList();
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportHelper.class);
+ private static final String CREATE_REPORT_URL = "/fineract-provider/api/v1/reports?" + Utils.TENANT_IDENTIFIER;
+
+ public StretchyReportHelper(final RequestSpecification requestSpec, final ResponseSpecification responseSpec) {
+ this.requestSpec = requestSpec;
+ this.responseSpec = responseSpec;
+ }
+
+ public String build() {
Review Comment:
removed.
--
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 #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
galovics commented on PR #2310:
URL: https://github.com/apache/fineract/pull/2310#issuecomment-1115265019
Please hold off with merging this. I'd like to check this in detail tomorrow.
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864043061
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL = "/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL = "/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec, this.responseSpec);
+
+ final ResponseSpecification errorResponse = new ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec, this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" + Utils.TENANT_IDENTIFIER + "&R_officeId=1";
+ LinkedHashMap reportData = this.stretchyReportHelper.getStretchyReportDetail(this.requestSpec, this.responseSpec, url, "");
+ ArrayList<String> rdata = (ArrayList<String>) reportData.get("data");
+ Integer reportDataSize = rdata.size();
+ Assertions.assertNotNull(reportDataSize);
+
+ Boolean isPaginationAllowed = true;
+ if (isPaginationAllowed) {
+ Integer pageNo = 0;
+ Integer orderBy = 1;
+ Integer pageCount = 0;
+ Integer pageContent = Math.toIntExact(
+ GlobalConfigurationHelper.getGlobalConfigurationByName(requestSpec, responseSpec, "page-content-limit").getValue());
+ pageCount = reportDataSize / pageContent;
Review Comment:
this code help to get total pageNo count as per the pageContent value.
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r865778436
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -61,23 +71,19 @@
import org.springframework.stereotype.Service;
@Service
+@AllArgsConstructor(onConstructor = @__(@Autowired))
public class ReadReportingServiceImpl implements ReadReportingService {
+ private static final String ORDER_BY_REGEX_PATTERN = "^[0-9]*$";
Review Comment:
Since, Stretchy report is a generic functionality and can serve multiple reports in a generic way, knowing the actual column name for each report will be very difficult.
Even if the column order changes user can select the correct index and run the report to receive the report as per the requirement.
Column idx method is adopted to keep it generic and to work with the current framework with minimal invasive changes.
--
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 #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
galovics commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864571483
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/GenericDataServiceImpl.java:
##########
@@ -88,7 +88,7 @@ public GenericResultsetData fillGenericResultSet(final String sql) {
resultsetDataRows.add(resultsetDataRow);
}
- return new GenericResultsetData(columnHeaders, resultsetDataRows);
+ return new GenericResultsetData(columnHeaders, resultsetDataRows, 0, 0);
Review Comment:
I mean 0, 0 makes sense here? I don't think so.
The total size of the result should be the size of the resultsetDataRows, isn't it? Same for the recordsPerPage since this is not a paginated API.
Let's try to be consistent with the data we're trying to represent.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -161,16 +167,48 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
-
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
-
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ final StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams, isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed = Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+
+ if (isPaginationAllowed) {
+ result = retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
Review Comment:
Let's not pass a mutable StringBuilder as a parameter.
The code should be looking like this:
```
String sql = ... // base SQL
...
if (pagination) {
sql = retrievePaginatedSql(sql)
}
...
return fillGenericResultset(sql)
```
Or similar to this.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java:
##########
@@ -1315,7 +1315,7 @@ public GenericResultsetData retrieveDataTableGenericResultSet(final String dataT
final List<ResultsetRowData> result = fillDatatableResultSetDataRows(sql);
- return new GenericResultsetData(columnHeaders, result);
+ return new GenericResultsetData(columnHeaders, result, 0, 0);
Review Comment:
Same as for the other.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -161,16 +167,48 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
-
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
-
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ final StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams, isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed = Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+
+ if (isPaginationAllowed) {
+ result = retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
+ } else {
+ result = this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ }
final long elapsed = System.currentTimeMillis() - startTime;
LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time: {}", name, type, elapsed);
return result;
}
+ public GenericResultsetData retrieveGenericResultsetWithPagination(final StringBuilder sqlStringBuilder,
+ final Map<String, String> queryParams) {
+ final GenericResultsetData result;
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+
+ final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors);
+ baseDataValidator.reset().parameter(PAGE_NO).value(queryParams.get(PAGE_NO)).notNull().throwValidationErrors();
+
+ baseDataValidator.reset().parameter(PAGINATION_ORDER_BY).value(queryParams.get(PAGINATION_ORDER_BY)).ignoreIfNull()
+ .matchesRegularExpression(ORDER_BY_REGEX_PATTERN).throwValidationErrors();
+ int pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
+
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(), null,
+ new ReportMapper(sqlStringBuilder));
+ int pageNo = Integer.parseInt(queryParams.get(ReportingConstants.PAGE_NO));
+
+ pageNo = pageNo * pageSize;
+ sqlStringBuilder.append(" order by ").append(queryParams.get(PAGINATION_ORDER_BY));
+ sqlStringBuilder.append(" ");
+ sqlStringBuilder.append(sqlGenerator.limit(pageSize, pageNo));
+ result = this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ result.setTotalItems(reportData.getTotalFilteredRecords());
Review Comment:
I don't like this. Manipulating this after it's already constructed seems to be a big code smell. Why don't we set these during construction time?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -61,23 +71,19 @@
import org.springframework.stereotype.Service;
@Service
+@AllArgsConstructor(onConstructor = @__(@Autowired))
Review Comment:
RequiredArgsConstructor makes more sense here plus there's no need to put the Autowired annotation onto the generated constructor since Spring 4.3+ supports constructor dependency injection without it.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java:
##########
@@ -1337,7 +1337,7 @@ private GenericResultsetData retrieveDataTableGenericResultSetForUpdate(final St
final List<ResultsetRowData> result = fillDatatableResultSetDataRows(sql);
- return new GenericResultsetData(columnHeaders, result);
+ return new GenericResultsetData(columnHeaders, result, 0, 0);
Review Comment:
Same as for the other.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -161,16 +167,48 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
-
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
-
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ final StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams, isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed = Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+
+ if (isPaginationAllowed) {
+ result = retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
+ } else {
+ result = this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ }
final long elapsed = System.currentTimeMillis() - startTime;
LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time: {}", name, type, elapsed);
return result;
}
+ public GenericResultsetData retrieveGenericResultsetWithPagination(final StringBuilder sqlStringBuilder,
+ final Map<String, String> queryParams) {
+ final GenericResultsetData result;
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+
+ final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors);
Review Comment:
Please extract the validation to a separate method/class to improve readability.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -61,23 +71,19 @@
import org.springframework.stereotype.Service;
@Service
+@AllArgsConstructor(onConstructor = @__(@Autowired))
public class ReadReportingServiceImpl implements ReadReportingService {
+ private static final String ORDER_BY_REGEX_PATTERN = "^[0-9]*$";
Review Comment:
I just realized this. Why do we want to sort only based on the number of the column? This can be very difficult to maintain. Imagine people start building on this and suddenly we change the SQL that we run and we switch up the column order. Functionally nothing will break without ordering but if you use ordering, it's not gonna sort on the expected columns.
Why don't we switch this into providing the column name an order by that?
If this is something you really want to keep (column number ordering) then please add test cases to cover every single column ordering, otherwise we'll regress in the future.
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL = "/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL = "/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec, this.responseSpec);
+
+ final ResponseSpecification errorResponse = new ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec, this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" + Utils.TENANT_IDENTIFIER + "&R_officeId=1";
+ LinkedHashMap reportData = this.stretchyReportHelper.getStretchyReportDetail(this.requestSpec, this.responseSpec, url, "");
+ ArrayList<String> rdata = (ArrayList<String>) reportData.get("data");
+ Integer reportDataSize = rdata.size();
+ Assertions.assertNotNull(reportDataSize);
+
+ Boolean isPaginationAllowed = true;
+ if (isPaginationAllowed) {
Review Comment:
Okay but then why the if? The test-case is clearly for paginationAllowed only but then why the check?
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864037589
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -162,9 +181,33 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final GenericResultsetData result;
+ Boolean paginationAllowed = BooleanUtil.parseBoolean(queryParams.get(ReportingConstants.isPaginationAllowed));
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ if (paginationAllowed) {
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
Review Comment:
done
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -37,8 +40,15 @@
import java.util.Map;
import java.util.Set;
import javax.ws.rs.core.StreamingOutput;
+import liquibase.util.BooleanUtil;
Review Comment:
ok
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r865645158
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -161,16 +167,48 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
-
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
-
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ final StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams, isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed = Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+
+ if (isPaginationAllowed) {
+ result = retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
+ } else {
+ result = this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ }
final long elapsed = System.currentTimeMillis() - startTime;
LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time: {}", name, type, elapsed);
return result;
}
+ public GenericResultsetData retrieveGenericResultsetWithPagination(final StringBuilder sqlStringBuilder,
+ final Map<String, String> queryParams) {
+ final GenericResultsetData result;
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+
+ final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors);
+ baseDataValidator.reset().parameter(PAGE_NO).value(queryParams.get(PAGE_NO)).notNull().throwValidationErrors();
+
+ baseDataValidator.reset().parameter(PAGINATION_ORDER_BY).value(queryParams.get(PAGINATION_ORDER_BY)).ignoreIfNull()
+ .matchesRegularExpression(ORDER_BY_REGEX_PATTERN).throwValidationErrors();
+ int pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
+
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(), null,
+ new ReportMapper(sqlStringBuilder));
+ int pageNo = Integer.parseInt(queryParams.get(ReportingConstants.PAGE_NO));
+
+ pageNo = pageNo * pageSize;
+ sqlStringBuilder.append(" order by ").append(queryParams.get(PAGINATION_ORDER_BY));
+ sqlStringBuilder.append(" ");
+ sqlStringBuilder.append(sqlGenerator.limit(pageSize, pageNo));
+ result = this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ result.setTotalItems(reportData.getTotalFilteredRecords());
Review Comment:
totalItems and recordsPerPage required for frontend and we are not getting this from the sql.
--
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] logoutdhaval closed pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval closed pull request #2310: FINERACT-1609: Added Pagination to reports
URL: https://github.com/apache/fineract/pull/2310
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864035685
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -69,14 +79,23 @@ public class ReadReportingServiceImpl implements ReadReportingService {
private final PlatformSecurityContext context;
private final GenericDataService genericDataService;
private final SqlInjectionPreventerService sqlInjectionPreventerService;
+ private final DatabaseSpecificSQLGenerator sqlGenerator;
+ private final ConfigurationDomainService configurationDomainService;
+ private final PaginationHelper paginationHelper;
+ private static final String ORDER_BY_REGEX_PATTERN = "^[0-9]*$";
@Autowired
public ReadReportingServiceImpl(final PlatformSecurityContext context, final JdbcTemplate jdbcTemplate,
- final GenericDataService genericDataService, SqlInjectionPreventerService sqlInjectionPreventerService) {
+ final GenericDataService genericDataService, SqlInjectionPreventerService sqlInjectionPreventerService,
Review Comment:
done
--
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 #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
galovics commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r863541735
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/domain/ConfigurationDomainServiceJpa.java:
##########
@@ -220,6 +220,16 @@ public int getRoundingMode() {
return defaultValue;
}
+ @Override
+ public Integer reportsPaginationNumberOfItemsPerPage() {
+ final String propertyName = "reports-pagination-number-of-items-per-page";
Review Comment:
I know this is kind of the standard but would you mind starting to extract these into a constant?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -162,9 +181,33 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final GenericResultsetData result;
+ Boolean paginationAllowed = BooleanUtil.parseBoolean(queryParams.get(ReportingConstants.isPaginationAllowed));
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ if (paginationAllowed) {
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+
+ final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors);
+ baseDataValidator.reset().parameter(pageNo).value(queryParams.get(pageNo)).notNull().throwValidationErrors();
+
+ baseDataValidator.reset().parameter(paginationOrderBy).value(queryParams.get(paginationOrderBy)).ignoreIfNull()
+ .matchesRegularExpression(ORDER_BY_REGEX_PATTERN).throwValidationErrors();
+ Integer pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
+
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sql, null, new ReportMapper(sql));
+ Integer pageNo = Integer.parseInt(queryParams.get(ReportingConstants.pageNo));
+
+ pageNo = pageNo * pageSize;
+ sql = sql + " order by " + queryParams.get(paginationOrderBy) + " limit " + pageSize + " offset " + pageNo;
Review Comment:
This is a PostgreSQL specific SQL. That's why we have the DatabaseSpecificSQLGenerator#limit method available, to generate this.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java:
##########
@@ -1315,7 +1315,7 @@ public GenericResultsetData retrieveDataTableGenericResultSet(final String dataT
final List<ResultsetRowData> result = fillDatatableResultSetDataRows(sql);
- return new GenericResultsetData(columnHeaders, result);
+ return new GenericResultsetData(columnHeaders, result, null, null);
Review Comment:
Smelly usage.
##########
integration-tests/src/test/java/org/apache/fineract/integrationtest/stretchyreports/StretchyReportHelper.java:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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.integrationtest.stretchyreports;
+
+import com.google.gson.Gson;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportHelper {
+
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+
+ private String nameOfReport = "report_test";
+ private String reportType = "Table";
+ private String reportSQL = null;
+ private String useReport = null;
+ List<String> reportParameters = Collections.<String>emptyList();
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportHelper.class);
+ private static final String CREATE_REPORT_URL = "/fineract-provider/api/v1/reports?" + Utils.TENANT_IDENTIFIER;
+
+ public StretchyReportHelper(final RequestSpecification requestSpec, final ResponseSpecification responseSpec) {
+ this.requestSpec = requestSpec;
+ this.responseSpec = responseSpec;
+ }
+
+ public String build() {
Review Comment:
build what?
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/common/GlobalConfigurationHelper.java:
##########
@@ -99,8 +99,8 @@ public static void verifyAllDefaultGlobalConfigurations(final RequestSpecificati
ArrayList<HashMap> actualGlobalConfigurations = getAllGlobalConfigurations(requestSpec, responseSpec);
// There are currently 37 global configurations.
Review Comment:
Delete the comment please or at least update it. Worst comment is the misleading comment.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -69,14 +79,23 @@ public class ReadReportingServiceImpl implements ReadReportingService {
private final PlatformSecurityContext context;
private final GenericDataService genericDataService;
private final SqlInjectionPreventerService sqlInjectionPreventerService;
+ private final DatabaseSpecificSQLGenerator sqlGenerator;
+ private final ConfigurationDomainService configurationDomainService;
+ private final PaginationHelper paginationHelper;
+ private static final String ORDER_BY_REGEX_PATTERN = "^[0-9]*$";
@Autowired
public ReadReportingServiceImpl(final PlatformSecurityContext context, final JdbcTemplate jdbcTemplate,
- final GenericDataService genericDataService, SqlInjectionPreventerService sqlInjectionPreventerService) {
+ final GenericDataService genericDataService, SqlInjectionPreventerService sqlInjectionPreventerService,
Review Comment:
Why not turning it into a Lombok constructor?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -69,14 +79,23 @@ public class ReadReportingServiceImpl implements ReadReportingService {
private final PlatformSecurityContext context;
private final GenericDataService genericDataService;
private final SqlInjectionPreventerService sqlInjectionPreventerService;
+ private final DatabaseSpecificSQLGenerator sqlGenerator;
+ private final ConfigurationDomainService configurationDomainService;
+ private final PaginationHelper paginationHelper;
+ private static final String ORDER_BY_REGEX_PATTERN = "^[0-9]*$";
Review Comment:
Please put the static constant to the top.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/GenericResultsetData.java:
##########
@@ -27,10 +27,15 @@ public final class GenericResultsetData {
private final List<ResultsetColumnHeaderData> columnHeaders;
private final List<ResultsetRowData> data;
+ private Integer totalItems;
Review Comment:
Why not final?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -162,9 +181,33 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final GenericResultsetData result;
+ Boolean paginationAllowed = BooleanUtil.parseBoolean(queryParams.get(ReportingConstants.isPaginationAllowed));
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ if (paginationAllowed) {
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+
+ final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors);
+ baseDataValidator.reset().parameter(pageNo).value(queryParams.get(pageNo)).notNull().throwValidationErrors();
+
+ baseDataValidator.reset().parameter(paginationOrderBy).value(queryParams.get(paginationOrderBy)).ignoreIfNull()
+ .matchesRegularExpression(ORDER_BY_REGEX_PATTERN).throwValidationErrors();
+ Integer pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
+
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sql, null, new ReportMapper(sql));
+ Integer pageNo = Integer.parseInt(queryParams.get(ReportingConstants.pageNo));
+
+ pageNo = pageNo * pageSize;
+ sql = sql + " order by " + queryParams.get(paginationOrderBy) + " limit " + pageSize + " offset " + pageNo;
Review Comment:
Also, this is a potential SQL injection problem. Use query parameters instead of concatenating the string.
Also, where you can't use it, make sure to escape it (SqlInjectionPreventerService)
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/GenericResultsetData.java:
##########
@@ -41,6 +46,14 @@ public List<ResultsetRowData> getData() {
return this.data;
}
+ public void setReportSize(Integer totalItems) {
Review Comment:
Lombok setters + getters pls.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/GenericResultsetData.java:
##########
@@ -27,10 +27,15 @@ public final class GenericResultsetData {
private final List<ResultsetColumnHeaderData> columnHeaders;
private final List<ResultsetRowData> data;
+ private Integer totalItems;
Review Comment:
Why the wrapper Integer class instead of primitive int?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -162,9 +181,33 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final GenericResultsetData result;
+ Boolean paginationAllowed = BooleanUtil.parseBoolean(queryParams.get(ReportingConstants.isPaginationAllowed));
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ if (paginationAllowed) {
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+
+ final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors);
+ baseDataValidator.reset().parameter(pageNo).value(queryParams.get(pageNo)).notNull().throwValidationErrors();
+
+ baseDataValidator.reset().parameter(paginationOrderBy).value(queryParams.get(paginationOrderBy)).ignoreIfNull()
+ .matchesRegularExpression(ORDER_BY_REGEX_PATTERN).throwValidationErrors();
+ Integer pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
+
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sql, null, new ReportMapper(sql));
+ Integer pageNo = Integer.parseInt(queryParams.get(ReportingConstants.pageNo));
+
+ pageNo = pageNo * pageSize;
+ sql = sql + " order by " + queryParams.get(paginationOrderBy) + " limit " + pageSize + " offset " + pageNo;
+
+ result = this.genericDataService.fillGenericResultSet(sql);
+ result.setRecordsPerPage(pageSize);
Review Comment:
I don't like setting these after constructing the result.
By allowing setters to be called, we essentially allow screwing up the invariant of the class.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/GenericDataServiceImpl.java:
##########
@@ -88,7 +88,7 @@ public GenericResultsetData fillGenericResultSet(final String sql) {
resultsetDataRows.add(resultsetDataRow);
}
- return new GenericResultsetData(columnHeaders, resultsetDataRows);
+ return new GenericResultsetData(columnHeaders, resultsetDataRows, null, null);
Review Comment:
This is definitely misleading.
Either you're mixing up the responsibility of a GenericResultsetData by putting those 2 new attributes there while not using it everywhere.
Or you just need to provide those 2 numbers here as well
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -37,8 +40,15 @@
import java.util.Map;
import java.util.Set;
import javax.ws.rs.core.StreamingOutput;
+import liquibase.util.BooleanUtil;
Review Comment:
Please don't use Liquibase internal classes.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -216,6 +258,21 @@ private String getSql(final String name, final String type) {
throw new ReportNotFoundException(encodedName);
}
+ private static final class ReportMapper implements RowMapper<GenericResultsetData> {
+
+ private final String schema;
+
+ ReportMapper(String sql) {
+
+ this.schema = sql;
+ }
+
+ @Override
+ public GenericResultsetData mapRow(ResultSet rs, int rowNum) throws SQLException {
Review Comment:
Ehm. This is another smell of counting the rows the wrong way. This definitely has to go.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -162,9 +181,33 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final GenericResultsetData result;
+ Boolean paginationAllowed = BooleanUtil.parseBoolean(queryParams.get(ReportingConstants.isPaginationAllowed));
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ if (paginationAllowed) {
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
Review Comment:
Can we extract the whole paginationAllowed branch into a separate method?
Let's try to keep everything as simple as possible.
One branch for paginating the result, and one for non-paginated result.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/GenericResultsetData.java:
##########
@@ -27,10 +27,15 @@ public final class GenericResultsetData {
private final List<ResultsetColumnHeaderData> columnHeaders;
private final List<ResultsetRowData> data;
+ private Integer totalItems;
+ private Integer recordsPerPage;
- public GenericResultsetData(final List<ResultsetColumnHeaderData> columnHeaders, final List<ResultsetRowData> resultsetDataRows) {
+ public GenericResultsetData(final List<ResultsetColumnHeaderData> columnHeaders, final List<ResultsetRowData> resultsetDataRows,
Review Comment:
Please turn this constructor into a Lombok constructor.
##########
integration-tests/src/test/java/org/apache/fineract/integrationtest/stretchyreports/StretchyReportHelper.java:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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.integrationtest.stretchyreports;
+
+import com.google.gson.Gson;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportHelper {
+
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+
+ private String nameOfReport = "report_test";
+ private String reportType = "Table";
+ private String reportSQL = null;
+ private String useReport = null;
+ List<String> reportParameters = Collections.<String>emptyList();
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportHelper.class);
Review Comment:
Lombok logger pls.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -162,9 +181,33 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final GenericResultsetData result;
+ Boolean paginationAllowed = BooleanUtil.parseBoolean(queryParams.get(ReportingConstants.isPaginationAllowed));
Review Comment:
Why the wrapper Boolean variable type? Simply go with primitive boolean.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java:
##########
@@ -1337,7 +1337,7 @@ private GenericResultsetData retrieveDataTableGenericResultSetForUpdate(final St
final List<ResultsetRowData> result = fillDatatableResultSetDataRows(sql);
- return new GenericResultsetData(columnHeaders, result);
+ return new GenericResultsetData(columnHeaders, result, null, null);
Review Comment:
Smelly usage.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReportingConstants.java:
##########
@@ -0,0 +1,27 @@
+/**
+ * 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.dataqueries.service;
+
+@SuppressWarnings({ "HideUtilityClassConstructor" })
+public class ReportingConstants {
+
+ public static final String pageNo = "pageNo";
Review Comment:
Can you please follow the proper Java naming convention here for constants?
Should be PAGE_NO, IS_PAGINATION_ALLOWED, etc.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/report/service/ReportingProcessService.java:
##########
@@ -39,6 +43,21 @@ default Map<String, String> getReportParams(final MultivaluedMap<String, String>
SQLInjectionValidator.validateSQLInput(pValue);
reportParams.put(pKey, pValue);
}
+ if (k.equalsIgnoreCase(isPaginationAllowed)) {
Review Comment:
Why 3 different branches? Why not a single one with OR logical checks?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -162,9 +181,33 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final GenericResultsetData result;
+ Boolean paginationAllowed = BooleanUtil.parseBoolean(queryParams.get(ReportingConstants.isPaginationAllowed));
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ if (paginationAllowed) {
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+
+ final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors);
+ baseDataValidator.reset().parameter(pageNo).value(queryParams.get(pageNo)).notNull().throwValidationErrors();
+
+ baseDataValidator.reset().parameter(paginationOrderBy).value(queryParams.get(paginationOrderBy)).ignoreIfNull()
+ .matchesRegularExpression(ORDER_BY_REGEX_PATTERN).throwValidationErrors();
+ Integer pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
+
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sql, null, new ReportMapper(sql));
Review Comment:
Just as a note. This will retrieve the data itself plus it will run an extra query to count the resultset.
This is definitely wasting time since we're never using the dataset but only the count.
##########
integration-tests/src/test/java/org/apache/fineract/integrationtest/stretchyreports/StretchyReportHelper.java:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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.integrationtest.stretchyreports;
+
+import com.google.gson.Gson;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportHelper {
+
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+
+ private String nameOfReport = "report_test";
+ private String reportType = "Table";
+ private String reportSQL = null;
+ private String useReport = null;
+ List<String> reportParameters = Collections.<String>emptyList();
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportHelper.class);
+ private static final String CREATE_REPORT_URL = "/fineract-provider/api/v1/reports?" + Utils.TENANT_IDENTIFIER;
+
+ public StretchyReportHelper(final RequestSpecification requestSpec, final ResponseSpecification responseSpec) {
Review Comment:
Whats the reason you're passing these as constructor parameters? You are not using it since every method on the helper gets its own specifications in parameters.
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL = "/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL = "/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec, this.responseSpec);
+
+ final ResponseSpecification errorResponse = new ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec, this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" + Utils.TENANT_IDENTIFIER + "&R_officeId=1";
Review Comment:
Why constructing the URL here? Isn't it the job of the Helper you created?
##########
integration-tests/src/test/java/org/apache/fineract/integrationtest/stretchyreports/StretchyReportHelper.java:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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.integrationtest.stretchyreports;
+
+import com.google.gson.Gson;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportHelper {
+
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+
+ private String nameOfReport = "report_test";
+ private String reportType = "Table";
+ private String reportSQL = null;
+ private String useReport = null;
+ List<String> reportParameters = Collections.<String>emptyList();
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportHelper.class);
+ private static final String CREATE_REPORT_URL = "/fineract-provider/api/v1/reports?" + Utils.TENANT_IDENTIFIER;
+
+ public StretchyReportHelper(final RequestSpecification requestSpec, final ResponseSpecification responseSpec) {
+ this.requestSpec = requestSpec;
+ this.responseSpec = responseSpec;
+ }
+
+ public String build() {
+ final HashMap<String, Object> map = new HashMap<>();
+
+ map.put("reportName", this.nameOfReport);
+ map.put("reportSql", this.reportSQL);
+ map.put("reportType", this.reportType);
+ map.put("useReport", this.useReport);
+ map.put("reportParameters", this.reportParameters);
+
+ String reportCreateJson = new Gson().toJson(map);
+ LOG.info("{}", reportCreateJson);
+ return reportCreateJson;
+ }
+
+ public LinkedHashMap getStretchyReportDetail(final RequestSpecification requestSpec, final ResponseSpecification responseSpec,
Review Comment:
No Map return type please. Use the fineract-client for typesafe APIs like I did in ClientHelper#getClient
##########
integration-tests/src/test/java/org/apache/fineract/integrationtest/stretchyreports/StretchyReportHelper.java:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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.integrationtest.stretchyreports;
+
+import com.google.gson.Gson;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportHelper {
+
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+
+ private String nameOfReport = "report_test";
+ private String reportType = "Table";
+ private String reportSQL = null;
+ private String useReport = null;
+ List<String> reportParameters = Collections.<String>emptyList();
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportHelper.class);
+ private static final String CREATE_REPORT_URL = "/fineract-provider/api/v1/reports?" + Utils.TENANT_IDENTIFIER;
+
+ public StretchyReportHelper(final RequestSpecification requestSpec, final ResponseSpecification responseSpec) {
+ this.requestSpec = requestSpec;
+ this.responseSpec = responseSpec;
+ }
+
+ public String build() {
+ final HashMap<String, Object> map = new HashMap<>();
+
+ map.put("reportName", this.nameOfReport);
+ map.put("reportSql", this.reportSQL);
+ map.put("reportType", this.reportType);
+ map.put("useReport", this.useReport);
+ map.put("reportParameters", this.reportParameters);
+
+ String reportCreateJson = new Gson().toJson(map);
+ LOG.info("{}", reportCreateJson);
+ return reportCreateJson;
+ }
+
+ public LinkedHashMap getStretchyReportDetail(final RequestSpecification requestSpec, final ResponseSpecification responseSpec,
+ final String url, final String jsonReturn) {
+
+ final LinkedHashMap response = Utils.performServerGet(requestSpec, responseSpec, url, jsonReturn);
+ return response;
+ }
+
+ public Object getStretchyReportDataDetail(final RequestSpecification requestSpec, final ResponseSpecification responseSpec,
Review Comment:
Same as above.
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL = "/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL = "/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec, this.responseSpec);
+
+ final ResponseSpecification errorResponse = new ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec, this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" + Utils.TENANT_IDENTIFIER + "&R_officeId=1";
+ LinkedHashMap reportData = this.stretchyReportHelper.getStretchyReportDetail(this.requestSpec, this.responseSpec, url, "");
+ ArrayList<String> rdata = (ArrayList<String>) reportData.get("data");
+ Integer reportDataSize = rdata.size();
+ Assertions.assertNotNull(reportDataSize);
+
+ Boolean isPaginationAllowed = true;
+ if (isPaginationAllowed) {
+ Integer pageNo = 0;
+ Integer orderBy = 1;
+ Integer pageCount = 0;
+ Integer pageContent = Math.toIntExact(
+ GlobalConfigurationHelper.getGlobalConfigurationByName(requestSpec, responseSpec, "page-content-limit").getValue());
Review Comment:
This is the wrong parameter.
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL = "/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL = "/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec, this.responseSpec);
+
+ final ResponseSpecification errorResponse = new ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec, this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" + Utils.TENANT_IDENTIFIER + "&R_officeId=1";
+ LinkedHashMap reportData = this.stretchyReportHelper.getStretchyReportDetail(this.requestSpec, this.responseSpec, url, "");
+ ArrayList<String> rdata = (ArrayList<String>) reportData.get("data");
+ Integer reportDataSize = rdata.size();
+ Assertions.assertNotNull(reportDataSize);
+
+ Boolean isPaginationAllowed = true;
+ if (isPaginationAllowed) {
Review Comment:
Isn't this always evaluating to true?
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL = "/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL = "/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec, this.responseSpec);
+
+ final ResponseSpecification errorResponse = new ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec, this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" + Utils.TENANT_IDENTIFIER + "&R_officeId=1";
+ LinkedHashMap reportData = this.stretchyReportHelper.getStretchyReportDetail(this.requestSpec, this.responseSpec, url, "");
+ ArrayList<String> rdata = (ArrayList<String>) reportData.get("data");
+ Integer reportDataSize = rdata.size();
+ Assertions.assertNotNull(reportDataSize);
+
+ Boolean isPaginationAllowed = true;
+ if (isPaginationAllowed) {
+ Integer pageNo = 0;
+ Integer orderBy = 1;
+ Integer pageCount = 0;
+ Integer pageContent = Math.toIntExact(
+ GlobalConfigurationHelper.getGlobalConfigurationByName(requestSpec, responseSpec, "page-content-limit").getValue());
+ pageCount = reportDataSize / pageContent;
+ if (reportDataSize % pageContent != 0) {
+ pageCount++;
+ }
+ Integer resultant_report_with_limit_size = 0;
Review Comment:
This is not conforming with the Java naming guideline.
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL = "/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL = "/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec, this.responseSpec);
+
+ final ResponseSpecification errorResponse = new ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec, this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" + Utils.TENANT_IDENTIFIER + "&R_officeId=1";
+ LinkedHashMap reportData = this.stretchyReportHelper.getStretchyReportDetail(this.requestSpec, this.responseSpec, url, "");
+ ArrayList<String> rdata = (ArrayList<String>) reportData.get("data");
+ Integer reportDataSize = rdata.size();
+ Assertions.assertNotNull(reportDataSize);
+
+ Boolean isPaginationAllowed = true;
+ if (isPaginationAllowed) {
+ Integer pageNo = 0;
+ Integer orderBy = 1;
+ Integer pageCount = 0;
+ Integer pageContent = Math.toIntExact(
+ GlobalConfigurationHelper.getGlobalConfigurationByName(requestSpec, responseSpec, "page-content-limit").getValue());
+ pageCount = reportDataSize / pageContent;
Review Comment:
This logic is very much not trivial. Do you mind explaining whats going on here?
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864033900
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/domain/ConfigurationDomainServiceJpa.java:
##########
@@ -220,6 +220,16 @@ public int getRoundingMode() {
return defaultValue;
}
+ @Override
+ public Integer reportsPaginationNumberOfItemsPerPage() {
+ final String propertyName = "reports-pagination-number-of-items-per-page";
Review Comment:
done
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/GenericResultsetData.java:
##########
@@ -27,10 +27,15 @@ public final class GenericResultsetData {
private final List<ResultsetColumnHeaderData> columnHeaders;
private final List<ResultsetRowData> data;
+ private Integer totalItems;
+ private Integer recordsPerPage;
- public GenericResultsetData(final List<ResultsetColumnHeaderData> columnHeaders, final List<ResultsetRowData> resultsetDataRows) {
+ public GenericResultsetData(final List<ResultsetColumnHeaderData> columnHeaders, final List<ResultsetRowData> resultsetDataRows,
Review Comment:
done
--
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 #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
galovics commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r883538073
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}", LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams, isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed = Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(), null,
+ new ReportMapper(sqlStringBuilder));
+ if (isPaginationAllowed) {
+ sqlStringBuilder = retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
}
+ result = this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ int pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final long elapsed = System.currentTimeMillis() - startTime;
+ LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time: {}", name, type, elapsed);
Review Comment:
Info is way too verbose here. Debug/trace please.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/GenericResultsetData.java:
##########
@@ -53,6 +61,11 @@ public String getColTypeOfColumnNamed(final String columnName) {
return colType;
}
+ public static GenericResultsetData setTotalItemsAndRecordsPerPage(final GenericResultsetData genericResultsetData, final int totalItems,
Review Comment:
This is a very bad pattern. This method is not setting anything, it's constructing a brand new object. Why isn't the constructor above enough for everything?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}", LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams, isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed = Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(), null,
+ new ReportMapper(sqlStringBuilder));
+ if (isPaginationAllowed) {
+ sqlStringBuilder = retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
}
+ result = this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ int pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final long elapsed = System.currentTimeMillis() - startTime;
+ LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time: {}", name, type, elapsed);
Review Comment:
LogParameterEscapeUtil is also missing here.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -51,24 +59,31 @@
import org.apache.fineract.infrastructure.documentmanagement.contentrepository.FileSystemContentRepository;
import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext;
import org.apache.fineract.infrastructure.security.service.SqlInjectionPreventerService;
-import org.apache.fineract.infrastructure.security.utils.LogParameterEscapeUtil;
import org.apache.fineract.useradministration.domain.AppUser;
import org.owasp.esapi.ESAPI;
import org.owasp.esapi.codecs.UnixCodec;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.RowMapper;
import org.springframework.jdbc.support.rowset.SqlRowSet;
import org.springframework.stereotype.Service;
@Service
-@Slf4j
+
@RequiredArgsConstructor
public class ReadReportingServiceImpl implements ReadReportingService {
+ private static final String ORDER_BY_REGEX_PATTERN = "^[0-9]*$";
+ private static final Logger LOG = LoggerFactory.getLogger(ReadReportingServiceImpl.class);
Review Comment:
Why was the `@Slf4j` annotation replaced here?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}", LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
Review Comment:
I don't see the reason to use a StringBuilder here. Clarify please.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}", LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams, isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed = Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(), null,
+ new ReportMapper(sqlStringBuilder));
Review Comment:
The ReportMapper is not doing anything, why do we have it?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}", LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams, isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed = Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(), null,
Review Comment:
Why is this needed, again?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -213,6 +251,21 @@ private String getSql(final String name, final String type) {
throw new ReportNotFoundException(encodedName);
}
+ private static final class ReportMapper implements RowMapper<GenericResultsetData> {
Review Comment:
This is superflous. Not doing anything.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}", LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams, isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed = Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(), null,
+ new ReportMapper(sqlStringBuilder));
+ if (isPaginationAllowed) {
+ sqlStringBuilder = retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
}
+ result = this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ int pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final long elapsed = System.currentTimeMillis() - startTime;
+ LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time: {}", name, type, elapsed);
+ return GenericResultsetData.setTotalItemsAndRecordsPerPage(result, reportData.getTotalFilteredRecords(), pageSize);
+ }
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ public StringBuilder retrieveGenericResultsetWithPagination(final StringBuilder sqlStringBuilder,
Review Comment:
This method is lying. It's not retrieving a generic resultset with pagination. It's constructing the SQL for it.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}", LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
Review Comment:
Also, you removed the LogParameterEscapeUtil call from here. Please redo it.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}", LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
Review Comment:
Info is way too verbose here. Debug/trace please.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}", LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams, isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed = Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(), null,
+ new ReportMapper(sqlStringBuilder));
+ if (isPaginationAllowed) {
+ sqlStringBuilder = retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
}
+ result = this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ int pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final long elapsed = System.currentTimeMillis() - startTime;
+ LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time: {}", name, type, elapsed);
+ return GenericResultsetData.setTotalItemsAndRecordsPerPage(result, reportData.getTotalFilteredRecords(), pageSize);
+ }
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ public StringBuilder retrieveGenericResultsetWithPagination(final StringBuilder sqlStringBuilder,
+ final Map<String, String> queryParams) {
+ retrieveGenericResultsetWithPaginationDataValidator(queryParams);
+ int pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
+ int pageNo = Integer.parseInt(queryParams.get(ReportingConstants.OFFSET));
+
+ pageNo = pageNo * pageSize;
+ sqlStringBuilder.append(" order by ").append(queryParams.get(PAGINATION_ORDER_BY));
+ sqlStringBuilder.append(" ");
Review Comment:
Very very bad pattern. You're making this method to have side-effects while also returning the object. Decide on one and do it accordingly. Preferably return the Stirng.
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r863842029
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/GenericResultsetData.java:
##########
@@ -27,10 +27,15 @@ public final class GenericResultsetData {
private final List<ResultsetColumnHeaderData> columnHeaders;
private final List<ResultsetRowData> data;
+ private Integer totalReportSize;
Review Comment:
changed to totalItem
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864041528
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL = "/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL = "/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec, this.responseSpec);
+
+ final ResponseSpecification errorResponse = new ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec, this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" + Utils.TENANT_IDENTIFIER + "&R_officeId=1";
+ LinkedHashMap reportData = this.stretchyReportHelper.getStretchyReportDetail(this.requestSpec, this.responseSpec, url, "");
+ ArrayList<String> rdata = (ArrayList<String>) reportData.get("data");
+ Integer reportDataSize = rdata.size();
+ Assertions.assertNotNull(reportDataSize);
+
+ Boolean isPaginationAllowed = true;
+ if (isPaginationAllowed) {
+ Integer pageNo = 0;
+ Integer orderBy = 1;
+ Integer pageCount = 0;
+ Integer pageContent = Math.toIntExact(
+ GlobalConfigurationHelper.getGlobalConfigurationByName(requestSpec, responseSpec, "page-content-limit").getValue());
+ pageCount = reportDataSize / pageContent;
+ if (reportDataSize % pageContent != 0) {
+ pageCount++;
+ }
+ Integer resultant_report_with_limit_size = 0;
Review Comment:
changed to resultantReportWithLimitSize
--
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 #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
galovics commented on PR #2310:
URL: https://github.com/apache/fineract/pull/2310#issuecomment-1198013728
Closing this as it's abandoned.
--
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 #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
galovics commented on PR #2310:
URL: https://github.com/apache/fineract/pull/2310#issuecomment-1115950795
Accidentally closed the PR but reopened it for now.
I wanted to say, I'm also missing unit tests for some of the service classes because I feel like the integration tests are not really covering each scenario.
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864035349
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/GenericResultsetData.java:
##########
@@ -27,10 +27,15 @@ public final class GenericResultsetData {
private final List<ResultsetColumnHeaderData> columnHeaders;
private final List<ResultsetRowData> data;
+ private Integer totalItems;
Review Comment:
changed to primitive
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -69,14 +79,23 @@ public class ReadReportingServiceImpl implements ReadReportingService {
private final PlatformSecurityContext context;
private final GenericDataService genericDataService;
private final SqlInjectionPreventerService sqlInjectionPreventerService;
+ private final DatabaseSpecificSQLGenerator sqlGenerator;
+ private final ConfigurationDomainService configurationDomainService;
+ private final PaginationHelper paginationHelper;
+ private static final String ORDER_BY_REGEX_PATTERN = "^[0-9]*$";
Review Comment:
done
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864036681
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -162,9 +181,33 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final GenericResultsetData result;
+ Boolean paginationAllowed = BooleanUtil.parseBoolean(queryParams.get(ReportingConstants.isPaginationAllowed));
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ if (paginationAllowed) {
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+
+ final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors);
+ baseDataValidator.reset().parameter(pageNo).value(queryParams.get(pageNo)).notNull().throwValidationErrors();
+
+ baseDataValidator.reset().parameter(paginationOrderBy).value(queryParams.get(paginationOrderBy)).ignoreIfNull()
+ .matchesRegularExpression(ORDER_BY_REGEX_PATTERN).throwValidationErrors();
+ Integer pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
+
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sql, null, new ReportMapper(sql));
+ Integer pageNo = Integer.parseInt(queryParams.get(ReportingConstants.pageNo));
+
+ pageNo = pageNo * pageSize;
+ sql = sql + " order by " + queryParams.get(paginationOrderBy) + " limit " + pageSize + " offset " + pageNo;
Review Comment:
changed the implementation to your suggested one.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -162,9 +181,33 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final GenericResultsetData result;
+ Boolean paginationAllowed = BooleanUtil.parseBoolean(queryParams.get(ReportingConstants.isPaginationAllowed));
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ if (paginationAllowed) {
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+
+ final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors);
+ baseDataValidator.reset().parameter(pageNo).value(queryParams.get(pageNo)).notNull().throwValidationErrors();
+
+ baseDataValidator.reset().parameter(paginationOrderBy).value(queryParams.get(paginationOrderBy)).ignoreIfNull()
+ .matchesRegularExpression(ORDER_BY_REGEX_PATTERN).throwValidationErrors();
+ Integer pageSize = this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
+
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sql, null, new ReportMapper(sql));
+ Integer pageNo = Integer.parseInt(queryParams.get(ReportingConstants.pageNo));
+
+ pageNo = pageNo * pageSize;
+ sql = sql + " order by " + queryParams.get(paginationOrderBy) + " limit " + pageSize + " offset " + pageNo;
Review Comment:
done
--
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] fynmanoj commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
fynmanoj commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r863021056
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/common/GlobalConfigurationHelper.java:
##########
@@ -99,8 +99,8 @@ public static void verifyAllDefaultGlobalConfigurations(final RequestSpecificati
ArrayList<HashMap> actualGlobalConfigurations = getAllGlobalConfigurations(requestSpec, responseSpec);
// There are currently 37 global configurations.
- Assertions.assertEquals(38, expectedGlobalConfigurations.size());
- Assertions.assertEquals(38, actualGlobalConfigurations.size());
+ Assertions.assertEquals(39, expectedGlobalConfigurations.size());
+ Assertions.assertEquals(39, actualGlobalConfigurations.size());
Review Comment:
this also requires updating the Map for teardown. in `getAllDefaultGlobalConfigurations` method of `GlobalConfigurationHelper`
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/GenericResultsetData.java:
##########
@@ -41,6 +46,14 @@ public List<ResultsetRowData> getData() {
return this.data;
}
+ public void setReportSize(Integer totalReportSize) {
Review Comment:
Let the setter name be the same as the variable. Easier to read code.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -162,9 +180,25 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
-
- final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql);
+ String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final GenericResultsetData result;
+ Boolean paginationAllowed = BooleanUtil.parseBoolean(queryParams.get(ReportingConstants.paginationAllowed));
+
+ if (paginationAllowed) {
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+ final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors);
+ baseDataValidator.reset().parameter(pageNo).value(queryParams.get(pageNo)).notNull().throwValidationErrors();
+ Integer pageContent = this.configurationDomainService.pageContentLimit();
+ Page<GenericResultsetData> reportData = this.paginationHelper.fetchPage(this.jdbcTemplate, sql, null, new ReportMapper(sql));
+ Integer pageNo = Integer.parseInt(queryParams.get(ReportingConstants.pageNo));
+ pageNo = pageNo * pageContent;
+ sql = sql + " order by " + queryParams.get(paginationOrderBy) + " limit " + pageContent + " offset " + pageNo;
Review Comment:
validate `paginationOrderBy`.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/domain/ConfigurationDomainServiceJpa.java:
##########
@@ -220,6 +220,16 @@ public int getRoundingMode() {
return defaultValue;
}
+ @Override
+ public Integer pageContentLimit() {
+ final String propertyName = "page-content-limit";
Review Comment:
Use more descriptive config name here . sample :"reports-pagination-number-of-items-per-page"
--
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] github-actions[bot] commented on pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #2310:
URL: https://github.com/apache/fineract/pull/2310#issuecomment-1166691808
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.
--
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 closed pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
galovics closed pull request #2310: FINERACT-1609: Added Pagination to reports
URL: https://github.com/apache/fineract/pull/2310
--
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 #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
vidakovic commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r863052590
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/GenericResultsetData.java:
##########
@@ -27,10 +27,15 @@ public final class GenericResultsetData {
private final List<ResultsetColumnHeaderData> columnHeaders;
private final List<ResultsetRowData> data;
+ private Integer totalReportSize;
Review Comment:
... the class is called "GenericResultsetData"... is it then really necessary to have the word "report" in this variable? We could name things like in Spring Data... just a suggestion.
--
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] logoutdhaval closed pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval closed pull request #2310: FINERACT-1609: Added Pagination to reports
URL: https://github.com/apache/fineract/pull/2310
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864041041
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL = "/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL = "/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec, this.responseSpec);
+
+ final ResponseSpecification errorResponse = new ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec, this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" + Utils.TENANT_IDENTIFIER + "&R_officeId=1";
+ LinkedHashMap reportData = this.stretchyReportHelper.getStretchyReportDetail(this.requestSpec, this.responseSpec, url, "");
+ ArrayList<String> rdata = (ArrayList<String>) reportData.get("data");
+ Integer reportDataSize = rdata.size();
+ Assertions.assertNotNull(reportDataSize);
+
+ Boolean isPaginationAllowed = true;
+ if (isPaginationAllowed) {
+ Integer pageNo = 0;
+ Integer orderBy = 1;
+ Integer pageCount = 0;
+ Integer pageContent = Math.toIntExact(
+ GlobalConfigurationHelper.getGlobalConfigurationByName(requestSpec, responseSpec, "page-content-limit").getValue());
Review Comment:
resolved
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864040824
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL = "/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL = "/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec, this.responseSpec);
+
+ final ResponseSpecification errorResponse = new ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec, this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" + Utils.TENANT_IDENTIFIER + "&R_officeId=1";
+ LinkedHashMap reportData = this.stretchyReportHelper.getStretchyReportDetail(this.requestSpec, this.responseSpec, url, "");
+ ArrayList<String> rdata = (ArrayList<String>) reportData.get("data");
+ Integer reportDataSize = rdata.size();
+ Assertions.assertNotNull(reportDataSize);
+
+ Boolean isPaginationAllowed = true;
+ if (isPaginationAllowed) {
Review Comment:
This test case is for paginationAllowed only.
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864043266
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/common/GlobalConfigurationHelper.java:
##########
@@ -99,8 +99,8 @@ public static void verifyAllDefaultGlobalConfigurations(final RequestSpecificati
ArrayList<HashMap> actualGlobalConfigurations = getAllGlobalConfigurations(requestSpec, responseSpec);
// There are currently 37 global configurations.
Review Comment:
done
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864035043
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/GenericDataServiceImpl.java:
##########
@@ -88,7 +88,7 @@ public GenericResultsetData fillGenericResultSet(final String sql) {
resultsetDataRows.add(resultsetDataRow);
}
- return new GenericResultsetData(columnHeaders, resultsetDataRows);
+ return new GenericResultsetData(columnHeaders, resultsetDataRows, null, null);
Review Comment:
done
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864038430
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/report/service/ReportingProcessService.java:
##########
@@ -39,6 +43,21 @@ default Map<String, String> getReportParams(final MultivaluedMap<String, String>
SQLInjectionValidator.validateSQLInput(pValue);
reportParams.put(pKey, pValue);
}
+ if (k.equalsIgnoreCase(isPaginationAllowed)) {
Review Comment:
done
##########
integration-tests/src/test/java/org/apache/fineract/integrationtest/stretchyreports/StretchyReportHelper.java:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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.integrationtest.stretchyreports;
+
+import com.google.gson.Gson;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportHelper {
+
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+
+ private String nameOfReport = "report_test";
+ private String reportType = "Table";
+ private String reportSQL = null;
+ private String useReport = null;
+ List<String> reportParameters = Collections.<String>emptyList();
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportHelper.class);
Review Comment:
done.
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864038167
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java:
##########
@@ -1315,7 +1315,7 @@ public GenericResultsetData retrieveDataTableGenericResultSet(final String dataT
final List<ResultsetRowData> result = fillDatatableResultSetDataRows(sql);
- return new GenericResultsetData(columnHeaders, result);
+ return new GenericResultsetData(columnHeaders, result, null, null);
Review Comment:
corrected.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReportingConstants.java:
##########
@@ -0,0 +1,27 @@
+/**
+ * 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.dataqueries.service;
+
+@SuppressWarnings({ "HideUtilityClassConstructor" })
+public class ReportingConstants {
+
+ public static final String pageNo = "pageNo";
Review Comment:
done
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864037922
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -162,9 +181,33 @@ public GenericResultsetData retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
- final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport);
+ final GenericResultsetData result;
+ Boolean paginationAllowed = BooleanUtil.parseBoolean(queryParams.get(ReportingConstants.isPaginationAllowed));
Review Comment:
done
--
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 pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
vidakovic commented on PR #2310:
URL: https://github.com/apache/fineract/pull/2310#issuecomment-1115901534
Same here... left already one comment, but might have more... not entirely sure if we are reinventing something.
--
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] logoutdhaval commented on a diff in pull request #2310: FINERACT-1609: Added Pagination to reports
Posted by GitBox <gi...@apache.org>.
logoutdhaval commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864039892
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG = LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL = "/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL = "/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec, this.responseSpec);
+
+ final ResponseSpecification errorResponse = new ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec, this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" + Utils.TENANT_IDENTIFIER + "&R_officeId=1";
Review Comment:
added to helper class
--
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