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

sean carnahan: 0013 - [backend] add payment method retrieve api #41

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
Binary file added passing-payment-controller-test.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added passing-payment-service-test.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added payment-query-1.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added payment-query-2.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added payment-sql.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
39 changes: 39 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,39 @@
package com.bravo.user.controller;

import com.bravo.user.annotation.SwaggerController;
import com.bravo.user.model.dto.PaymentDto;
import com.bravo.user.service.PaymentService;
import com.bravo.user.validator.UserValidator;

import java.util.List;
import javax.servlet.http.HttpServletResponse;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;

@RequestMapping(value = "/payment")
@SwaggerController
public class PaymentController {

private final PaymentService paymentService;
private final UserValidator userValidator;

public PaymentController(PaymentService paymentService, UserValidator userValidator) {
this.paymentService = paymentService;
this.userValidator = userValidator;
}

@GetMapping(value = "/retrieve/{userId}")
@ResponseBody
public List<PaymentDto> retrieve(
final @PathVariable String userId,
final @RequestParam(required = false) Integer page,
final @RequestParam(required = false) Integer size,
final HttpServletResponse httpResponse
) {
userValidator.validateId(userId);
return paymentService.retrieveByUserId(userId);
}
}
15 changes: 15 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,15 @@
package com.bravo.user.dao.repository;

import java.util.List;

import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.stereotype.Repository;

import com.bravo.user.dao.model.Payment;

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

Choose a reason for hiding this comment

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

This should be extending PagingAndSortingRepository. And the findByUserId() method should include a Pageable parameter.

Copy link
Author

Choose a reason for hiding this comment

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

Hi Darrin,

Good call on the paging. Yeah at first I had it set up with the paging specifications, but then took it out because it didn't look like the requirements asked for paging. So went ahead and added it back in and updated the unit tests. I added the screenshots in the reply for the other comment. Thanks!

Choose a reason for hiding this comment

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

Thanks @seancarnahan !! Yes that's exactly what I was looking for. You took it a step further with JpaSpecificationExecutor. The bottom line was to tell the Repository how to page the results (which can be done in several ways). Looks better, thank u.


List<Payment> findByUserId(final String userId);

}
31 changes: 31 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,31 @@
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.model.dto.PaymentDto;
import java.util.List;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Service;

@Service
public class PaymentService {

private static final Logger LOGGER = LoggerFactory.getLogger(PaymentService.class);

private final PaymentRepository paymentRepository;
private final ResourceMapper resourceMapper;

public PaymentService(PaymentRepository paymentRepository, ResourceMapper resourceMapper) {
this.paymentRepository = paymentRepository;
this.resourceMapper = resourceMapper;
}

public List<PaymentDto> retrieveByUserId(final String userId) {

Choose a reason for hiding this comment

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

This method should accept a Pageable parameter

Copy link
Author

Choose a reason for hiding this comment

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

image

Screen Shot 2022-12-29 at 12 43 20 PM

image

final List<Payment> paymentList = paymentRepository.findByUserId(userId);
LOGGER.info("found {} payment(s)", paymentList.size());

return resourceMapper.convertPayments(paymentList);
}
}
2 changes: 1 addition & 1 deletion src/main/resources/application.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ spring:
password: ${DB_PASSWORD:password}
username: ${DB_USERNAME:sa}
url: ${DB_URL:jdbc:h2:mem:testdb}
h2.console.enabled: false
h2.console.enabled: true

Choose a reason for hiding this comment

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

Like this for the testing version of the appliation.yml. In the root, is this a security issue?

Choose a reason for hiding this comment

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

Agreed. I was thinking this was done to do queries and/or look at the schema. But it should only be done locally when working with the code.

Copy link
Author

Choose a reason for hiding this comment

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

Very true, nice catch. Should not be checked into main branch. Removed

seancarnahan marked this conversation as resolved.
Show resolved Hide resolved
jpa:
database-platform: ${DB_DIALECT:org.hibernate.dialect.H2Dialect}
properties.hibernate:
Expand Down
5 changes: 5 additions & 0 deletions src/main/resources/data.sql
Original file line number Diff line number Diff line change
Expand Up @@ -648,3 +648,8 @@ 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) values
('payment-1', '008a4215-0b1d-445e-b655-a964039cbb5a', 'card-number-1', 12, 2027),

Choose a reason for hiding this comment

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

Is there potential for a card number that is not only digits? I've not seen that.

Choose a reason for hiding this comment

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

LOL. Yeah I noticed these "non-numeric" card #'s as well. Maybe he was just trying to make it obvious that's it's a card #, without needing to look at which column the value is mapped to on above line. My best guess here.

Copy link
Author

Choose a reason for hiding this comment

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

yeah was just trying to make it obvious haha. But it is also a varchar within the schema:

create table payment (
    id varchar(60) primary key,
    user_id varchar(60) not null,
    card_number varchar(16) not null unique,
    expiry_month integer not null,
    expiry_year integer not null,
    updated timestamp not null default current_timestamp()
);

