vorburger commented on a change in pull request #1416:
URL: https://github.com/apache/fineract/pull/1416#discussion_r528218646
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/report/provider/ReportingProcessServiceProvider.java
##########
@@ -20,66 +20,39 @@
import java.util.Collection;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
-import org.apache.commons.lang3.ArrayUtils;
-import org.apache.fineract.infrastructure.report.annotation.ReportService;
-import
org.apache.fineract.infrastructure.report.service.ReportingProcessService;
+import org.apache.fineract.api.ReportingProcessService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import org.springframework.beans.BeansException;
-import org.springframework.context.ApplicationContext;
-import org.springframework.context.ApplicationContextAware;
+import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Scope;
import org.springframework.stereotype.Component;
@Component
@Scope("singleton")
-public class ReportingProcessServiceProvider implements
ApplicationContextAware {
+public class ReportingProcessServiceProvider {
Review comment:
I have integrated the changes you proposed for
`ReportingProcessServiceProvider` into my #1368 already, so that in this PR you
don't have to change this class anymore, hope you don't mind? (It's not a
verbatim copy/paste but inspired by your changes here.)
##########
File path:
fineract-api/src/main/java/org/apache/fineract/api/ReportingProcessService.java
##########
@@ -0,0 +1,55 @@
+/**
+ * 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.api;
+
+import java.io.OutputStream;
+import java.util.Map;
+
+public interface ReportingProcessService {
+
+ enum ReportType {
+
+ HTML("text/html"), PDF("application/pdf"),
XLS("application/vnd.ms-excel"), XLSX(
+
"application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"),
CSV("text/csv");
+
+ ReportType(String contentType) {
+ this.contentType = contentType;
+ }
+
+ private final String contentType;
+
+ public String getContentType() {
+ return contentType;
+ }
+ }
+
+ String PARAM_OUTPUT_TYPE = "output-type";
+ String PARAM_LOCALE = "locale";
+ String PARAM_TENANT_URL = "tenantUrl";
+ String PARAM_USER_HIERARCHIE = "userhierarchy";
+ String PARAM_USERNAME = "username";
+ String PARAM_PASSWORD = "password";
+ String PARAM_USER_ID = "userid";
+ String PARAM_DATABASE_ULR = "databaseUrl";
+ String PARAMETER_PREFIX = "R_";
+
+ void process(String name, Map<String, String> parameter, OutputStream os);
Review comment:
As mentioned in FINERACT-1264, I would suggest that we use a signature
like this instead (we should definitely get rid of the
`javax.ws.rs.core.Response` which we currently use, that's too much "web
specific", and doesn't belong here):
```suggestion
ByteSource process(String name, MediaType format, Map<String, String>
parameters);
```
##########
File path:
fineract-api/src/main/java/org/apache/fineract/api/exception/AbstractApiException.java
##########
@@ -0,0 +1,86 @@
+/**
+ * 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.api.exception;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Exception with internationalization support (it's message can be
translated).
+ */
+public abstract class AbstractApiException extends RuntimeException {
Review comment:
Copying (forking) instead of just moving existing Exception types is
going to cause issues. I don't think this is a good idea. I would EITHER just
move them, keeping the same package, or (better?) not have them in the API, for
now. The `PentahoReportingProcessServiceImpl` can just throw some more generic
exception?
##########
File path:
fineract-api/src/main/java/org/apache/fineract/api/ReportingProcessService.java
##########
@@ -0,0 +1,55 @@
+/**
+ * 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.api;
+
+import java.io.OutputStream;
+import java.util.Map;
+
+public interface ReportingProcessService {
+
+ enum ReportType {
+
+ HTML("text/html"), PDF("application/pdf"),
XLS("application/vnd.ms-excel"), XLSX(
+
"application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"),
CSV("text/csv");
+
+ ReportType(String contentType) {
+ this.contentType = contentType;
+ }
+
+ private final String contentType;
+
+ public String getContentType() {
+ return contentType;
+ }
+ }
+
+ String PARAM_OUTPUT_TYPE = "output-type";
+ String PARAM_LOCALE = "locale";
+ String PARAM_TENANT_URL = "tenantUrl";
+ String PARAM_USER_HIERARCHIE = "userhierarchy";
+ String PARAM_USERNAME = "username";
+ String PARAM_PASSWORD = "password";
+ String PARAM_USER_ID = "userid";
+ String PARAM_DATABASE_ULR = "databaseUrl";
+ String PARAMETER_PREFIX = "R_";
Review comment:
I'm not sure we should make all these part of the Reporting API.
Definitely not as String constants. Perhaps as strongly typed arguments? Better
in a `ReportOptions` kind of wrapper? We should carve out what (1) the first
external
https://github.com/vorburger/fineract-pentaho/blob/develop/src/main/java/org/apache/fineract/infrastructure/report/service/PentahoReportingProcessServiceImpl.java,
(2) the internal `DatatableReportingProcessService` (new in #1368) and (3)
other ones you may have in mind really require... some thoughts:
* The output-type IMHO should be replaced by the `MediaType` argument
* The Locale should better be in some sort of new `UserContext` kind of
object, what to you think?
* userhierarchy could be part of such a `UserContext` perhaps? Also a
`userID` I guess.
* What is PARAMETER_PREFIX = "R_" for now? Won't we pass those as part of
the `Map<String, String> parameters` arg?
* About the TenantDB connection details: We should find some appropriate
existing JDBC class which we can "pass through" for all these. What's type in
JDBC best encapsulates such connection details? I wish we could just pass an
actual `Connection`, but some of these reporting tools will probably want to
connect to the DB themselves? (This is actually not great at all from a
security point of view, I mean the reporting plugin can just query, and write,
whatever it wants - but that's not the point of this PR.)
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/infrastructure/reportmailingjob/service/ReportMailingJobWritePlatformServiceImpl.java
##########
@@ -447,7 +450,18 @@ private StringBuilder generateReportOutputStream(final
ReportMailingJob reportMa
.findReportingProcessService(reportType);
if (reportingProcessService != null) {
- final Response processReport =
reportingProcessService.processRequest(reportName, reportParams);
+ final Map<String, String> params =
toSingleValueMap(reportParams);
Review comment:
This is copy/pasted from above... :crying_cat_face: let's find a better
solution together?
##########
File path:
fineract-api/src/main/java/org/apache/fineract/api/ReportingProcessService.java
##########
@@ -0,0 +1,55 @@
+/**
+ * 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.api;
+
+import java.io.OutputStream;
+import java.util.Map;
+
+public interface ReportingProcessService {
+
+ enum ReportType {
+
+ HTML("text/html"), PDF("application/pdf"),
XLS("application/vnd.ms-excel"), XLSX(
+
"application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"),
CSV("text/csv");
+
+ ReportType(String contentType) {
+ this.contentType = contentType;
+ }
+
+ private final String contentType;
+
+ public String getContentType() {
+ return contentType;
+ }
+ }
+
+ String PARAM_OUTPUT_TYPE = "output-type";
+ String PARAM_LOCALE = "locale";
+ String PARAM_TENANT_URL = "tenantUrl";
+ String PARAM_USER_HIERARCHIE = "userhierarchy";
+ String PARAM_USERNAME = "username";
+ String PARAM_PASSWORD = "password";
+ String PARAM_USER_ID = "userid";
+ String PARAM_DATABASE_ULR = "databaseUrl";
+ String PARAMETER_PREFIX = "R_";
+
+ void process(String name, Map<String, String> parameter, OutputStream os);
+
+ String getType();
Review comment:
Why add this instead of just using the
`org.apache.fineract.infrastructure.report.annotation.ReportService` annotation
with the `type()` as it's currently?
##########
File path:
fineract-api/src/main/java/org/apache/fineract/api/ReportingProcessService.java
##########
@@ -0,0 +1,55 @@
+/**
+ * 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.api;
+
+import java.io.OutputStream;
+import java.util.Map;
+
+public interface ReportingProcessService {
+
+ enum ReportType {
+
+ HTML("text/html"), PDF("application/pdf"),
XLS("application/vnd.ms-excel"), XLSX(
+
"application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"),
CSV("text/csv");
+
+ ReportType(String contentType) {
+ this.contentType = contentType;
+ }
+
+ private final String contentType;
+
+ public String getContentType() {
+ return contentType;
+ }
+ }
Review comment:
I'm not sure we should make this part of the Reporting plugin API. Isn't
this better up to each Plugin? If anything, we could simply have a `String
contentType` to indicate what the user of this API wants to receive in
response? We could also use a strong type than `String`, there are about 27
types we can choose from, e.g. Spring's `MimeType`, note their `MimeTypeUtils`,
or Spring's `MediaType`, or JAX RS' `MediaType` (but I think this API should
NOT be tied to JAX RS) or Guava's `MediaType` (I personally actually prefer
that last one, Guava).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
