Remove and edit producs groups #40
Conversation
|
The adminpanel controllers don't follow REST design 😉 |
| public String addProductGroup(@ModelAttribute @Validated ProductGroupRequest productGroupRequest) { | ||
| productService.addProductGroup(productGroupRequest); | ||
| System.out.println("here"); |
There was a problem hiding this comment.
Whoops, this is old.
| public String addProductGroup(@ModelAttribute @Validated ProductGroupRequest productGroupRequest) { | ||
| productService.addProductGroup(productGroupRequest); | ||
| System.out.println("here"); |
| public String addProductGroup(@ModelAttribute @Validated ProductGroupRequest productGroupRequest) { | ||
| productService.addProductGroup(productGroupRequest); | ||
| System.out.println("here"); | ||
|
|
||
| return "redirect:/products"; |
There was a problem hiding this comment.
Please add RedirectAttributes to show a message on the success of the adding
| public String deleteProductGroup(@PathVariable int productGroupId, RedirectAttributes redirectAttributes) { | ||
| try { | ||
| productService.deleteProductGroup(productGroupId); | ||
| } catch (RuntimeException e) { |
There was a problem hiding this comment.
Let's add a ExceptionHandler method for this controller, as the logic is the same for all RuntimeExceptions
There was a problem hiding this comment.
Try to have that "redirect" to the current page, so you don't lose a filled in form.
There was a problem hiding this comment.
How would you imagine that? This is a delete endpoint, what should happen?
| throw new ProductInUseException("Products are already ordered"); | ||
| @Override | ||
| public void editProductGroup(ProductGroupRequest productGroupRequest) { | ||
| if (productGroupRequest.getId() != 0) { |
There was a problem hiding this comment.
It was also in the editProduct() method, could you shed some light on this?
There was a problem hiding this comment.
Hmm, probably because the id's are int, which defaults to 0 instead of null
| @Override | ||
| public void editProductGroup(ProductGroupRequest productGroupRequest) { | ||
| if (productGroupRequest.getId() != 0) { | ||
| Committee committee = committeeRepository.findOne(productGroupRequest.getCommitteeId()); |
There was a problem hiding this comment.
Let's take the committeeRepository out of this Service. Use the CommitteeService instead
| public void editProductGroup(ProductGroupRequest productGroupRequest) { | ||
| if (productGroupRequest.getId() != 0) { | ||
| Committee committee = committeeRepository.findOne(productGroupRequest.getCommitteeId()); | ||
| ProductGroup productGroup = productGroupRepository.findOne(productGroupRequest.getId()); |
There was a problem hiding this comment.
Same for this. Create/use a servicecall getProductGroupById() for this, with proper error handling
|
|
||
| @Override | ||
| public void deleteProductGroup(int productGroupId) { | ||
| ProductGroup productGroup = productGroupRepository.findOne(productGroupId); |
There was a problem hiding this comment.
So do we want to make a new ProductGroupService and ProductGroupServiceImpl? Either that or leave it here.
There was a problem hiding this comment.
I think we can put that in one service. Once we need more functionality that requires a lot more methods, we'll separate them.
| public void deleteProductGroup(int productGroupId) { | ||
| ProductGroup productGroup = productGroupRepository.findOne(productGroupId); | ||
| if (!productGroup.getProducts().isEmpty()) { | ||
| throw new ProductGroupInUseException("Product group must be empty"); |
There was a problem hiding this comment.
Is this really the case? What would break if you remove a group with products in it?
There was a problem hiding this comment.
I had previously implemented it in a way where products were removed from the group when the group was removed, but this may be an action where we don't want it to be very easy to perform. I'm also not quite sure which way to go, it made sense to me that since there is a limit involved, you don't want it to be this easy, just like removing a product that is already ordered.
There was a problem hiding this comment.
Alright, then perhaps add a section to the editGroup page where you can remove products currently in the group?
There was a problem hiding this comment.
We can also add the removal of tickets from a group as a hoothub issue, since this is not very high prioroty. It is also not relevant to editing the product group, it would make more sense to have it in a different page. /products/group/editproductsor something similar.
There was a problem hiding this comment.
Leave it like this for now, and create a HootHub issue to implement this nicely.
| package ch.wisv.payments.exception; | ||
|
|
||
| public class ProductGroupInUseException extends RuntimeException { | ||
| public ProductGroupInUseException(String s) { |
There was a problem hiding this comment.
This constructor is already in RuntimeException
There was a problem hiding this comment.
You unfortunately have to do this to make use of the super(...) constructor though.
|
I'm not quite sure what your plan was with the return to the form on the delete, since there is no form at all there.
Issue created at #41 |
Still wondering if all endpoints are correct, an endpoint in the form of
/products/addis not commonly used,/productswould be a better option to do and aligns with standard RESTful endpoint usage.