Opened /api/catalog/books/{book_id} endpoint for deleting book #97
Conversation
src/main/java/com/productdock/adapter/in/web/DeleteBookApi.java
Outdated
Show resolved
Hide resolved
src/main/java/com/productdock/adapter/in/web/DeleteBookApi.java
Outdated
Show resolved
Hide resolved
src/main/java/com/productdock/application/port/out/messaging/DeleteBookMessagingOutPort.java
Show resolved
Hide resolved
src/main/java/com/productdock/adapter/out/kafka/DeletedBookMessagePublisher.java
Show resolved
Hide resolved
src/test/java/com/productdock/data/provider/out/kafka/KafkaTestConsumer.java
Outdated
Show resolved
Hide resolved
src/test/java/com/productdock/integration/DeleteBookApiTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/productdock/application/service/DeleteBookService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/productdock/application/service/DeleteBookService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/productdock/application/service/DeleteBookService.java
Show resolved
Hide resolved
src/main/java/com/productdock/adapter/out/web/RentalsApiClient.java
Outdated
Show resolved
Hide resolved
| public void deleteBook(Long bookId) { | ||
| validate(bookId); | ||
| bookRepository.deleteById(bookId); | ||
| deleteBookMessagingOutPort.sendMessage(bookId); |
There was a problem hiding this comment.
What if sendMessage fails for some reason? Do we want to delete a book without notifying everyone else?
Any idea how to prevent this?
| message = message.concat(status).concat(" by ").concat(userName).concat(". "); | ||
| } | ||
|
|
||
| return message; |
There was a problem hiding this comment.
How will this message look like if the bookRentals has more than one element? Let's say we have two copies of the same book and both are rented.
My guess it would be something like:
"Book is rented by sinisa.tutus. rented by nenad.jeckovic"
I'm not saying it is bad, just is this what you wanted (assuming I recreated the message correctly 😄 )?
There was a problem hiding this comment.
Also if this happens if (rental.status() == null || rental.user() == null) it will return Cannot read rental status. regardless of what we generated in the message before (also the case when we have more than one element in the list)
| package com.productdock.adapter.in.web.dto; | ||
|
|
||
| import com.productdock.domain.RentalStatus; | ||
|
|
||
| import java.util.Date; | ||
|
|
||
| public record BookRentalStateDto (UserProfileDto user, RentalStatus status, Date date){} | ||
|
|
There was a problem hiding this comment.
Why we have this record inside in package when it is used in out ?
There was a problem hiding this comment.
And does it need to be part of adapter layer or part of domain?
| package com.productdock.adapter.in.web.dto; | ||
|
|
||
|
|
||
| public record UserProfileDto (String fullName, String image, String email) {} |
| @Override | ||
| @SneakyThrows | ||
| public void deleteBook(Long bookId) { | ||
| validate(bookId); |
There was a problem hiding this comment.
Give it a bit more context with naming of this method. Method is private so it's ok to have a longer name
src/test/java/com/productdock/integration/DeleteBookApiTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/productdock/application/service/DeleteBookServiceShould.java
Outdated
Show resolved
Hide resolved
src/test/java/com/productdock/application/service/DeleteBookServiceShould.java
Outdated
Show resolved
Hide resolved
| assertThatThrownBy(() -> deleteBookService.deleteBook(BOOK_ID)) | ||
| .isInstanceOf(DeleteBookException.class); |
There was a problem hiding this comment.
Also here, for this type of exception you want to check the exception message also
You will have more test cases for that
|
NenadJeckovic
changed the title
Added delete book functionality with tests.