Add endpoint to set spares definition #413

[joelvdavies]

Description

See #413. Went with the aggregate query approach as an experiment, we could equally just loop though and insert the values. There is no need to insert all usage status fields as I have done at the moment.

Notes

• Had a conflict with some of the pytest setup fixtures when inheriting so renamed all in e2e tests to be consistent
• Moved some id_dicts to their base class e.g. instead of having usage_status_value_id_dict in catalogue category's create dsl, it is now populated in the usage status create dsl to avoid repeating in cases like this

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 #413, relates to #90

[codecov]

Codecov Report

Attention: Patch coverage is 98.93617% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.92%. Comparing base (3010334) to head (6e00914).
Report is 19 commits behind head on develop.

Files with missing lines	Patch %	Lines
inventory_management_system_api/schemas/setting.py	94.11%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #421      +/-   ##
===========================================
+ Coverage    97.87%   97.92%   +0.05%     
===========================================
  Files           41       45       +4     
  Lines         1504     1592      +88     
===========================================
+ Hits          1472     1559      +87     
- Misses          32       33       +1     

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

[asuresh-code]

Looks Good overall, and seems to work well. Have some minor changes/comments on the implementation, testing is good except for a small typo.

[asuresh-code]

Currently, both MissingRecordErrors and InvalidObjectIdErrors return a 422 in the response. They both have the same response detail which is good, although I think they should both be set to 404? In the Object Storage get Image endpoint, I believe we came to that conclusion, and makes sense for it to be the same here.

[asuresh-code]

In the Settings Schema, it says L54 is not covered by tests, although I can see there is a duplicate_usage_status test that checks for the error, so I'm not sure if I'm missing something or the tool has made a mistake.

[asuresh-code]

Suggested change

	SettingInBaseT = TypeVar("SettingInBaseT", bound=SettingInBase)
	SettingOutBaseT = TypeVar("SettingOutBaseT", bound=SettingOutBase)
	SettingInBaseT = TypeVar("SettingInBaseT", bound=SettingInBase)
	SettingOutBaseT = TypeVar("SettingOutBaseT", bound=Union[SettingOutBase, SettingInBaseT])

I think this should ensure that SettingOutBaseT is the same settings as the InBase (i.e Would not include SparesDefinitionIn and ExpiryDefinitionOut), but again it runs fine as it is without errors, so similar to the last comment.

[joelvdavies]

I think having the union here would actually mean it can be any subtype of SettingOutBase or SettingInBase (https://stackoverflow.com/questions/59933946/difference-between-typevart-a-b-and-typevart-bound-uniona-b). I don't think there is a way to have them coupled like that unfortunately.

[joelvdavies]

Currently, both MissingRecordErrors and InvalidObjectIdErrors return a 422 in the response. They both have the same response detail which is good, although I think they should both be set to 404? In the Object Storage get Image endpoint, I believe we came to that conclusion, and makes sense for it to be the same here.

In this case it should be a 422 I think like the others, the difference is that it is not in the url path e.g. /image/asdasdasd should be a 404, but posting something to /image that has an invalid id in it would be a 422. So here as its the usage status inside the body of the request that doesn't exist or is invalid then it should be a 422 as the request could not be processed.

[joelvdavies]

duplicate_usage_status

Yes codecov is correct as it excludes e2e tests in its coverage checking. We have generally ignored unit testing validation in the schemas in unit tests due to them usually being created inside the router level when a request is made, so without adding a test file for schemas it makes sense to use the e2e tests to validate them instead but as a consequence they are left untested in unit tests and so show up on the coverage e.g. https://app.codecov.io/gh/ral-facilities/inventory-management-system-api/pull/421/blob/inventory_management_system_api/schemas/catalogue_category.py.

[asuresh-code]

Looks good overall, just some more final minor changes.

[asuresh-code]

LGTM!

[VKTB]

Works well, just some minor things.