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

Us add payment entity 2 #48

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/main/java/com/bravo/user/controller/PaymentController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package com.bravo.user.controller;

import com.bravo.user.model.dto.PaymentDto;
import com.bravo.user.service.PaymentService;
import com.bravo.user.validator.UserValidator;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.tags.Tag;
import lombok.extern.slf4j.Slf4j;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.ResponseBody;
import org.springframework.web.bind.annotation.RestController;

import java.util.List;

@RestController
@Tag(name = "Payment", description = "User payments")
@RequestMapping(value = "/payment")
@Slf4j
public class PaymentController {

private final PaymentService paymentService;
private final UserValidator userValidator;

public PaymentController(PaymentService paymentService, UserValidator userValidator) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you know about @RequiredArgsConstructor?

Copy link
Author

Choose a reason for hiding this comment

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

I do, I just forgot, and also tried to follow UserController convention as requested.

this.paymentService = paymentService;
this.userValidator = userValidator;
}

@Operation(summary = "Retrieves a list of payments by User")
@GetMapping("/retrieve/{userId}")
@ResponseBody
public List<PaymentDto> retrievePaymentsByUser(final @PathVariable String userId) {

log.info("Retrieving payments for user Id {}", userId);
Copy link
Member

Choose a reason for hiding this comment

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

Do you think this log belongs after the validation?

Copy link
Author

Choose a reason for hiding this comment

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

For sure, I just think I like to keep trail of everithing that happens, so it is easier to debug in my opinion.


userValidator.validateId(userId);
return paymentService.retrieveByUserId(userId);
}
}
13 changes: 13 additions & 0 deletions src/main/java/com/bravo/user/dao/repository/PaymentRepository.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.bravo.user.dao.repository;

import com.bravo.user.dao.model.Payment;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.stereotype.Repository;

import java.util.List;

@Repository
public interface PaymentRepository extends JpaRepository<Payment, String> {

List<Payment> findAllByUserId(String userId);
Copy link
Member

Choose a reason for hiding this comment

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

How is this different than findByUserId? Does findByIgnoreCase make sense here?

Copy link
Author

Choose a reason for hiding this comment

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

I just use that more descriptive even though it is the same result as byUserId. And about the ignoreCase, I think we should look for exact keys, and also we could face some performance issues if we start doing searches with ignore case, let's say we have an index in the userId, as far as I am concerned this type of searches would ignore the index and probably be an slow query.

}
37 changes: 37 additions & 0 deletions src/main/java/com/bravo/user/service/PaymentService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package com.bravo.user.service;

import com.bravo.user.dao.model.Payment;
import com.bravo.user.dao.model.mapper.ResourceMapper;
import com.bravo.user.dao.repository.PaymentRepository;
import com.bravo.user.dao.repository.UserRepository;
import com.bravo.user.exception.DataNotFoundException;
import com.bravo.user.model.dto.PaymentDto;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Service;

import java.util.List;

@Service
@Slf4j
public class PaymentService {

private final PaymentRepository paymentRepository;
private final UserRepository userRepository;
private final ResourceMapper mapper;

public PaymentService(PaymentRepository paymentRepository, UserRepository userRepository, ResourceMapper mapper) {
this.paymentRepository = paymentRepository;
this.userRepository = userRepository;
this.mapper = mapper;
}

public List<PaymentDto> retrieveByUserId(String userId) {
log.info("Retrieving all payments by user Id");
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make sense to have a log at the Controller and Service layers. You are not passing the userId into the log message.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, I just think I like to keep trail of everithing that happens, so it is easier to debug in my opinion.


userRepository.findById(userId).orElseThrow(() -> new DataNotFoundException("User Id does not exist. Please validate input"));
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to declare a existsById query instead of doing a search to throw away the result. exists or count would be more efficient on the database. Alternatively do we need to check at all? Is it fine to return 0 payments when the userId provided does not exist?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you are right, maybe I am to defensive so if something does not look right I rather throw an exception, and let the user give it another try.

final List<Payment> payments = paymentRepository.findAllByUserId(userId);

log.info("Found {} payments for user {}", payments.size(), userId);
return mapper.convertPayments(payments);
}
}
8 changes: 8 additions & 0 deletions src/main/resources/data.sql
Original file line number Diff line number Diff line change
Expand Up @@ -646,3 +646,11 @@ insert into address (id, user_id, line1, line2, city, state, zip) values
('42f33d30-f3f8-4743-a94e-4db11fdb747d', '008a4215-0b1d-445e-b655-a964039cbb5a', '412 Maple St', null, 'Dowagiac', 'Michigan', '49047'),
('579872ec-46f8-46b5-b809-d0724d965f0e', '00963d9b-f884-485e-9455-fcf30c6ac379', '237 Mountain Ter', 'Apt 10', 'Odenville', 'Alabama', '35120'),
('95a983d0-ba0e-4f30-afb6-667d4724b253', '00963d9b-f884-485e-9455-fcf30c6ac379', '107 Annettes Ct', null, 'Aydlett', 'North Carolina', '27916');

