-
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
trello ticket 13 - candidate-sample-payment-addition #32
base: main
Are you sure you want to change the base?
Changes from 4 commits
5a94198
43db612
1269862
212c5f1
e1e8463
e55716f
535cd72
1e12677
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,57 @@ | ||
package com.bravo.user.controller; | ||
|
||
import com.bravo.user.annotation.SwaggerController; | ||
import com.bravo.user.exception.BadRequestException; | ||
import com.bravo.user.model.dto.PaymentDto; | ||
import com.bravo.user.model.filter.PaymentFilter; | ||
import com.bravo.user.service.PaymentService; | ||
import com.bravo.user.utility.PageUtil; | ||
import com.bravo.user.validator.UserValidator; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
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 UserValidator userValidator; | ||
private final PaymentService paymentService; | ||
|
||
public PaymentController( | ||
UserValidator userValidator, | ||
PaymentService paymentService | ||
){ | ||
this.userValidator = userValidator; | ||
this.paymentService = paymentService; | ||
} | ||
|
||
@GetMapping(value = "/retrieve") | ||
@ResponseBody | ||
public List<PaymentDto> retrieve( | ||
final @RequestParam String userId, | ||
final @RequestParam(required = false) Integer page, | ||
final @RequestParam(required = false) Integer size, | ||
final HttpServletResponse httpResponse | ||
){ | ||
userValidator.validateId(userId); | ||
final PageRequest pageRequest = PageUtil.createPageRequest(page, size); | ||
return paymentService.retrievePaymentByUserId(userId, pageRequest, httpResponse); | ||
} | ||
|
||
@PostMapping(value = "/retrieve") | ||
@ResponseBody | ||
public List<PaymentDto> retrieve( | ||
final @RequestBody PaymentFilter filter, | ||
final @RequestParam(required = false) Integer page, | ||
final @RequestParam(required = false) Integer size, | ||
final HttpServletResponse httpResponse | ||
){ | ||
final PageRequest pageRequest = PageUtil.createPageRequest(page, size); | ||
return paymentService.retrieve(filter, pageRequest, httpResponse); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
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,31 @@ | ||
package com.bravo.user.dao.specification; | ||
|
||
import com.bravo.user.dao.model.Payment; | ||
import com.bravo.user.model.filter.PaymentFilter; | ||
import lombok.EqualsAndHashCode; | ||
|
||
import javax.persistence.criteria.CriteriaBuilder; | ||
import javax.persistence.criteria.CriteriaQuery; | ||
import javax.persistence.criteria.Root; | ||
import java.util.Set; | ||
|
||
@EqualsAndHashCode | ||
public class PaymentSpecification extends AbstractSpecification<Payment>{ | ||
|
||
private final PaymentFilter filter; | ||
|
||
public PaymentSpecification(final PaymentFilter filter) { | ||
this.filter = filter; | ||
} | ||
|
||
@Override | ||
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 |
---|---|---|
@@ -1,14 +1,25 @@ | ||
package com.bravo.user.model.dto; | ||
|
||
import java.time.LocalDateTime; | ||
|
||
import com.bravo.user.dao.model.Payment; | ||
import lombok.Data; | ||
import lombok.NoArgsConstructor; | ||
|
||
@Data | ||
@NoArgsConstructor | ||
public class PaymentDto { | ||
|
||
private String id; | ||
private String userId; | ||
private String cardNumberLast4; | ||
private Integer expiryMonth; | ||
private Integer expiryYear; | ||
private LocalDateTime updated; | ||
|
||
public PaymentDto(final String id) { | ||
this(); | ||
this.id = id; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package com.bravo.user.model.filter; | ||
|
||
import lombok.AllArgsConstructor; | ||
import lombok.Builder; | ||
import lombok.Data; | ||
import lombok.NoArgsConstructor; | ||
|
||
import java.util.Set; | ||
|
||
@Data | ||
@Builder | ||
@AllArgsConstructor | ||
@NoArgsConstructor | ||
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. With this being new, is there a reason for it having a builder a constructor and setters? Having three ways to set values seems overkill. 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. AllArgsConstructor was needed in order to create the object in the retrievePaymentByUserId on line 39 in the PaymentService class. NoArgsConstructor - needed to stop an issue with serialization of the filter request object, it would return a Bad Request due to the application not being able to deserialize the object. Error from terminal: (although at least one Creator exists): cannot deserialize from Object value (no delegate- or property-based Creator). I could get rid of the AllArgsConstructor by initializing an instance of the 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. Code generators like Lombok are a toss up. In this case, it's autogen needs to match the serialization. My preference has evolved to using immutable builders in this situation and configuring the serialization to use builders. The main idea here is to pick one way to set the values and stick to it. Someone copied a few pages from effective java here https://cs108.epfl.ch/archive/19/c/i/EffectiveJava_Item17.pdf. I highly recommend the book. It helped my view of Java coding immensely. The goal should be immutable value objects. 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. Thank you for the share, I will read this and take a look at getting the book (always room for learning more). |
||
public class PaymentFilter { | ||
|
||
private String userId; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
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); | ||
private final PaymentRepository paymentRepository; | ||
private final ResourceMapper resourceMapper; | ||
|
||
public PaymentService( | ||
PaymentRepository paymentRepository, | ||
ResourceMapper resourceMapper | ||
){ | ||
this.paymentRepository = paymentRepository; | ||
this.resourceMapper = resourceMapper; | ||
} | ||
|
||
public List<PaymentDto> retrievePaymentByUserId( | ||
final String userId, | ||
final PageRequest pageRequest, | ||
final HttpServletResponse httpResponse | ||
){ | ||
return retrieve(new PaymentFilter(userId), pageRequest, httpResponse); | ||
} | ||
|
||
public List<PaymentDto> retrieve( | ||
final PaymentFilter filter, | ||
final PageRequest pageRequest, | ||
final HttpServletResponse httpResponse | ||
){ | ||
LOGGER.info("Request to retrieve payment information being conducted... paymentFilter: {}", filter); | ||
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. Good use of string formatting for the logger. This log message doesn't seem INFO to me. There are some reasons to log like this. Not sure they apply here. Would like to know what your thinking was. 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 logger is kind of a habit I have from when I worked at Verizon (their in home standard). It was required in the service area anytime something came in or went out, in the hopes of trouble shooting problems that may arise either from front end or the database. I could change it to Trace or Debug though. |
||
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 {} payment(s)", payments.size()); | ||
|
||
PageUtil.updatePageHeaders(httpResponse, paymentPage, pageRequest); | ||
return payments; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,9 @@ public class PageUtil { | |
|
||
private static final int DEFAULT_SIZE = 20; | ||
|
||
public static PageRequest createPageRequest() { | ||
return createPageRequest(null, null); | ||
} | ||
Comment on lines
+13
to
+15
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. This looks like it is only used for test code. In general, having nulls can create additional checks that need to happen in code. Since this is only in test, could the other initializer be used with hardcoded values? 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. This was more of a code cleanup effort from having null values in tests to acquire default values from the PageUtil (page 1, size 20). Just a personal choice, but the original call could still be used, and this removed. |
||
public static PageRequest createPageRequest(final Integer page, final Integer size){ | ||
return createPageRequest(page, size, DEFAULT_SIZE); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,114 @@ | ||
package com.bravo.user.controller; | ||
|
||
import com.bravo.user.App; | ||
import com.bravo.user.model.dto.PaymentDto; | ||
import com.bravo.user.model.filter.PaymentFilter; | ||
import com.bravo.user.service.PaymentService; | ||
import com.bravo.user.utility.PageUtil; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
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.http.MediaType; | ||
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.post; | ||
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 { | ||
|
||
private static final ObjectMapper MAPPER = new ObjectMapper(); | ||
|
||
@Autowired | ||
private MockMvc mockMvc; | ||
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. Happy to see mockmvc. That is often missed. 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. Thank you, it's been a little while since I have done JUnits and Mockito, so I am trying to do more learning and example coding in my free-time. |
||
@MockBean | ||
private PaymentService paymentService; | ||
|
||
private List<PaymentDto> payments; | ||
private PaymentFilter paymentFilter; | ||
|
||
@BeforeEach | ||
public void beforeEach(){ | ||
this.payments = IntStream | ||
.range(1,6) | ||
.mapToObj(id -> new PaymentDto(Integer.toString(id))) | ||
.collect(Collectors.toList()); | ||
|
||
this.paymentFilter = new PaymentFilter("1"); | ||
} | ||
|
||
@Test | ||
public void getRetrieveWithUserId() throws Exception { | ||
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 the throws needed here? 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, it is to cover the .perform, and result.andExpect calls as both throw exceptions. I could wrap these into a try/catch block, but this makes the code cleaner. |
||
when(paymentService | ||
.retrievePaymentByUserId(anyString(), any(PageRequest.class), any(HttpServletResponse.class))) | ||
.thenReturn(payments); | ||
|
||
final ResultActions result = this.mockMvc | ||
.perform(get( "/payment/retrieve?userId=1")) | ||
.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(); | ||
verify(paymentService).retrievePaymentByUserId( | ||
eq("1"), eq(pageRequest), any(HttpServletResponse.class) | ||
); | ||
} | ||
|
||
@Test | ||
public void postRetrieveWithFilter() throws Exception { | ||
when(paymentService.retrieve(any(PaymentFilter.class), any(PageRequest.class), any(HttpServletResponse.class))) | ||
.thenReturn(payments); | ||
Comment on lines
+84
to
+86
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. Which layers are being tested with this test? 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. Only the flow of the Controller, as the mock is designed to merely mock the results of calling the Service class. |
||
|
||
final String jsonResult = MAPPER.writeValueAsString(paymentFilter); | ||
|
||
final ResultActions result = this.mockMvc | ||
.perform(post("/payment/retrieve") | ||
.contentType(MediaType.APPLICATION_JSON) | ||
.content(jsonResult) | ||
.accept(MediaType.APPLICATION_JSON)) | ||
.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(); | ||
verify(paymentService).retrieve(eq(paymentFilter), eq(pageRequest), any(HttpServletResponse.class)); | ||
} | ||
|
||
@Test | ||
public void retrieveWithUserIdMissing() throws Exception { | ||
this.mockMvc.perform(get("/payment/retrieve")).andExpect(status().isBadRequest()); | ||
} | ||
|
||
@Test | ||
public void retrieveWithEmptyUserId() throws Exception { | ||
this.mockMvc.perform(get("/payment/retrieve?userId=")).andExpect(status().isBadRequest()); | ||
} | ||
} |
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.
Are there any other properties in here that should be required or added to the constructor. Setting final here when there is a setter from @DaTa annotation seems to only protect the value in certain situations. Is there a path to full immutability?
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.
The constructor was mainly created to simplify code used in other places as a code cleanup effort; cleaner than using no args and setter. As far as making it more immutable, I can look into that.
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 am pretty new with lombok (only heard about it a week ago), so I did some digging on the @builder. It looks like this would be a better alternative in the code in terms of creating objects with specific values, and won't clutter up the class I am trying to instantiate with values.
Though I did read a little more and saw @value, which does the same thing as @DaTa, but it makes the Class or Method fully immutable. I don't think this would be a good thing to do for the types of classes I am using (granted, @builder can make a referenced version I could use, but there would be a lot of recoding I would need to do at the moment), but the idea behind this is interesting, and something I wouldn't mind trying out in my free time.
If this isn't what you were looking for, let me know and I will do more research. I will also read up more on lombok and try some examples in my free time to see if it would be possible to update it to @value and be functional.
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.
Exactly :) . @value or an Immutable @builder would be my preference here. If there are multiple levels to the object, builders can have objects that are made with builders as subclasses (though that can get ugly fast).