Add number_of_spares field to catalogue items #416

[joelvdavies]

Description

See #416.

Testing instructions

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

Agile board tracking

Closes #416

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.92%. Comparing base (ec9d81d) to head (b742092).
Report is 20 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #426   +/-   ##
========================================
  Coverage    97.92%   97.92%           
========================================
  Files           45       45           
  Lines         1592     1594    +2     
========================================
+ Hits          1559     1561    +2     
  Misses          33       33           

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

[asuresh-code]

Looks good overall, have some minor comments to make.

[asuresh-code]

Suggested change

	default=None, description="Number of spares currently available within this catalogue item if known"
	default=None, description="The number of spares currently available within this catalogue item"

All of the other descriptions start with The and if known doesn't seem necessary since other optional fields don't include this text.

[joelvdavies]

True, the others I have done have not as I thought it was unnecessary to have the The, but I will for the sake of keeping it the same in the file, the rest can be unified later. I think the if known is worth emphasising in this case? They others are directly edited by the user, so I expect if there is an optional description then they would understand that when its None it just doesn't have one rather than it not being known, whereas the spares it is specifically because it isn't known not just that it doesn't have one.

[asuresh-code]

Yeah I think it makes sense to leave if known in for this case. I thought there were some fields similar to number_of_spares (Optional, not known but exists), but I just realised I was looking at the Patch Schema where everything is made Optional

[asuresh-code]

More of a general comment about the migration scripts, but we now have 2 separate migration scripts for catalogue items, for the expected_lifetime and number_of_spares fields respectively. If we do migrations with the expected_lifetime script on the database currently, wouldn't they fail, since there will be a new field number_of_spares that would cause the OldCatalogueItem and NewCatalogueItem models to fail?

[joelvdavies]

More of a general comment about the migration scripts, but we now have 2 separate migration scripts for catalogue items, for the expected_lifetime and number_of_spares fields respectively. If we do migrations with the expected_lifetime script on the database currently, wouldn't they fail, since there will be a new field number_of_spares that would cause the OldCatalogueItem and NewCatalogueItem models to fail?

Yes that is correct, the migrations have to be done in the right order at the right time. I had the same thought and created another PR to implement something like alembic #430 to address these.

[asuresh-code]

Just some final comments in regards to the mock_data

[asuresh-code]

Suggested change

	**CATALOGUE_ITEM_DATA_REQUIRED_VALUES_ONLY,
	"catalogue_category_id": str(ObjectId()),
	"manufacturer_id": str(ObjectId()),
	"number_of_spares": None,
	}
	**CATALOGUE_ITEM_DATA_REQUIRED_VALUES_ONLY,
	"catalogue_category_id": str(ObjectId()),
	"manufacturer_id": str(ObjectId()),
	}

It's optional, so it shouldn't be included in the REQUIRED_VALUES_ONLY?

[joelvdavies]

The reason it is like this is because the CatalogueItemBase doesn't have it set to None automatically. The idea being that with the actual calculation, I never want it to default to anything - it should only be None if the calculation itself returns it, but forgetting to set it will cause an error and prompt a developer to use the calculation to populate it instead. The others are set to None because if you forget to set it, its fine.

[asuresh-code]

In the mock_data.py, number_of_spares is included in the INDATA and GETDATA catalogue item entities, but not the DATA catalogue item entities. I understand why it's included in the GETDATA ones, but shouldn't they be omitted from the INDATA examples, since they are not inputted during Catalogue Item Patch/Post requests? Similarly, I assumed that DATA entities represent the attributes catalogue items possess when stored in the database, in which case shouldn't they include number_of_spares?

[joelvdavies]

In the mock_data.py, number_of_spares is included in the INDATA and GETDATA catalogue item entities, but not the DATA catalogue item entities. I understand why it's included in the GETDATA ones, but shouldn't they be omitted from the INDATA examples, since they are not inputted during Catalogue Item Patch/Post requests? Similarly, I assumed that DATA entities represent the attributes catalogue items possess when stored in the database, in which case shouldn't they include number_of_spares?

I have put some comments at the top of the mock_data.py to try and explain these models. All the _IN_DATA's should match the In models, so they reflect what should go into the repository methods and into the database, which in this case is different to the patch/post schemas because they are not user assignable and are only calculated internally (similar to the comment I made above). The _DATA only dictionaries are for post schemas, and I don't think I do add the number_of_spares to them? You are right that those shouldn't because a user can't assign them. The _GET_DATA on the other hand does because a user getting a catalogue item needs to see the number of spares in the returned schema (its view only).

[asuresh-code]

I have put some comments at the top of the mock_data.py to try and explain these models. All the _IN_DATA's should match the In models, so they reflect what should go into the repository methods and into the database, which in this case is different to the patch/post schemas because they are not user assignable and are only calculated internally (similar to the comment I made above). The _DATA only dictionaries are for post schemas, and I don't think I do add the number_of_spares to them? You are right that those shouldn't because a user can't assign them. The _GET_DATA on the other hand does because a user getting a catalogue item needs to see the number of spares in the returned schema (its view only).

Thank you for the explanation; helped clear everything up.