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

Pdf session connection #21

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Pdf session connection #21

merged 5 commits into from
Nov 29, 2023

Conversation

sarinali
Copy link
Collaborator

@sarinali sarinali commented Nov 24, 2023

Description

Created endpoint for linking PDF uploads to the current active session

Type of change

  • New feature (non-breaking change which adds functionality)
2023-11-24.12-03-13.mp4

How Has This Been Tested?

Tested using two JUnit Tests:

@Test
    void testPDFLinkValidSession() throws Exception {
        String currentDirectory = System.getProperty("user.dir");
        String pathName = currentDirectory + "/src/main/java/callhub/connect/pdfs/fall2023.pdf";
        File pdfFile = new File(pathName);
        MockMultipartFile sampleFile = new MockMultipartFile(
                "file",
                pdfFile.getName(),
                "application/pdf",
                new FileInputStream(pdfFile));


        when(documentRepository.save(any())).thenReturn(new FileDocument(
                "MockFile",
                new Binary("sample pdf content".getBytes()),
                LocalDate.now()));

        Session mockSession = new Session(true, "ABCDEF");

        // Mock the behavior of the sessionRepository.save() method
        when(sessionRepository.getSessionsByActiveAndCode(true, "ABCDEF")).thenReturn(mockSession);

        MockMultipartHttpServletRequestBuilder multipartRequest =
                MockMvcRequestBuilders.multipart("/files/session_add_pdf");
        mockMvc.perform(multipartRequest.file(sampleFile).param("session", "ABCDEF").param("name", "MockFile")).andExpect(status().isOk());
    }

    @Test
    void testPDFLinkInvalidSession() throws Exception {
        String currentDirectory = System.getProperty("user.dir");
        String pathName = currentDirectory + "/src/main/java/callhub/connect/pdfs/fall2023.pdf";
        File pdfFile = new File(pathName);
        MockMultipartFile sampleFile = new MockMultipartFile(
                "file",
                pdfFile.getName(),
                "application/pdf",
                new FileInputStream(pdfFile));


        when(documentRepository.save(any())).thenReturn(new FileDocument(
                "MockFile",
                new Binary("sample pdf content".getBytes()),
                LocalDate.now()));

        Session mockSession = new Session(true, "ABCDEF");

        // Mock the behavior of the sessionRepository.save() method
        when(sessionRepository.getSessionsByActiveAndCode(true, "ABCDEF")).thenReturn(mockSession);

        MockMultipartHttpServletRequestBuilder multipartRequest =
                MockMvcRequestBuilders.multipart("/files/session_add_pdf");
        mockMvc.perform(multipartRequest.file(sampleFile).param("session", "123456").param("name", "MockFile")).andExpect(status().isBadRequest());
    }```

Copy link
Member

@3milyfz 3milyfz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

  • Tested locally and works as expected
  • I added a few comments on some changes I would make
  • Most are minor things including missing import statements and potential duplicate code that can be refactored

String currentDirectory = System.getProperty("user.dir");
String pathName = currentDirectory + "/src/main/java/callhub/connect/pdfs/fall2023.pdf";
File pdfFile = new File(pathName);
MockMultipartFile sampleFile = new MockMultipartFile(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplication in Test Setup:

  • There is some duplication in setting up the MockMultipartFile in both test methods.
  • Consider refactoring this into a separate method or a @beforeeach setup method to reduce duplication!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remind me to fix this after i merge

@zjayee
Copy link
Contributor

zjayee commented Nov 25, 2023

Can we wait for clean architecture code to be merged first before merging this code

Copy link
Contributor

@zjayee zjayee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good with some minor comments

@Test
void testPDFLinkValidSession() throws Exception {
String currentDirectory = System.getProperty("user.dir");
String pathName = currentDirectory + "/src/main/java/callhub/connect/pdfs/fall2023.pdf";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this file and create a blank pdf in the test setup for testing instead?

@Test
void testPDFLinkInvalidSession() throws Exception {
String currentDirectory = System.getProperty("user.dir");
String pathName = currentDirectory + "/src/main/java/callhub/connect/pdfs/fall2023.pdf";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here

@sarinali sarinali requested a review from zjayee November 29, 2023 23:31
Copy link
Contributor

@zjayee zjayee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved + future refactoring

@sarinali sarinali merged commit 4e24aae into master Nov 29, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants