2022-Spring-NYU-DevOps-Shopcarts / shopcarts

Shopcarts squad.
Apache License 2.0
2 stars 2 forks source link

refactor item route #122

Closed yjjw closed 2 years ago

codecov-commenter commented 2 years ago

Codecov Report

Merging #122 (498e83b) into main (eeb0b40) will decrease coverage by 1.86%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #122      +/-   ##
==========================================
- Coverage   97.33%   95.47%   -1.87%     
==========================================
  Files           7        7              
  Lines         413      442      +29     
==========================================
+ Hits          402      422      +20     
- Misses         11       20       +9     
Impacted Files Coverage Δ
service/routes.py 100.00% <100.00%> (ø)
service/utils/error_handlers.py 51.51% <0.00%> (-27.28%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update eeb0b40...498e83b. Read the comment docs.

TimothyXu commented 2 years ago

Most of the work is done which is great! Refactoring is fine. But 3 issues to deal with:

  1. As prof said in our slack channel, A 400_BAD_REQUEST is reserved for the payload. Not the path. so we need to change all the urls back to etc. and not check for them manually. And when we try to call e.g. /shopcarts/wrongid we should get the default 404 from Flask and that's fine.
  2. Once this is done we should also delete most of error_handlers.py so they don't impact our nosetests coverage
  3. Unless I'm understanding HTML incorrectly due to not knowing any front end, we need to get rid of the static get route at /static/. image
yjjw commented 2 years ago

Most of the work is done which is great! Refactoring is fine. But 3 issues to deal with:

  1. As prof said in our slack channel, A 400_BAD_REQUEST is reserved for the payload. Not the path. so we need to change all the urls back to int:shopcart_id etc. and not check for them manually. And when we try to call e.g. /shopcarts/wrongid we should get the default 404 from Flask and that's fine.
  2. Once this is done we should also delete most of error_handlers.py so they don't impact our nosetests coverage
  3. Unless I'm understanding HTML incorrectly due to not knowing any front end, we need to get rid of the static get route at /static/path:filename.
image

I don't quite understand the third point about static, doesn't professor mean that we want to leave the static paths?

TimothyXu commented 2 years ago

Most of the work is done which is great! Refactoring is fine. But 3 issues to deal with:

  1. As prof said in our slack channel, A 400_BAD_REQUEST is reserved for the payload. Not the path. so we need to change all the urls back to int:shopcart_id etc. and not check for them manually. And when we try to call e.g. /shopcarts/wrongid we should get the default 404 from Flask and that's fine.
  2. Once this is done we should also delete most of error_handlers.py so they don't impact our nosetests coverage
  3. Unless I'm understanding HTML incorrectly due to not knowing any front end, we need to get rid of the static get route at /static/path:filename.
image

I don't quite understand the third point about static, doesn't professor mean that we want to leave the static paths?

The professor said the swagger statics are needed, but I thought the /static/ should not be there. Or is that needed for our webpage to function? I just don't know any front end.

yjjw commented 2 years ago

Most of the work is done which is great! Refactoring is fine. But 3 issues to deal with:

  1. As prof said in our slack channel, A 400_BAD_REQUEST is reserved for the payload. Not the path. so we need to change all the urls back to int:shopcart_id etc. and not check for them manually. And when we try to call e.g. /shopcarts/wrongid we should get the default 404 from Flask and that's fine.
  2. Once this is done we should also delete most of error_handlers.py so they don't impact our nosetests coverage
  3. Unless I'm understanding HTML incorrectly due to not knowing any front end, we need to get rid of the static get route at /static/path:filename.
image

I don't quite understand the third point about static, doesn't professor mean that we want to leave the static paths?

The professor said the swagger statics are needed, but I thought the /static/path:filename should not be there. Or is that needed for our webpage to function? I just don't know any front end.

Sorry I don't understand this part either.. I will fix other parts first anyways

TimothyXu commented 2 years ago

Most of the work is done which is great! Refactoring is fine. But 3 issues to deal with:

  1. As prof said in our slack channel, A 400_BAD_REQUEST is reserved for the payload. Not the path. so we need to change all the urls back to int:shopcart_id etc. and not check for them manually. And when we try to call e.g. /shopcarts/wrongid we should get the default 404 from Flask and that's fine.
  2. Once this is done we should also delete most of error_handlers.py so they don't impact our nosetests coverage
  3. Unless I'm understanding HTML incorrectly due to not knowing any front end, we need to get rid of the static get route at /static/path:filename.
image

I don't quite understand the third point about static, doesn't professor mean that we want to leave the static paths?

The professor said the swagger statics are needed, but I thought the /static/path:filename should not be there. Or is that needed for our webpage to function? I just don't know any front end.

Sorry I don't understand this part either.. I will fix other parts first anyways

So professor said our API should not have any GET routes that's not explicitly specified by our API. We currently can do e.g. GET http://localhost:8000/static/images/newapp-icon.png. I thought this is not allowed then we need to fix it. OR maybe these GETs are necessary for our UI to work, in which case then we don't need to fix it. I just don't know because I don't know how websites work

kdokm commented 2 years ago

@yjjw @TimothyXu From my knowledge, the static endpoint is automatically generated by flask, and it works together with some flask APIs, so I don't think we are required to get rid of it.

TimothyXu commented 2 years ago

@kdokm Thank you! Great so the refactoring's all done