Us add payment entity 2 #48

jluisbn · 2023-08-15T17:29:00Z

No description provided.

jluisbn · 2023-08-15T17:31:24Z

Screenshot for existing user and existing payment

Screenshot for existing user but no payment found

Screenshot for user not found

wheeleruniverse · 2023-08-17T15:57:29Z

src/main/java/com/bravo/user/controller/PaymentController.java

+    private final PaymentService paymentService;
+    private final UserValidator userValidator;
+
+    public PaymentController(PaymentService paymentService, UserValidator userValidator) {


Do you know about @RequiredArgsConstructor?

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

wheeleruniverse · 2023-08-17T15:58:06Z

src/main/java/com/bravo/user/controller/PaymentController.java

+    @ResponseBody
+    public List<PaymentDto> retrievePaymentsByUser(final @PathVariable String userId) {
+
+        log.info("Retrieving payments for user Id {}", userId);


Do you think this log belongs after the validation?

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

wheeleruniverse · 2023-08-17T15:59:05Z

src/main/java/com/bravo/user/dao/repository/PaymentRepository.java

+@Repository
+public interface PaymentRepository extends JpaRepository<Payment, String> {
+
+    List<Payment> findAllByUserId(String userId);


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

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.

wheeleruniverse · 2023-08-17T16:00:07Z

src/main/java/com/bravo/user/service/PaymentService.java

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


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.

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

wheeleruniverse · 2023-08-17T16:01:31Z

src/main/java/com/bravo/user/service/PaymentService.java

+    public List<PaymentDto> retrieveByUserId(String userId) {
+        log.info("Retrieving all payments by user Id");
+
+        userRepository.findById(userId).orElseThrow(() -> new DataNotFoundException("User Id does not exist. Please validate input"));


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?

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.

wheeleruniverse · 2023-08-17T16:02:02Z

src/main/resources/data.sql

+('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'});


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

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

wheeleruniverse · 2023-08-17T16:06:42Z

src/test/java/com/bravo/user/controller/PaymentControllerTest.java

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


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/

You are right. Will keep that in mind.

wheeleruniverse · 2023-08-17T16:10:01Z

src/test/java/com/bravo/user/service/PaymentServiceTest.java

+    private final UserRepository userRepository = mock(UserRepository.class);
+    private final ResourceMapper resourceMapper = mock(ResourceMapper.class);
+
+    private final PaymentService underTest = new PaymentService(


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

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

wheeleruniverse · 2023-08-17T16:12:28Z

src/test/java/com/bravo/user/service/PaymentServiceTest.java

+        final String userId = "user-id";
+
+        when(userRepository.findById(userId)).thenReturn(Optional.of(TestUtil.getUserData()));
+        when(paymentRepository.findAllByUserId(userId)).thenReturn(TestUtil.getPayments());


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);

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.

wheeleruniverse · 2023-08-17T16:13:16Z

src/test/java/com/bravo/user/service/PaymentServiceTest.java

+        List<PaymentDto> payments = underTest.retrieveByUserId(userId);
+
+        assertNotNull(payments);
+        assertEquals(payments.size(), 0);


What about assertEmpty?

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

wheeleruniverse · 2023-08-17T16:17:48Z

src/test/java/com/bravo/user/util/TestUtil.java

+        userDto.setLastName("Ford");
+        userDto.setPhoneNumber("");
+
+        return new User(userDto);


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.

Yes, I missed that one.

wheeleruniverse · 2023-08-17T16:19:36Z

Thank you so much! I left some comments on the PR for your benefit. We don't expect any additional commits so don't feel obligated to do so. Let me know if you have any questions or comments. Please respond to the questions I left in your code if you wouldn't mind. I would love to get your thoughts.

jluisbn added 2 commits August 15, 2023 11:01

Added payment logic and added payment data

a1a4452

Added unit tests for payment controller and payment service

8214028

wheeleruniverse reviewed Aug 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Us add payment entity 2 #48

Us add payment entity 2 #48

jluisbn commented Aug 15, 2023

jluisbn commented Aug 15, 2023

wheeleruniverse Aug 17, 2023

jluisbn Aug 18, 2023

wheeleruniverse Aug 17, 2023

jluisbn Aug 18, 2023

wheeleruniverse Aug 17, 2023

jluisbn Aug 18, 2023

wheeleruniverse Aug 17, 2023

jluisbn Aug 18, 2023

wheeleruniverse Aug 17, 2023

jluisbn Aug 18, 2023

wheeleruniverse Aug 17, 2023

jluisbn Aug 18, 2023

wheeleruniverse Aug 17, 2023

jluisbn Aug 18, 2023

wheeleruniverse Aug 17, 2023

jluisbn Aug 18, 2023

wheeleruniverse Aug 17, 2023 •

edited

Loading

jluisbn Aug 18, 2023

wheeleruniverse Aug 17, 2023

jluisbn Aug 18, 2023

wheeleruniverse Aug 17, 2023

jluisbn Aug 18, 2023

wheeleruniverse commented Aug 17, 2023

Us add payment entity 2 #48

Are you sure you want to change the base?

Us add payment entity 2 #48

Conversation

jluisbn commented Aug 15, 2023

jluisbn commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wheeleruniverse Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wheeleruniverse commented Aug 17, 2023

wheeleruniverse Aug 17, 2023 •

edited

Loading