insert into payment (id, user_id, card_number, expiry_month, expiry_year, updated) values
('15040055-a640-4cea-cfb7-357669fcdefe', 'fd6d21f6-f1c2-473d-8ed7-f3f9c7550cc9', '7219038435696266', 8, 2028, {ts '2023-07-01 07:20:16.267'}),
('751c48b7-bbe4-4e94-bacb-31eb81fd2c12', 'f9096b14-daab-430f-835b-2f0d651e3559', '4390580235433205', 9, 2027, {ts '2022-02-11 09:55:48.371'}),
('6033d04d-c602-4354-a411-d376a9215c68', 'f8bbbc7a-867b-4e8d-a2a5-8b7c07a4589c', '3997869182123667', 1, 2024, {ts '2020-11-12 11:22:15.264'}),
('dd52bd06-d337-4624-d9bf-9593f2cff4c1', '06c70a5b-1950-4765-81b6-4bb2378e0991', '8439375960556557', 2, 2026, {ts '2018-05-11 17:22:15.125'}),
('1628a7c9-ed7e-4582-eac6-b3321c2fdbe1', 'd54b6b5d-de10-4c06-9837-c7457937daf8', '5929456670566065', 3, 2025, {ts '2019-04-26 06:55:27.748'}),
('6931d250-f972-4c5b-fc65-85904473de56', 'f8160123-1766-41c7-8056-2acbfd3c4f2a', '2981890323295572', 7, 2024, {ts '2022-08-08 09:11:52.428'});
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this syntax? {ts '2023-07-01 07:20:16.267'} for the updated field?

Copy link
Author

Choose a reason for hiding this comment

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

I thought it was just a timestamp as when the user updated their information. Probably should I paid more attention to it.

79 changes: 79 additions & 0 deletions src/test/java/com/bravo/user/controller/PaymentControllerTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package com.bravo.user.controller;

import com.bravo.user.App;
import com.bravo.user.service.PaymentService;
import com.bravo.user.util.TestUtil;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit.jupiter.SpringExtension;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.MvcResult;

import java.util.Collections;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.when;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

@ContextConfiguration(classes = {App.class})
@ExtendWith(SpringExtension.class)
@SpringBootTest()
@AutoConfigureMockMvc
class PaymentControllerTest {

@Autowired
private MockMvc mockMvc;

@MockBean
private PaymentService paymentService;

@Test
void retrievePaymentsByUser() throws Exception {

final String userId = "user-id";

when(paymentService.retrieveByUserId(userId)).thenReturn(TestUtil.getPaymentsDto());

final MvcResult result = this.mockMvc
.perform(get("/payment/retrieve/" + userId))
.andExpect(status().isOk())
.andReturn();

String responseBody = result.getResponse().getContentAsString();

assertNotNull(result);
assertNotNull(responseBody);
assertTrue(responseBody.contains("\"id\" : \"15040055-a640-4cea-cfb7-357669fcdefe\""));
Copy link
Member

Choose a reason for hiding this comment

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

You can chain the .getResponse().getContentAsString() onto result since the result variable is only used that 1 time.

final String responseBody = this.mockMvc
                .perform(get("/payment/retrieve/" + userId))
                .andExpect(status().isOk())
                .andReturn()
                .getResponse()
                .getContentAsString();
  • Why is the responseBody variable not final?
  • The assertNotNull(result) is not necessary because the creation of responseBody would cause a NPE
  • The assertNotNull(responseBody) is not necessary because the assertTrue would cause a NPE if the responseBody is null and the test will fail.

Have you heard of jsonpath? https://www.petrikainulainen.net/programming/spring-framework/integration-testing-of-spring-mvc-applications-write-clean-assertions-with-jsonpath/

Copy link
Author

Choose a reason for hiding this comment

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

You are right. Will keep that in mind.


}

@Test
void retrievePaymentsByUserEmptyPayments() throws Exception {

final String userId = "user-id";

when(paymentService.retrieveByUserId(userId)).thenReturn(Collections.emptyList());

final MvcResult result = this.mockMvc
.perform(get("/payment/retrieve/" + userId))
.andExpect(status().isOk())
.andReturn();

String responseBody = result.getResponse().getContentAsString();
assertNotNull(result);
assertEquals(responseBody, "[ ]");
}

@Test
void retrievePaymentsByUserNotFound() throws Exception {
this.mockMvc.perform(get("/payment/retrieve/")).andExpect(status().isNotFound());
}
}
78 changes: 78 additions & 0 deletions src/test/java/com/bravo/user/service/PaymentServiceTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package com.bravo.user.service;

import com.bravo.user.App;
import com.bravo.user.dao.model.mapper.ResourceMapper;
import com.bravo.user.dao.repository.PaymentRepository;
import com.bravo.user.dao.repository.UserRepository;
import com.bravo.user.exception.DataNotFoundException;
import com.bravo.user.model.dto.PaymentDto;
import com.bravo.user.util.TestUtil;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit.jupiter.SpringExtension;


