Refactor error handling #74

[joelvdavies]

Description

Attempts to refactor error handling to match a similar method to object-storage-api.

Notes

• Even if not merged, I noticed the test_catalogue_item for services, UpdateDSL check_update_success doesn't check that the obsolete replacement catalogue item was obtained correctly - this should be fixed. Also the following
  • The items check if a system exists in repo layer, this has been moved to the service like is done for catalogue items (for catalogue categories)
  • The TestDelete in e2e/test_item.py shows different error messages getting an item and deleting one that doesnt exist when they could be the same
  • The test_catalogue_item file in services, TestCreate::test_create_with_all_properties did not specify an obsolete replacement catalogue item id, so did not test this part of the code before
  • test_update_name_to_duplicate in TestUpdate inside test_manufacturer.py (repositories) - did not actually call or assert anything
  • Catalogue category repo and service both check if the parent catalogue category exists - everywhere else (except systems) we rely on just the service (I have not modified this, but think it would save some extra tests to remove from the repo - I think its more akin to checking thinks like the system id for items that relies on access to the other repos)
• Non existence checks for most things have been removed from service tests - this is because to mock them would just be adding a side effect of the raised error, and this is already covered by the e2e tests so the only place where they are left are where there is a try/except that needs to be unit tested for coverage
• Some error messages have been changed to be more consistent with one another - these should be updated in the front end MSW mocks before merging

Testing instructions

Add a set up instructions describing how the reviewer should test the code

• Review code
• Check Actions build
• Review changes to test coverage

Agile board tracking

Closes #74

[codecov]

Codecov Report

Attention: Patch coverage is 71.95572% with 76 lines in your changes missing coverage. Please review.

Project coverage is 72.52%. Comparing base (3922bff) to head (2c26921).

Files with missing lines	Patch %	Lines
...gement_system_api/routers/v1/catalogue_category.py	0.00%	13 Missing ⚠️
...ventory_management_system_api/routers/v1/system.py	0.00%	11 Missing ⚠️
...management_system_api/routers/v1/catalogue_item.py	0.00%	10 Missing ⚠️
inventory_management_system_api/routers/v1/item.py	0.00%	10 Missing ⚠️
...y_management_system_api/routers/v1/manufacturer.py	0.00%	8 Missing ⚠️
...anagement_system_api/schemas/catalogue_category.py	14.28%	6 Missing ⚠️
inventory_management_system_api/main.py	0.00%	5 Missing ⚠️
inventory_management_system_api/routers/v1/unit.py	0.00%	5 Missing ⚠️
...y_management_system_api/routers/v1/usage_status.py	0.00%	5 Missing ⚠️
inventory_management_system_api/services/item.py	72.72%	3 Missing ⚠️

❌ Your patch check has failed because the patch coverage (71.95%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop     #517       +/-   ##
============================================
+ Coverage    61.26%   72.52%   +11.25%     
============================================
  Files           57       57               
  Lines         2685     2300      -385     
============================================
+ Hits          1645     1668       +23     
+ Misses        1040      632      -408     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[asuresh-code]

Reviewed just the implementation, looks good, most of the suggestions were around documentation/comments.

[asuresh-code]

Since this method no longer directly raises the MissingRecordError, I think it can be removed from the docstrings.

[joelvdavies]

Yeah I wasn't sure what to do around those, my initial instinct was to remove them, but when I tried to look it up I found people suggesting it can also make sense to mention those that are likely to be raised/are uncaught.
https://stackoverflow.com/questions/33262919/should-the-docstring-only-contain-the-exceptions-that-are-explicitly-raised-by-a
https://stackoverflow.com/questions/58635016/should-docstring-contain-a-raises-statement-if-the-error-is-handled-in-the-cod
It would be nice to remove them so we don't need to duplicate them though...

[asuresh-code]

Yeah I could see why it would be beneficial to mention exceptions raised further down the chain. If it was done here, then I think it should be replicated in other functions as well.

My only concern is that in some functions in the router level there could be 8 or 9 exceptions listed. Seems like a lot, but on the other hand might be useful to see all of the exceptions a method could throw, especially at the router level, so you could see everything associated with the operation.

[asuresh-code]

MissingReocrdError can be removed from docstrings.

[asuresh-code]

Can remove MissingRecordError from Docstings.

[VKTB]

If self.get raises a MissingRecordError, then 404 - Catalogue category not found will be returned as the response. Without these changes, 500 - Something went wrong is returned because of the unhandled exception that occurs when .model_dump() is done in the router due to the value returned being None. While I am not sure whether not handling this case is the right thing to do but I believe returning 500 in this case is better than 404.