submission commit#99
Conversation
|
Hi @0lcm bye 👋 |
local migrations that we're accidentally not committed during development
|
Hi @hasona23, I noticed that I had accidentally forgotten to commit a couple migrations during development, so I pushed them just now. Thank you taking the time to review my project! |
hasona23
left a comment
hasona23
left a comment
There was a problem hiding this comment.
Hi @0lcm
Review point:
1- Critical 🔴 :
1- Price Validation:
2- Check for duplicate project tags
3- I don't understand why are tags stored if i cant display them as list and search by tag nor add 1 to product without typing name manually
4- Sales Reivew produces this error
5- Same error happened with adding sale
6- Same error happens with also deleting Sales
7- There is an error without pagination/get items logic which doesn't happen in the Tag system. the error is duplicate data i thought it was database but the data in api.db is fine and not duplicated (and ofcourse sqlite wont duplicate id)
2- Warning 🟡:
1- Solution/Repository naming: it is advised to name as [project name].[GitHub name]
2- Pages system doesn't handle the edge case of an empty page in 2 ways:
- Not removing Next page button
- Allowing to go to empty page
3- Don't make user have to memorize id of products to delete as this will get even more complex when it is not incrementing integer ID (1,2,3,4) but hash id (5f4dcc3b5aa765d61d8327deb882cf99).
instead you could either
- Display them all like in list or with more brief information and then prompt for id
- Use a selection prompt (single or multi up to you)
4- Back option in delete product doesn't work the "null argument" text shouldn't be displayed
5-
you need to make it case-in-sensitive and make sure
6- It is better to not clear any of the input data until all is entered dont clear after each input paramter so i could keep track of what i enterd up till now
6,7- Also for search to leave it even with displaying data so that i could know what is the search for or just type what i set for the search paramters
6,8- For add could allow me
9- It is better and mostly common way to put it in the appsettings.json file any config paramters should go there you could even see it in the academy repositoy although it is not sent through github but reading the README implies the use of it
3- Suggestion 🔵 :
1- Don't clear screen in searching products section till i enter my search paramters
2- Dont delete screen until I finished adding all Create product parameters
3- Display Products as a table instead of list it will cover less vertical space
4- It is better to make the enum displayed as a selection prompt instead of text prompt. it is easier for both you and the as user doesnt have to take care of spelling errors or hidden characters and you will have less headache validating what he wrote
4- Awesome 🟢
1- Pagination
2- Storing constant reused and some config data as constants and static such as Utils.cs and UiHelper.cs
3- The amazing use of Spectre.Console functions which most people ignore. although will benefit from adding slightly more colors and reduce/moderate the use of clear console
I Still didnt finish reading the whole code but the Critical 🔴 must be done as the project wont be accepted till then.
I must say it is amazing all the stuff you did including pagination and your first semi-working API with ui Using Spectre console
I know this may seem daunting at first but it is your first time so this actually quite normal😄
looking forward to seeing your adjustments 😄
Good luck!
|
Critical 🔴 :
2) Something wrong with title header display
3) Cant find the products i made to delete. perhaps there is missing pagination or something
4) Empty page in tag listing
5) Empty infinite pages number in sales
Moderate 🟡 :
6) Just prompting for ID in delete sales and tags would be better to do like in products. You should think about how user feels when using the app is it good for him to keep searching for products and find and memorize their id
You are so close to making it |
2 participants
No description provided.