TheCSharpAcademy / CONSOLE.PointOfSale

6 stars 15 forks source link

Feature/view order details brun32700 #53

Closed brun32700 closed 1 year ago

brun32700 commented 1 year ago

Resolves #51

Summary: -> Removed list of products of an order from the ViewOrders() method. -> Added list of products of an order to the ViewOrderDetails() method using provided order Id. -> After being shown list of products of an order, the user is then asked whether they want to view another orders, order details. If not, they are sent back to the main menu. -> Added GetOrder(int id) in KebabController, to retrieve a single order by its Id.

Observations: -> _userInput.GetValidAnswer() - there is no case sensitivity, or spacing (trim) check, is this intended? (so Yes, YES doesn't work etc.) -> _userInput.GetValidAnswer() - When wrong value is inserted, the user is not informed that there is a wrong value (not user friendly)

Please review my pull request, and let me know if I missed something or something needs improvement. Thanks in advance.

brun32700 commented 1 year ago

@TheCSharpAcademy I have updated the method to use the index for getting the order details, if you could please re-check it.

So I pulled the orders list into the ViewOrderDetails() method as you suggested and call the ViewOrders() method to display the orders to the user. But now I am querying the orders list in both of the methods.

Would it be better if we adjusted the ViewOrders() method to pass in an orders list to be displayed? Because the orders in ViewOrderDetails() and the orders in ViewOrders() could be different, as new orders are being added constantly (assuming multiple users are using this app simultaneously)?

I could just display the orders using the list in the ViewOrderDetails() method, but then I would be repeating the function of ViewOrders().

image

TheCSharpAcademy commented 1 year ago

Would it be better if we adjusted the ViewOrders() method to pass in an orders list to be displayed? Because the orders in ViewOrderDetails() and the orders in ViewOrders() could be different, as new orders are being added constantly (assuming multiple users are using this app simultaneously)?

@brun32700 Yeah that works! View orders eventually will live in a UI class, but right now you're right, it would be unnecessary code repetition.

brun32700 commented 1 year ago

@TheCSharpAcademy I have changed the ViewOrders() method to accept a list of orders to print out to the console, instead of getting the list from within the method. Is this good now?

TheCSharpAcademy commented 1 year ago

@brun32700 I'll approve the PR but we were still grabbing the wrong Id. Check out the modification:

image

brun32700 commented 1 year ago

@TheCSharpAcademy In my eyes it seems to be the correct Id (on my side anyway). See images below:

Unless you mean that I should not assume that the list of orders will be displayed in the same order (could be reversed in ViewOrdersMethod etc), so that is why we need to get the actual Id of the order and not the index of the order in the list?

If this is the case then, where should I submit a pull request to? Do I need to create a new issue for it?

image image