[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.
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?