fix: set upstream link for re-copied block from course originally from library #35784

navinkarkera · 2024-11-06T10:38:22Z

Description

Sets upstream link to library block for blocks that were copied from a course block which were originally copied/imported from a library.

Fix for #35752

Supporting information

https://github.com/openedx/edx-platform/issues/35752\
Private-ref: FAL-3935

Testing instructions

Follow instructions from issue: https://github.com/openedx/edx-platform/issues/35752\

Deadline

ASAP

openedx-webhooks · 2024-11-06T10:38:29Z

Thanks for the pull request, @navinkarkera!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/wg-maintenance-edx-platform. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

pomegranited

👍 Works perfectly, great job @navinkarkera ! Left a couple of minor suggestions.

I tested this using the instructions from the issue with the [contentstore.new_studio_mfe.use_new_unit_page](http://local.edly.io:8000/admin/waffle/flag/43/change/) waffle flag disabled, so I could use the legacy Studio UI to see the "update available" links.
I read through the code
~~I checked for accessibility issues by using my keyboard to navigate~~ N/A
Includes great documentation :)
~~User-facing strings are extracted for translation~~ N/A

pomegranited · 2024-11-07T01:57:06Z

cms/djangoapps/contentstore/helpers.py

+    * the xblock is copied from a v2 library; the library block is set as upstream.
+    * the xblock is copied from a course; no upstream is set, only copied_from_block is set.
+    * the xblock is copied from a course where the source block was imported from a library; the original libary block
+      is set as upstream.


Your documentation here is so good -- it's really helpful to see all the many ways we manage links between blocks. Thank you for taking the time to explain!

pomegranited · 2024-11-07T02:02:22Z

cms/djangoapps/contentstore/helpers.py

+    if copied_from_block is None:
+        return


[opinion] Can we make copied_from_block a required string parameter instead of returning here? I think that would be clearer.

Suggested change

if copied_from_block is None:

return

assert copied_from_block

Did not add assert, but updated the type and moved the check to caller function.

pomegranited · 2024-11-07T02:03:37Z

cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py

+        # first verify link for copied block from library
+        new_block_key = _copy_paste_and_assert_link(self.lib_block_key)
+        # next verify link for copied block from the pasted block
+        _copy_paste_and_assert_link(new_block_key)


Oo that's nice!

pomegranited · 2024-11-07T02:25:04Z

cms/djangoapps/contentstore/helpers.py

@@ -323,6 +323,58 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) ->
    return new_xblock, notices


+def fetch_and_set_upstream_link(


This function seems pretty local to _import_xml_node_to_parent.. should it be an internal function to discourage use outside of this module?

Suggested change

def fetch_and_set_upstream_link(

def _fetch_and_set_upstream_link(

Makes sense. Updated.

…m library Sets upstream link to library block for blocks that were copied from a course block which were originally copied/imported from a library.

navinkarkera · 2024-11-07T05:41:12Z

cms/djangoapps/contentstore/helpers.py

@@ -436,7 +465,7 @@ def _import_xml_node_to_parent(
        # Allow an XBlock to do anything fancy it may need to when pasted from the clipboard.
        # These blocks may handle their own children or parenting if needed. Let them return booleans to
        # let us know if we need to handle these or not.
-        children_handed = new_xblock.studio_post_paste(store, node)
+        children_handled = new_xblock.studio_post_paste(store, node)


Typo in variable causing children_handed to always be False.

navinkarkera · 2024-11-07T05:41:52Z

cms/djangoapps/contentstore/helpers.py

+    if copied_from_block is None:
+        return


Did not add assert, but updated the type and moved the check to caller function.

navinkarkera · 2024-11-07T05:42:09Z

cms/djangoapps/contentstore/helpers.py

@@ -323,6 +323,58 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) ->
    return new_xblock, notices


+def fetch_and_set_upstream_link(


Makes sense. Updated.

…m library (openedx#35784) Sets upstream link to library block for blocks that were copied from a course block which were originally copied/imported from a library. (cherry picked from commit d82aada)

…m library (#35784) (#35801) Sets upstream link to library block for blocks that were copied from a course block which were originally copied/imported from a library. (cherry picked from commit d82aada)

edx-pipeline-bot · 2024-11-07T16:28:48Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-11-07T16:48:13Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-11-07T16:48:13Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-11-07T17:36:17Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-11-07T17:55:11Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-11-07T17:55:11Z

2U Release Notice: This PR has been deployed to the edX production environment.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 6, 2024

navinkarkera marked this pull request as ready for review November 6, 2024 10:44

kdmccormick self-requested a review November 6, 2024 16:35

pomegranited approved these changes Nov 7, 2024

View reviewed changes

navinkarkera added 3 commits November 7, 2024 11:06

fix: set upstream link for re-copied block from course originally fro…

26045de

…m library Sets upstream link to library block for blocks that were copied from a course block which were originally copied/imported from a library.

refactor: apply review suggestions

f7470d2

fix: typo

dac4072

navinkarkera force-pushed the navin/fix-copy-copied-upstream branch from bcc1fb5 to dac4072 Compare November 7, 2024 05:39

navinkarkera commented Nov 7, 2024

View reviewed changes

ChrisChV approved these changes Nov 7, 2024

View reviewed changes

ChrisChV merged commit d82aada into openedx:master Nov 7, 2024
49 checks passed

ChrisChV deleted the navin/fix-copy-copied-upstream branch November 7, 2024 14:54

navinkarkera mentioned this pull request Nov 7, 2024

[sumac] fix: set upstream link for re-copied block from course originally from library (#35784) #35801

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: set upstream link for re-copied block from course originally from library #35784

fix: set upstream link for re-copied block from course originally from library #35784

navinkarkera commented Nov 6, 2024

openedx-webhooks commented Nov 6, 2024 •

edited

Loading

pomegranited left a comment

pomegranited Nov 7, 2024

pomegranited Nov 7, 2024

navinkarkera Nov 7, 2024

pomegranited Nov 7, 2024

pomegranited Nov 7, 2024

navinkarkera Nov 7, 2024

navinkarkera Nov 7, 2024

navinkarkera Nov 7, 2024

navinkarkera Nov 7, 2024

edx-pipeline-bot commented Nov 7, 2024

edx-pipeline-bot commented Nov 7, 2024

edx-pipeline-bot commented Nov 7, 2024

edx-pipeline-bot commented Nov 7, 2024

edx-pipeline-bot commented Nov 7, 2024

edx-pipeline-bot commented Nov 7, 2024

		@@ -323,6 +323,58 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) ->
		return new_xblock, notices


		def fetch_and_set_upstream_link(

	def fetch_and_set_upstream_link(
	def _fetch_and_set_upstream_link(

fix: set upstream link for re-copied block from course originally from library #35784

fix: set upstream link for re-copied block from course originally from library #35784

Conversation

navinkarkera commented Nov 6, 2024

Description

Supporting information

Testing instructions

Deadline

openedx-webhooks commented Nov 6, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edx-pipeline-bot commented Nov 7, 2024

edx-pipeline-bot commented Nov 7, 2024

edx-pipeline-bot commented Nov 7, 2024

edx-pipeline-bot commented Nov 7, 2024

edx-pipeline-bot commented Nov 7, 2024

edx-pipeline-bot commented Nov 7, 2024

openedx-webhooks commented Nov 6, 2024 •

edited

Loading