Fix CustomFieldChoices Import #131
Conversation
- Added `pre_import` feature to skip records. - Added more test data. - Skipped importing non-existent content types for `ObjectChange` model. - Separated custom fields and object change customization to distinct module.
snaselj
requested review from
glennmatthews,
lampwins and
chadell
as code owners
| @@ -254,11 +256,13 @@ def import_data(self) -> None: | |||
| else: | |||
| wrapper = self.configure_model(content_type) | |||
|
|
|||
| if wrapper.pre_import: | |||
| if not wrapper.pre_import(data, 1): | |||
There was a problem hiding this comment.
What's the significance of 1 here and 2 below? Looks like you're using it in create_choice_set() to skip some work if it's 1 - can we use constants or something to make this more obvious? Can you document it?
There was a problem hiding this comment.
This is the first pass, described in the documentation and mentioned in the comment few lines above (252)
There was a problem hiding this comment.
Instead of calling pre_import twice with a different number, should we have two separate wrapper functions that get called?
There was a problem hiding this comment.
Using single function here means less code to be used. Most pre_import implementations has common code for both passes.
| @@ -254,11 +256,13 @@ def import_data(self) -> None: | |||
| else: | |||
| wrapper = self.configure_model(content_type) | |||
|
|
|||
| if wrapper.pre_import: | |||
| if not wrapper.pre_import(data, 1): | |||
| continue | |||
There was a problem hiding this comment.
Why doesn't this skip increment wrapper.skipped_count?
There was a problem hiding this comment.
As it is the first pass, this doesn't import data, but enhance the importer structure. Actual skipping is done in the second pass.
| contenttypes.ContentType \ | ||
| dcim.Device \ | ||
| dcim.DeviceRole \ | ||
| dcim.DeviceType \ | ||
| dcim.Manufacturer \ | ||
| dcim.Site \ | ||
| extras.CustomField \ | ||
| extras.CustomFieldChoiceSet \ | ||
| extras.JournalEntry \ | ||
| extras.ObjectChange \ | ||
| ipam.FHRPGroup \ | ||
| tenancy.Tenant \ | ||
| tenancy.TenantGroup \ |
There was a problem hiding this comment.
This is a weirdly specific subset of models - can we document why this specific list was chosen, and why other models were omitted?
There was a problem hiding this comment.
Will improve the documentation in the other PR.
This custom set is a minimal import with things that needs to be tested. Models are going to be added here.
| @@ -62,6 +62,7 @@ class ModelSummary(NamedTuple): | |||
| nautobot_flags: str | |||
| default_reference_uid: Union[str, bool, int, float, None] | |||
| imported_count: int | |||
| skipped_count: int | |||
There was a problem hiding this comment.
Where's the documentation of what these fields mean? skipped_count means number of models skipped? number of instances skipped? or what?
There was a problem hiding this comment.
As this is in related to single model, it means number of models instances, that has been skipped. Will improve the documentation in a separate PR.
Closes: NaN
What's Changed
CustomFieldChoices.pre_importfeature to skip records.ObjectChangemodel.