Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add StudioContentNode class to allow scripted curation of existing content from Studio #412

Merged
merged 6 commits into from
Jul 21, 2023

Conversation

jamalex
Copy link
Member

@jamalex jamalex commented Mar 25, 2023

In order to support the ability to create a new channel that includes content nodes that already exist on Kolibri Studio, this PR introduces a new node class called StudioContentNode, which is provided with a reference to the IDs of content nodes that already exist on Studio, along with potential overrides of existing metadata such as the title, tags, or thumbnail. Studio then clones the existing node from its source channel and puts it into the appropriate spot in this newly cheffed channel.

This PR depends on changes @rtibbles shall shortly open a PR for on https://github.com/learningequality/studio.

@@ -615,6 +619,68 @@ def validate(self):
assert len(tag) <= 30, (
"ERROR: tag " + tag + " is too long. Tags should be 30 chars or less."
)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just moved up from a subclass to ensure it gets run for all node types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same with the "role" stuff above)

@jamalex jamalex force-pushed the remote_content_nodes branch from d85aa7a to d464ee9 Compare March 25, 2023 03:32
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me and the examples make it clear how it can be used - just had non-blocking thought on the example code.

One thought was that it might be a bit more consistent and clear to use Studio in place of Remote in the naming scheme since that's where we're getting the node from.


SOURCE_DOMAIN = "testdomain.org" ## change me!

original_channel_data = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I read through this it was odd that this null-filled dict was being referenced in a class definition until I got to the bottom and saw how the values were being set before the constructor call.

I think that this would read a bit more clearly if the values for this were passed as kwargs to the CuratedChannelChef constructor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this was that we need somewhere "global" to store the computed IDs that are generated during the creation of the original channel -- which would be messy to pull out later through introspection of that channel.

In terms of how they then get passed into the curated chef, I agree that arguments would be nicer. But currently the kwargs passed into a chef constructor are then just ignored completely and not stored anywhere. There are lots of things around these ricecooker idioms I'd love to rearchitect, but I was picking my battles for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For now I'll add a comment here to make this clearer, though)

@jamalex
Copy link
Member Author

jamalex commented Apr 28, 2023

One thought was that it might be a bit more consistent and clear to use Studio in place of Remote in the naming scheme since that's where we're getting the node from.

Yeah, this was @rtibbles' recommendation to match the naming pattern of RemoteFile that's already in there (which is used to avoid needing to reupload the same file later when it's already on Studio). I agree that a Studio prefix would be more explicit (since it doesn't allow an arbitrary remote URL etc). @rtibbles, what do you think of a rename across both? (with perhaps a deprecated alias for RemoteFile for backcompat?)

@rtibbles
Copy link
Member

rtibbles commented May 2, 2023

This change seems fine, but the backcompat alias would be helpful.

Then in 3 years' time when we allow ricecooker to run against Kolibri we can rename it to Remote again and leave the Studio alias for back compat :)

@jamalex jamalex changed the title Add RemoteContentNode class to allow scripted curation of existing content from Studio Add StudioContentNode class to allow scripted curation of existing content from Studio Jul 9, 2023
@jamalex
Copy link
Member Author

jamalex commented Jul 9, 2023

  • Added comment to clarify global dict purpose
  • Changed names of this and RemoteFile, with backcompat aliases
  • Fixed IncompleteRead tests based on this by downgrading urllib3 version. Would it be preferable to run with that version only in the Github Actions context, or have the test/production envs be consistent?

@rtibbles rtibbles merged commit 5c9f8e1 into learningequality:develop Jul 21, 2023
@jamalex jamalex deleted the remote_content_nodes branch August 10, 2023 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants