-
Notifications
You must be signed in to change notification settings - Fork 41
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
Greg Vella Candidate Assessment #38
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
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.utility.PageUtil; | ||
import com.bravo.user.validator.UserValidator; | ||
import org.springframework.data.domain.PageRequest; | ||
import org.springframework.web.bind.annotation.*; | ||
import javax.servlet.http.HttpServletResponse; | ||
import java.util.List; | ||
|
||
@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 = "/user/{id}") | ||
@ResponseBody | ||
public List<PaymentDto> retrieveUserPayments( | ||
final @PathVariable String id, | ||
final @RequestParam(required = false) Integer page, | ||
final @RequestParam(required = false) Integer size, | ||
final HttpServletResponse httpResponse) { | ||
userValidator.validateId(id); | ||
final PageRequest pageRequest = PageUtil.createPageRequest(page, size); | ||
return paymentService.retrievePayments(id, pageRequest, httpResponse); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package com.bravo.user.dao.repository; | ||
|
||
import com.bravo.user.dao.model.Payment; | ||
import org.springframework.data.jpa.repository.JpaRepository; | ||
import org.springframework.data.jpa.repository.JpaSpecificationExecutor; | ||
import org.springframework.stereotype.Repository; | ||
|
||
@Repository | ||
public interface PaymentRepository extends JpaRepository<Payment, String>, JpaSpecificationExecutor<Payment> { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package com.bravo.user.dao.specification; | ||
|
||
import com.bravo.user.dao.model.Payment; | ||
import com.bravo.user.model.filter.PaymentFilter; | ||
import javax.persistence.criteria.CriteriaBuilder; | ||
import javax.persistence.criteria.CriteriaQuery; | ||
import javax.persistence.criteria.Root; | ||
import java.util.Set; | ||
|
||
public class PaymentSpecification extends AbstractSpecification<Payment> { | ||
|
||
private final PaymentFilter filter; | ||
|
||
public PaymentSpecification(final PaymentFilter filter){ | ||
this.filter = filter; | ||
} | ||
|
||
@Override | ||
public void doFilter( | ||
Root<Payment> root, | ||
CriteriaQuery<?> criteriaQuery, | ||
CriteriaBuilder criteriaBuilder | ||
){ | ||
applyStringFilterToFields(Set.of(root.get("userId")), filter.getUserId()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package com.bravo.user.model.filter; | ||
|
||
import lombok.AllArgsConstructor; | ||
import lombok.Builder; | ||
import lombok.Data; | ||
import lombok.NoArgsConstructor; | ||
|
||
@Data | ||
@Builder | ||
@AllArgsConstructor | ||
@NoArgsConstructor | ||
public class PaymentFilter { | ||
private String userId; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
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.specification.PaymentSpecification; | ||
import com.bravo.user.model.dto.PaymentDto; | ||
import com.bravo.user.model.filter.PaymentFilter; | ||
import com.bravo.user.utility.PageUtil; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import org.springframework.data.domain.Page; | ||
import org.springframework.data.domain.PageRequest; | ||
import org.springframework.stereotype.Service; | ||
import javax.servlet.http.HttpServletResponse; | ||
import java.util.List; | ||
|
||
@Service | ||
public class PaymentService { | ||
|
||
private static final Logger LOGGER = LoggerFactory.getLogger(PaymentService.class); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've seen this in many different ways so would like your thoughts: here, LOGGER is all caps. Is that in line with established naming conventions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Existing UserService.class used this format, so I kept it consistent for the PaymentService. I've used the SLF4J annotation previously (so the reference is log.info, etc.). If no prior precedent in existing code, I would have likely named it log. |
||
|
||
private final PaymentRepository paymentRepository; | ||
|
||
private final ResourceMapper resourceMapper; | ||
|
||
public PaymentService(PaymentRepository paymentRepository, ResourceMapper resourceMapper) { | ||
this.paymentRepository = paymentRepository; | ||
this.resourceMapper = resourceMapper; | ||
} | ||
|
||
public List<PaymentDto> retrievePayments( | ||
final String userId, | ||
final PageRequest pageRequest, | ||
final HttpServletResponse httpResponse | ||
){ | ||
final PaymentFilter filter = PaymentFilter.builder().userId(userId).build(); | ||
final PaymentSpecification specification = new PaymentSpecification(filter); | ||
final Page<Payment> paymentPage = paymentRepository.findAll(specification, pageRequest); | ||
final List<PaymentDto> payments = resourceMapper.convertPayments(paymentPage.getContent()); | ||
LOGGER.info("found {} payments(s)", payments.size()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this enough value to be in included in every call? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, in this case, doesn't really add any significant value for production debugging, verification, etc. Could be more useful if it output userid and payment list size. Would not output any payment data to logs (due to data sensitivity). So could be removed or enhanced, depending on what's needed. |
||
|
||
PageUtil.updatePageHeaders(httpResponse, paymentPage, pageRequest); | ||
return payments; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -648,3 +648,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) values | ||
('43f33d30-f3f8-4743-a94e-4db11fdb747d', '008a4215-0b1d-445e-b655-a964039cbb5a', '1234567890123456', 12, 2024), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The last 4 digits is repeated in all tests, this may lead to good code coverage and not great scenario coverage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Should have randomized the trailing digits instead of the leading digits to make unique card numbers. |
||
('53f33d30-f3f8-4743-a94e-4db11fdb747d', '008a4215-0b1d-445e-b655-a964039cbb5a', '4434567890123456', 4, 2026), | ||
('77f33d30-f3f8-4743-a94e-4db11fdb747d', '008a4215-0b1d-445e-b655-a964039cbb5a', '5534567890123456', 10, 2027), | ||
('589872ec-46f8-46b5-b809-d0724d965f0e', '00963d9b-f884-485e-9455-fcf30c6ac379', '2234567890123456', 12, 2024), | ||
('96a983d0-ba0e-4f30-afb6-667d4724b253', '00963d9b-f884-485e-9455-fcf30c6ac379', '3234567890123456', 7, 2023), | ||
('11a983d0-ba0e-4f30-afb6-667d4724b253', '18227685-5345-46f7-96b0-c8428e0a2153', '8884567890123456', 9, 2025); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
package com.bravo.user.controller; | ||
|
||
import com.bravo.user.App; | ||
import com.bravo.user.model.dto.PaymentDto; | ||
import com.bravo.user.service.PaymentService; | ||
import com.bravo.user.utility.PageUtil; | ||
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.data.domain.PageRequest; | ||
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 javax.servlet.http.HttpServletResponse; | ||
import java.util.List; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.IntStream; | ||
|
||
import static org.mockito.ArgumentMatchers.*; | ||
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; | ||
|
||
@ContextConfiguration(classes = {App.class}) | ||
@ExtendWith(SpringExtension.class) | ||
@SpringBootTest() | ||
@AutoConfigureMockMvc | ||
public 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()); | ||
Comment on lines
+47
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is ids mutable here? Looking at this, it isn't the most readable code, it does work of course. Would like to know if you see this as better than a traditional for loop. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, values in ids variable are mutable. The ids reference is immutable. I normally prefer for loops over this type of statement as I find it easier to understand. However, I was trying to keep the test classes consistent (as this construct was used in existing classes). |
||
|
||
this.payments = ids.stream() | ||
.map(id -> createPaymentDto(Integer.toString(id))) | ||
.collect(Collectors.toList()); | ||
} | ||
|
||
@Test | ||
void getRetrieveWithUserId() throws Exception { | ||
when(paymentService | ||
.retrievePayments(anyString(), any(PageRequest.class), any(HttpServletResponse.class))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using anyString here matches everything. Does being that permission take away from the value of this test or is it testing something else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would consider controller tests to be more about ensuring the integration of the components. The service tests are more geared to test the business logic. So in this example, I don't feel it diminishes the value of the test. But I'm always open to other interpretations or better ways to do things. :) |
||
.thenReturn(payments); | ||
|
||
final ResultActions result = this.mockMvc | ||
.perform(get("/payment/user/1234")) | ||
.andExpect(status().isOk()); | ||
|
||
for(int i = 0; i < payments.size(); i++){ | ||
result.andExpect(jsonPath(String.format("$[%d].id", i)).value(payments.get(i).getId())); | ||
} | ||
|
||
final PageRequest pageRequest = PageUtil.createPageRequest(null, null); | ||
verify(paymentService).retrievePayments( | ||
eq("1234"), eq(pageRequest), any(HttpServletResponse.class) | ||
); | ||
} | ||
|
||
@Test | ||
void getRetrieveWithUserIdMissing() throws Exception { | ||
this.mockMvc.perform(get("/payment/user/")) | ||
.andExpect(status().isNotFound()); | ||
} | ||
|
||
@Test | ||
void getRetrieveWithUserIdSpaces() throws Exception { | ||
this.mockMvc.perform(get("/payment/user/ ")) | ||
.andExpect(status().isBadRequest()); | ||
} | ||
|
||
private PaymentDto createPaymentDto(final String id){ | ||
final PaymentDto paymentDto = new PaymentDto(); | ||
paymentDto.setId(id); | ||
return paymentDto; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
package com.bravo.user.service; | ||
|
||
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.dao.specification.PaymentSpecification; | ||
import com.bravo.user.model.dto.PaymentDto; | ||
import com.bravo.user.utility.PageUtil; | ||
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.data.domain.Page; | ||
import org.springframework.data.domain.PageRequest; | ||
import org.springframework.test.context.ContextConfiguration; | ||
import org.springframework.test.context.junit.jupiter.SpringExtension; | ||
|
||
import javax.servlet.http.HttpServletResponse; | ||
import java.util.List; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.IntStream; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.mockito.ArgumentMatchers.any; | ||
import static org.mockito.Mockito.*; | ||
|
||
@ContextConfiguration(classes = {App.class}) | ||
@ExtendWith(SpringExtension.class) | ||
@SpringBootTest | ||
public class PaymentServiceTest { | ||
@Autowired | ||
private HttpServletResponse httpResponse; | ||
|
||
@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 Page<Payment> mockPage = mock(Page.class); | ||
when(paymentRepository.findAll(any(PaymentSpecification.class), any(PageRequest.class))) | ||
.thenReturn(mockPage); | ||
|
||
final List<Payment> daoPayments = ids.stream() | ||
.map(id -> createPayment(Integer.toString(id))) | ||
.collect(Collectors.toList()); | ||
when(mockPage.getContent()).thenReturn(daoPayments); | ||
when(mockPage.getTotalPages()).thenReturn(9); | ||
|
||
this.dtoPayments = ids.stream() | ||
.map(id -> createPaymentDto(Integer.toString(id))) | ||
.collect(Collectors.toList()); | ||
when(resourceMapper.convertPayments(daoPayments)).thenReturn(dtoPayments); | ||
} | ||
|
||
@Test | ||
public void retrievePaymentByUserId() { | ||
final String input = "input"; | ||
final PageRequest pageRequest = PageUtil.createPageRequest(null, null); | ||
final List<PaymentDto> results = paymentService.retrievePayments(input, pageRequest, httpResponse); | ||
assertEquals(dtoPayments, results); | ||
assertEquals("9", httpResponse.getHeader("page-count")); | ||
assertEquals("1", httpResponse.getHeader("page-number")); | ||
assertEquals("20", httpResponse.getHeader("page-size")); | ||
|
||
} | ||
@Test | ||
public void retrievePaymentByUserIdPaged() { | ||
final String input = "input2"; | ||
final PageRequest pageRequest = PageUtil.createPageRequest(2, 5); | ||
final List<PaymentDto> results = paymentService.retrievePayments(input, pageRequest, httpResponse); | ||
assertEquals(dtoPayments, results); | ||
assertEquals("9", httpResponse.getHeader("page-count")); | ||
assertEquals("2", httpResponse.getHeader("page-number")); | ||
assertEquals("5", httpResponse.getHeader("page-size")); | ||
|
||
} | ||
|
||
private Payment createPayment(final String id){ | ||
final Payment payment = new Payment(); | ||
payment.setId(id); | ||
return payment; | ||
} | ||
|
||
private PaymentDto createPaymentDto(final String id){ | ||
final PaymentDto paymentDto = new PaymentDto(); | ||
paymentDto.setId(id); | ||
return paymentDto; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combinine Builders and Constructors allows for 3 different ways to set the value, the builder, the constructor, and the setter. Are all of these needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm only using the builder for this class, so really not needed. Habit from my last project of adding all 3 of these.