import java.util.Collections;
import java.util.List;
import java.util.Optional;

import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

@ContextConfiguration(classes = {App.class})
@ExtendWith(SpringExtension.class)
@SpringBootTest()
class PaymentServiceTest {

private final PaymentRepository paymentRepository = mock(PaymentRepository.class);
private final UserRepository userRepository = mock(UserRepository.class);
private final ResourceMapper resourceMapper = mock(ResourceMapper.class);

private final PaymentService underTest = new PaymentService(
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using mock and constructor instead of @MockBean, @Mock, and @InjectMocks?

Copy link
Author

Choose a reason for hiding this comment

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

This is just the way I have been doing this. But I think using annotations is a cleaner way.

paymentRepository,
userRepository,
resourceMapper
);

@Test
void retrieveByUserId() {
final String userId = "user-id";

when(userRepository.findById(userId)).thenReturn(Optional.of(TestUtil.getUserData()));
when(paymentRepository.findAllByUserId(userId)).thenReturn(TestUtil.getPayments());
Copy link
Member

@wheeleruniverse wheeleruniverse Aug 17, 2023

Choose a reason for hiding this comment

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

Is this better than a looser matcher and a verify? Do you know the difference?

when(userRepository.findById(anyString())).thenReturn(Optional.of(TestUtil.getUserData()));

...

verify(userRepository).findById(userId);

Copy link
Author

Choose a reason for hiding this comment

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

In my opinion that depends, because if you have a more complex method then you have to mock every call or access to their properties, but if you send a complete object (not a mock) then your code will not throw a NPE, and instead of having many "when then" I think it is a bit cleaner to have real objects.
And about the verify, I forgot to add that, I tend to always add it.

when(resourceMapper.convertPayments(TestUtil.getPayments())).thenReturn(TestUtil.getPaymentsDto());

List<PaymentDto> payments = underTest.retrieveByUserId(userId);

assertNotNull(payments);
assertEquals(payments.size(), 1);
}

@Test
void retrieveByUserIdNotPaymentsFound() {
final String userId = "user-id";

when(userRepository.findById(userId)).thenReturn(Optional.of(TestUtil.getUserData()));
when(paymentRepository.findAllByUserId(userId)).thenReturn(Collections.emptyList());
when(resourceMapper.convertPayments(Collections.emptyList())).thenReturn(Collections.emptyList());

List<PaymentDto> payments = underTest.retrieveByUserId(userId);

assertNotNull(payments);
assertEquals(payments.size(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

What about assertEmpty?

Copy link
Author

Choose a reason for hiding this comment

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

I have not used that one, and since I tend to use Sonarlint to avoid this code smells, Intellij did not suggest that method.

}

@Test
void retrieveByUserIdNotFound() {
final String userId = "Not found";

when(userRepository.findById(userId)).thenThrow(DataNotFoundException.class);

DataNotFoundException thrown = assertThrows(DataNotFoundException.class, () -> underTest.retrieveByUserId(userId));

assertNotNull(thrown);
}
}
49 changes: 49 additions & 0 deletions src/test/java/com/bravo/user/util/TestUtil.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package com.bravo.user.util;

import com.bravo.user.dao.model.Payment;
import com.bravo.user.dao.model.User;
import com.bravo.user.model.dto.PaymentDto;
import com.bravo.user.model.dto.UserSaveDto;

import java.time.LocalDateTime;
import java.util.List;

public class TestUtil {

public static User getUserData() {

UserSaveDto userDto = new UserSaveDto();

userDto.setFirstName("Harrison");
userDto.setMiddleName("");
userDto.setLastName("Ford");
userDto.setPhoneNumber("");

return new User(userDto);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you create UserSaveDto instead of User directly? These variables can be final and if you use @Builder you can remove the variable altogether.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I missed that one.

}

public static List<Payment> getPayments() {
Payment payment = new Payment();

payment.setId("15040055-a640-4cea-cfb7-357669fcdefe");
payment.setUserId("fd6d21f6-f1c2-473d-8ed7-f3f9c7550cc9");
payment.setCardNumber("4790580235733205");
payment.setExpiryMonth(8);
payment.setExpiryYear(2028);
payment.setUpdated(LocalDateTime.of(2023, 1, 25, 1, 25, 0));

return List.of(payment);
}

public static List<PaymentDto> getPaymentsDto() {
PaymentDto paymentDto = new PaymentDto();

paymentDto.setId("15040055-a640-4cea-cfb7-357669fcdefe");
paymentDto.setExpiryMonth(8);
paymentDto.setExpiryYear(2028);
paymentDto.setUpdated(LocalDateTime.of(2023, 1, 25, 1, 25, 0));
paymentDto.setCardNumberLast4("3205");

return List.of(paymentDto);
}
}