Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAT-4687: Implement zip secure file, DAT-4944: Add SpaDeploymentController test coverage #52

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jordanpagewhite
Copy link

Please see JIRA tickets for acceptance criteria:

DAT-4687: Implement zip secure file

In this issue, we are proposing adding the following two dependencies:

    <!-- https://mvnrepository.com/artifact/org.apache.poi/poi -->
    <dependency>
      <groupId>org.apache.poi</groupId>
      <artifactId>poi</artifactId>
      <version>5.2.4</version>
    </dependency>
    <!-- https://mvnrepository.com/artifact/org.apache.poi/poi-ooxml -->
    <dependency>
      <groupId>org.apache.poi</groupId>
      <artifactId>poi-ooxml</artifactId>
      <version>5.2.4</version>
    </dependency>

so that we can use the ZipSecureFile class in place of where we are currently using the ZipFile class. This provides some protection around certain circumstances that I can explain in our meeting tomorrow if you would like. Note that this required a few other, very minor, changes to subsequent lines of code where a certain zip-related class might be changed to the org.apache.commons version.

DAT-4944: Add testing to SpaDeploymentController class

My main/only goal in this issue was to add minimally viable testing to the SpaDeploymentController class. I both wanted to do this because 1) I wanted the class to be tested and 2) I knew that adding tests would be a good way to learn more about the codebase. This adds at least 1 test per method. In those cases, I have tried to add a sufficient test of only the "happy path" of the controller method that might mock non-controller code/dependencies if any exist. We can test those within the context of those classes instead of in the context of this controller class.

As a result of these changes, I am also proposing that we move 2 private, business logic methods: sanity() and requestTagging() from SpaDeploymentController to SPAUploadHandler. Please let me know what you think about this. My main motivation(s) in moving these methods are to 1) keep our controller classes as lean and focused as possible and 2) these types of business logic methods seemed more appropriate in a service/handler class. However, if there is context I am missing surrounding this choice and these methods, please just let me know.

@arkaprovob
Copy link
Contributor

hi @jordanpagewhite I noticed that you moved both methods, requestTagging and sanity, into SPAUploadHandler instead of calling them from the controller. I suggest delegating those tasks to the handleFileUpload method and placing them at the beginning for processing, similar to how it was done in the uploadSPA method. To enhance readability, consider refactoring the parameter of handleFileUpload from Triplet to FormData, as it aligns better with the entire process being handled by SPAUploadHandler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants