[ESoCC] Command to create channel versions #5660
Conversation
I tried just assigning it in the create() call but got a complaint about the direction of my assignment wrt the m2m relationship
nucleogenesis
force-pushed
the
esocc--channel-version-command
branch
from
4cf92bc to
d895d69
Compare
nucleogenesis
force-pushed
the
esocc--channel-version-command
branch
from
d895d69 to
a70ad56
Compare
| def test_channel_with_existing_special_permissions_in_published_data(self): | ||
| """ | ||
| If published_data already contains special_permissions_included, | ||
| the command should not recompute it. | ||
| """ |
There was a problem hiding this comment.
This feels right even though I don't think this would run into the problem unless we fail and rerun it - I'm not sure what the plan for running the command would look like so guidance there would be greatly appreciated!
There was a problem hiding this comment.
Yes, it is right just as it is now! This could be replicated in unstable since we already have some AuditedSpecialPermissionsLicense created there, but the behavior is correct!
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks a lot @nucleogenesis! Just noted a couple of things that I didn't note while writing the issue 😅.
| def test_channel_with_existing_special_permissions_in_published_data(self): | ||
| """ | ||
| If published_data already contains special_permissions_included, | ||
| the command should not recompute it. | ||
| """ |
There was a problem hiding this comment.
Yes, it is right just as it is now! This could be replicated in unstable since we already have some AuditedSpecialPermissionsLicense created there, but the behavior is correct!
| # to the most recent ChannelVersion instance | ||
| for channel in channels.iterator(): | ||
| logging.info(f"Processing channel {channel.id}") | ||
| last_created_ch_ver = None |
There was a problem hiding this comment.
Although it seems that this method is working fine, I am not sure if we can rely on the channel.published_data.values()returning always the most recent version at the end. I think a safer approach could be to check if pub_data.get("version") === channel.version.
| last_created_ch_ver = ChannelVersion.objects.create( | ||
| channel=channel, | ||
| version=pub_data.get("version"), | ||
| included_categories=pub_data.get("included_categories", []), | ||
| included_licenses=pub_data.get("included_licenses"), | ||
| included_languages=pub_data.get("included_languages"), | ||
| non_distributable_licenses_included=pub_data.get( | ||
| "non_distributable_licenses_included" | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Ah, apologies, I didn't specify this in the issue 😅, the ChannelVersion model has more fields besides the ones we need to compute here. Here, you can see all values we set to the channelVersion object on the publish flow, so we would need to set all of them here too.
Additionally, just to play safe, I think we can use the create_or_update method so that in case the command is re-run or in case we have some channel versions already set up (in unstable, for example), it doesn't fail because channel and version are unique together.
| logging.info( | ||
| f"Validating published data for channel {channel.id} version {pub_data['version']}" | ||
| ) | ||
| special_permissions = validate_published_data(pub_data, channel) |
There was a problem hiding this comment.
Just noting that validate_published_data is updating the published_data object by computing properties using the current published_nodes, which means that these computed properties only belong to the most recent version. Therefore, annotating these properties in previous versions would be an error. Since we don't need previous versions to have these computed properties, we don't need to care about computing them, and we can just call this validate_published_data if pub_data corresponds to the current channel version.
- Add TestValidatePublishedData class with direct unit tests for the validate function covering: return type, included_licenses, included_languages, included_categories, kind_counts, non_distributable_licenses, and special_permissions - Remove test_command_skips_channels_with_existing_version_info (didn't actually test idempotency) - Remove test_channel_with_existing_special_permissions_in_published_data (wasn't testing meaningful behavior) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks @nucleogenesis! Running the command found a couple of minor mismatches, nothing big 😄.
| # Validate published_data | ||
| for pub_data in channel.published_data.values(): | ||
| logging.info( | ||
| f"Validating published data for channel {channel.id} version {pub_data['version']}" |
There was a problem hiding this comment.
Whoops, it seems that we are not storing the version field inside each pub_data here, so this is returning an error because of that. It seems we need to get the version from the key of the object.
| valid_data, special_permissions = validate_published_data( | ||
| pub_data, channel | ||
| ) |
There was a problem hiding this comment.
Just noting that we should not validate/nor do any updates to the pub_data of versions that are not the last one, because we are computing this data with the current published nodes, not with the published nodes of any previous versions.
| "size": int(channel.published_size), | ||
| "resource_count": channel.total_resource_count, |
There was a problem hiding this comment.
These size and resource_count should also be obtained from the valid_data object, because we should rely on the data stored there; channel.published_size only matches with the most recent version.
Besides that, comparing with what we put into our published_data, it seems we are missing the date_published and version_notes.
| if not data.get("included_categories"): | ||
| included_categories_dicts = published_nodes.exclude( | ||
| categories=None | ||
| ).values_list("categories", flat=True) | ||
| data["included_categories"] = sorted( | ||
| set( | ||
| chain.from_iterable( | ||
| ( | ||
| node_categories_dict.keys() | ||
| for node_categories_dict in included_categories_dicts | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| if not data.get("included_languages"): | ||
| node_languages = published_nodes.exclude(language=None).values_list( | ||
| "language", flat=True | ||
| ) | ||
| file_languages = published_nodes.exclude(files__language=None).values_list( | ||
| "files__language", flat=True | ||
| ) | ||
| data["included_languages"] = list(set(chain(node_languages, file_languages))) |
There was a problem hiding this comment.
Here, for included_languages and included_categories that are already present in data we should do an additional validation: we should ensure that no present item is null. Previously with languages (not sure if with categories too), because of an error in the query, we were including null values in the included_languages list, but this was causing problems, and now the ChannelVersion doesn't support it.
| return AuditedSpecialPermissionsLicense.objects.bulk_create( | ||
| new_licenses, ignore_conflicts=True | ||
| ) |
There was a problem hiding this comment.
It seems that bulk_create returns the same list of new_licenses even if they were not actually created, and when we want to set this to the ChannelVersion node, it fails because some objects don't exist in the db. It seems it's safer to return just this after the bulk_create:
return AuditedSpecialPermissionsLicense.objects.filter(
description__in=special_perms_descriptions
)
| if not data.get("kind_counts"): | ||
| data["kind_counts"] = list( |
There was a problem hiding this comment.
Just noting that the key here should be called kind_count (in singular)
| for pub_data in channel.published_data.values(): | ||
| logging.info( | ||
| f"Validating published data for channel {channel.id} version {pub_data['version']}" | ||
| ) | ||
| valid_data, special_permissions = validate_published_data( | ||
| pub_data, channel | ||
| ) | ||
|
|
||
| # Create or update channel version | ||
| channel_version, _ = ChannelVersion.objects.update_or_create( | ||
| channel=channel, | ||
| version=valid_data.get("version"), | ||
| defaults={ | ||
| "included_categories": valid_data.get("included_categories"), | ||
| "included_licenses": valid_data.get("included_licenses"), | ||
| "included_languages": valid_data.get("included_languages"), | ||
| "non_distributable_licenses_included": valid_data.get( | ||
| "non_distributable_licenses_included" | ||
| ), | ||
| "kind_count": valid_data.get("kind_count"), | ||
| "size": int(channel.published_size), | ||
| "resource_count": channel.total_resource_count, | ||
| }, | ||
| ) | ||
|
|
||
| if channel.version == pub_data.get("version"): | ||
| channel.version_info = channel_version | ||
|
|
||
| # Set the M2M relation for special permissions | ||
| if special_permissions: | ||
| channel_version.special_permissions_included.set( | ||
| special_permissions | ||
| ) | ||
| logging.info( | ||
| f"Created channel version {channel_version.id} for channel {channel.id}" | ||
| ) |
There was a problem hiding this comment.
While running the command, I saw that if an error happened, the command exited. I think it'd be great if we wrapped the inner for body in a try-except block and saved the channel, version, and exception for those that failed to an array. At the end of the command, we can log that array. Just in case something happens, we have the info there. How does that sound?
2 participants
Summary
Adds a create_channel_versions command that uses the published_data dict on Channels to create new ChannelVersion instances.
I worked out the initial approach and some simple tests myself and asked Claude to help extrapolate the tests I made to include checks for things like the Audited license models and such.
References
Closes #5593
Reviewer guidance
Have I missed anything?