Zhengrui xia

[codecov-commenter]

Codecov Report

Merging #79 (9540d27) into main (6a31797) will increase coverage by 1.50%. The diff coverage is 98.11%.

@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
+ Coverage   96.67%   98.18%   +1.50%     
==========================================
  Files           5        7       +2     
  Lines         331      385      +54     
==========================================
+ Hits          320      378      +58     
+ Misses         11        7       -4     

Impacted Files	Coverage Δ
service/utils/log_handlers.py	90.90% <90.90%> (ø)
service/__init__.py	86.36% <100.00%> (+1.17%)	:arrow_up:
service/models.py	96.25% <100.00%> (+0.04%)	:arrow_up:
service/routes.py	100.00% <100.00%> (+0.68%)	:arrow_up:
service/utils/__init__.py	100.00% <100.00%> (ø)
service/utils/error_handlers.py	100.00% <100.00%> (ø)
service/utils/status.py	100.00% <100.00%> (ø)

---

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 6a31797...9540d27. Read the comment docs.

[kdokm]

Sorry but I don't think some of your changes to 'get' and 'list' are right. For 'get', if the user id is invalid, I think it will raise 404 because it doesn’t fit the url specification, and will not perform check for 400 BAD_REQUEST. As you can see, it returns 404 in the corresponding test case. For 'list', all_shopcart() is the one that finds all shopcart id with items, while all() finds all item records for shopcarts. I don't see why you want to change all_shopcart() to all(). And the original test case is correct since we have setup() method that will reset the table before each test.

[kdokm]

Sorry but I don't think some of your changes to 'get' and 'list' are right. For 'get', if the user id is invalid, I think it will raise 404 because it doesn’t fit the url specification, and will not perform check for 400 BAD_REQUEST. As you can see, it returns 404 in the corresponding test case. For 'list', all_shopcart() is the one that finds all shopcart id with items, while all() finds all item records for shopcarts. I don't see why you want to change all_shopcart() to all(). And the original test case is correct since we have setup() method that will reset the table before each test.

Maybe ‘get’ is ok if we delete the int limitation of user_id in the url, as I noticed #80 has put the check inside the method and return 400. Although I think we can wait for the prof’s reply in homework channel.

[TimothyXu]

Apart from a few typos everything looks good, but the only thing keeping me from merging it is that the test coverage is below 95% - there are quite a few lines in log_handlers.py that have no tests and according to homework instructions we cannot accept any pull requests without 95% code coverage :( Also you should merge new changes on main branch. I'll try to patch up the missing test for routes.py first, and the missing tests for 3 lines in models.py, then that'll be enough to get us to 95%

[lemadmax]

Apart from a few typos everything looks good, but the only thing keeping me from merging it is that the test coverage is below 95% - there are quite a few lines in log_handlers.py that have no tests and according to homework instructions we cannot accept any pull requests without 95% code coverage :( Also you should merge new changes on main branch. I'll try to patch up the missing test for routes.py first, and the missing tests for 3 lines in models.py, then that'll be enough to get us to 95%

Above 95% now