In practice, typically we could get test credit card numbers from creditcard providers.

Choose a reason for hiding this comment

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

Nice. Yeah, I was on a project where we did the actual checking for the number transposing and modulo checks. That's a bit over the top for this :) . Using all the rules here https://www.validcreditcardnumber.com/ or here https://developer.visa.com/capabilities/pav/reference#tag/Payment-Account-Validation-API/operation/Card%20Validation_v1%20-%20Latest. I would've non-varchared it because of potential savings in disk usage. That's likely negligible up to a few million records though. So no biggie here.

('payment-2', '00963d9b-f884-485e-9455-fcf30c6ac379', 'card-number-2', 11, 2026),
('payment-3', '00963d9b-f884-485e-9455-fcf30c6ac379', 'card-number-3', 10, 2025);
84 changes: 84 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,84 @@
package com.bravo.user.controller;

import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.verify;
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.jsonPath;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import org.junit.jupiter.api.BeforeEach;
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.ResultActions;

import com.bravo.user.App;
import com.bravo.user.model.dto.PaymentDto;
import com.bravo.user.service.PaymentService;

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

@Autowired
private MockMvc mockMvc;

@MockBean
private PaymentService paymentService;

private List<PaymentDto> payments;

@BeforeEach
public void beforeEach() {
final List<Integer> ids = IntStream.range(1, 10).boxed().collect(Collectors.toList());

this.payments = ids.stream().map(id -> createPaymentDto(Integer.toString(id)))
.collect(Collectors.toList());
}

@Test
void getRetrieveByUserId() throws Exception {
final String userId = "123a-456b";

when(paymentService.retrieveByUserId(anyString())).thenReturn(payments);

final ResultActions result = this.mockMvc.perform(get("/payment/retrieve/".concat(userId)))
.andExpect(status().isOk());

for (int i = 0; i < payments.size(); i++) {
result.andExpect(jsonPath(String.format("$[%d].id", i)).value(payments.get(i).getId()));
}

verify(paymentService).retrieveByUserId(userId);
}

@Test
void getRetrieveByUserId_Space() throws Exception {
this.mockMvc.perform(get("/payment/retrieve/ /")).andExpect(status().isBadRequest());
}

@Test
void getRetrieveByUserId_Missing() throws Exception {
this.mockMvc.perform(get("/payment/retrieve")).andExpect(status().isNotFound());
}

private PaymentDto createPaymentDto(final String id) {
final PaymentDto payment = new PaymentDto();
payment.setId(id);
return payment;
}

}
79 changes: 79 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,79 @@
package com.bravo.user.service;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import org.junit.jupiter.api.BeforeEach;
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.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 com.bravo.user.App;
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.model.dto.PaymentDto;

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

@Autowired
private PaymentService paymentService;

@MockBean
private ResourceMapper resourceMapper;

@MockBean
private PaymentRepository paymentRepository;

private List<PaymentDto> dtoPayments;

@BeforeEach
public void beforeEach() {
final List<Integer> ids = IntStream.range(1, 10).boxed().collect(Collectors.toList());

final List<Payment> daoPayments = ids.stream()
.map(id -> createPayment(Integer.toString(id))).collect(Collectors.toList());

when(paymentRepository.findByUserId(anyString())).thenReturn(daoPayments);

this.dtoPayments = ids.stream().map(id -> createPaymentDto(Integer.toString(id)))
.collect(Collectors.toList());

when(resourceMapper.convertPayments(daoPayments)).thenReturn(dtoPayments);
}

@Test
void retrieveByUserId() {
final String userId = "123a-456b";
final List<PaymentDto> results = paymentService.retrieveByUserId(userId);
assertEquals(dtoPayments, results);

verify(paymentRepository).findByUserId(userId);
}

private Payment createPayment(final String id) {
final Payment payment = new Payment();
payment.setId(id);
return payment;
}

private PaymentDto createPaymentDto(final String id) {
final PaymentDto payment = new PaymentDto();
payment.setId(id);
return payment;
}
Comment on lines +101 to +105

Choose a reason for hiding this comment

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

This sets the payment id. Are the other fields checked in these tests?

Copy link
Author

Choose a reason for hiding this comment

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

Was mainly just following the structure of the UserServiceTest, but we could and should test other fields as well.

Choose a reason for hiding this comment

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

The code in the repo isn't necessarily correct :) . I have been following Uncle Bob pretty closely and was a bit shocked by his calling untested code "immoral" https://www.amazon.com/Clean-Coder-Conduct-Professional-Programmers/dp/0137081073. While I think that is a bit overly harsh, from our business client's perspective it's of huge importance. Thanks for the replies. I put in a good word about you to Issac. :)

Copy link
Author

Choose a reason for hiding this comment

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

Great, thank you so much! And will have to check out that book and couldn't agree more that what matters for the client is what matters most.


}
Binary file added user_query.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.