[GitHub] [fineract] vorburger commented on a change in pull request #1100: FINERACT-1047 Integration test for Audit trails(Create)

[GitBox <gi...@apache.org>]

vorburger commented on a change in pull request #1100:
URL: https://github.com/apache/fineract/pull/1100#discussion_r444075740



##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/common/AuditHelper.java
##########
@@ -0,0 +1,49 @@
+/**
+ * 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.common;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ *
+ * @author Manthan Surkar
+ *
+ */
+
+public class AuditHelper {
+
+    private static final Logger LOG = LoggerFactory.getLogger(AuditHelper.class);
+    private static final String AUDIT_BASE_URL = "/fineract-provider/api/v1/audits?" + Utils.TENANT_IDENTIFIER;
+
+    public static void verifyAuditCreatedOnServer(final RequestSpecification requestSpec, final ResponseSpecification responseSpec,
+            final Integer resourceId, final String actionName, final String entityName) {
+        LOG.info("------------------------------CHECK IF AUDIT CREATED------------------------------------\n");
+        final String AUDIT_URL = "/fineract-provider/api/v1/audits/?" + "entityName=" + entityName + "&resourceId=" + resourceId
+                + "&actionName=" + actionName + "&" + Utils.TENANT_IDENTIFIER;
+        final ArrayList<Integer> responseAuditIDs = Utils.performServerGet(requestSpec, responseSpec, AUDIT_URL, "id");

Review comment:
       very minor feedback FYI, more for your learning: We usually use the abstract `List` instead of the concrete `ArrayList` implementation as type of the variable. Even better, since we are now on Java 11, why not start using `var` instead? Try just `var responseAuditIDs = Utils.performServerGet(requestSpec, responseSpec, AUDIT_URL, "id");` (and don't use `final var`, that's just ugly).
   
   ```suggestion
           final List<Integer> responseAuditIDs = Utils.performServerGet(requestSpec, responseSpec, AUDIT_URL, "id");
   ```

##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/AuditIntegrationTest.java
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 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.HashMap;
+import org.apache.fineract.integrationtests.common.AuditHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.OfficeHelper;
+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;
+
+/**
+ *
+ * @author Manthan Surkar
+ *
+ */
+public class AuditIntegrationTest {
+
+    private ResponseSpecification responseSpec;
+    private RequestSpecification requestSpec;
+    private ClientHelper clientHelper;
+
+    /**
+     * Sets up the essential settings for the TEST like contentType,
+     * expectedStatusCode. It uses the '@BeforeEach' annotation provided by
+     * jUnit.
+     */
+    @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();
+    }
+
+    /**
+     * Here we Create/Update different Entities and verify an audit is generated
+     * for each action. This can be further extened with more entities and
+     * actions in similiar way.
+     */
+    @Test
+    public void auditShouldbeCreated() {
+        this.clientHelper = new ClientHelper(this.requestSpec, this.responseSpec);
+
+        // When Client is created
+        final Integer clientId = ClientHelper.createClient(this.requestSpec, this.responseSpec);
+        Assertions.assertNotNull(clientId);
+        AuditHelper.verifyAuditCreatedOnServer(this.requestSpec, this.responseSpec, clientId, "CREATE", "CLIENT");

Review comment:
       For a new class like `AuditHelper`, I would go with the pattern of passing `this.requestSpec, this.responseSpec` to the constructor, and then have a method like `verifyAuditCreatedOnServer` not require it. Actually, even better, how about only passing the `requestSpec` and even just hard-code the `responseSpec` without having to pass it. This would make ITs more readable.
   
   ```suggestion
           AuditHelper.verifyAuditCreatedOnServer(this.requestSpec, this.responseSpec, clientId, "CREATE", "CLIENT");
   ```

##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/AuditIntegrationTest.java
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 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.HashMap;
+import org.apache.fineract.integrationtests.common.AuditHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.OfficeHelper;
+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;
+
+/**
+ *
+ * @author Manthan Surkar
+ *
+ */
+public class AuditIntegrationTest {
+
+    private ResponseSpecification responseSpec;
+    private RequestSpecification requestSpec;
+    private ClientHelper clientHelper;
+
+    /**
+     * Sets up the essential settings for the TEST like contentType,
+     * expectedStatusCode. It uses the '@BeforeEach' annotation provided by
+     * jUnit.
+     */
+    @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();
+    }
+
+    /**
+     * Here we Create/Update different Entities and verify an audit is generated
+     * for each action. This can be further extened with more entities and
+     * actions in similiar way.
+     */
+    @Test
+    public void auditShouldbeCreated() {
+        this.clientHelper = new ClientHelper(this.requestSpec, this.responseSpec);
+
+        // When Client is created
+        final Integer clientId = ClientHelper.createClient(this.requestSpec, this.responseSpec);

Review comment:
       as we've started discussing in #1019, it's actually kind of very wrong and confusing how classes such as `ClientHelper` and many other existing *Helper, take `this.requestSpec, this.responseSpec` BOTH as constructor arguments AND for their methods - that makes no sense; I should be either/or. That's not really the point of this PR though - just thought I'd let you know here for illustration.




----------------------------------------------------------------
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:
users@infra.apache.org