sean carnahan: 0013 - [backend] add payment method retrieve api #41
Conversation
| import com.bravo.user.dao.model.Payment; | ||
|
|
||
| @Repository | ||
| public interface PaymentRepository extends JpaRepository<Payment, String> { |
There was a problem hiding this comment.
This should be extending PagingAndSortingRepository. And the findByUserId() method should include a Pageable parameter.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
| this.resourceMapper = resourceMapper; | ||
| } | ||
|
|
||
| public List<PaymentDto> retrieveByUserId(final String userId) { |
There was a problem hiding this comment.
This method should accept a Pageable parameter
…nit tests with Paging
src/main/resources/application.yml
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Like this for the testing version of the appliation.yml. In the root, is this a security issue?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Very true, nice catch. Should not be checked into main branch. Removed
| @@ -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), | |||
There was a problem hiding this comment.
Is there potential for a card number that is not only digits? I've not seen that.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| private PaymentDto createPaymentDto(final String id) { | ||
| final PaymentDto payment = new PaymentDto(); | ||
| payment.setId(id); | ||
| return payment; | ||
| } |
There was a problem hiding this comment.
This sets the payment id. Are the other fields checked in these tests?
There was a problem hiding this comment.
Was mainly just following the structure of the UserServiceTest, but we could and should test other fields as well.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
seancarnahan
requested review from
tlynema-bravolt and
darrinamway
and removed request for
tlynema-bravolt and
darrinamway
No description provided.