Skip to content

fix: make it work in library v2 [FC-0076] #2272

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

Merged
merged 18 commits into from
Apr 17, 2025

Conversation

DanielVZ96
Copy link
Contributor

@DanielVZ96 DanielVZ96 commented Jan 30, 2025

TL;DR - Modifies functionality so that the xblock can be edited and used in the new Library page.

JIRA: JIRA-XXXX
Private-ref

What changed?

  • Uses display name alongside title element in order to correctly display the name in Library Page
  • Adds a few checks to avoid course specific logic when being edited in a Library V2 context
  • Update tests with display name changes

Developer Checklist

Testing Instructions

  1. Any field
  2. The title

Reviewer Checklist

Collectively, these should be completed by reviewers of this PR:

  • I've done a visual code review
  • I've tested the new functionality

FYI: @openedx/content-aurora

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 30, 2025
@openedx-webhooks
Copy link

openedx-webhooks commented Jan 30, 2025

Thanks for the pull request, @DanielVZ96!

This repository is currently maintained by @openedx/2u-aurora.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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.


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.

Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 96.36364% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.29%. Comparing base (90d5de9) to head (952b5d3).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
openassessment/xblock/utils/xml.py 88.23% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2272      +/-   ##
==========================================
- Coverage   95.29%   95.29%   -0.01%     
==========================================
  Files         195      195              
  Lines       21625    21664      +39     
  Branches     1502     1509       +7     
==========================================
+ Hits        20607    20644      +37     
  Misses        771      771              
- Partials      247      249       +2     
Flag Coverage Δ
unittests 95.29% <96.36%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@itsjeyd
Copy link

itsjeyd commented Mar 14, 2025

Hey @DanielVZ96, just checking in to see if you're still planning to continue working on this PR?

@itsjeyd itsjeyd added the inactive PR author has been unresponsive for several months label Mar 14, 2025
@DanielVZ96
Copy link
Contributor Author

@itsjeyd yep. I'll work on this before I go to holidays

@DanielVZ96
Copy link
Contributor Author

I'm only missing updating the tests.

@itsjeyd itsjeyd added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed inactive PR author has been unresponsive for several months labels Apr 1, 2025
@DanielVZ96 DanielVZ96 marked this pull request as ready for review April 5, 2025 22:44
@DanielVZ96 DanielVZ96 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 5, 2025
@itsjeyd
Copy link

itsjeyd commented Apr 10, 2025

@DanielVZ96 I see you marked this PR as ready for review, but it still has a bunch of failing tests as well as some merge conflicts to resolve.

Could you address those before we get the engineering review process going? Or were you looking for some early input on your approach before spending more time on this PR?

@DanielVZ96
Copy link
Contributor Author

@itsjeyd sorry i didn't notice the tests were failing after merging master. I'll fix them now

@DanielVZ96 DanielVZ96 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Apr 13, 2025
@DanielVZ96 DanielVZ96 moved this from Waiting on Author to Ready for Review in Contributions Apr 13, 2025
Copy link

@ChrisChV ChrisChV left a comment

Choose a reason for hiding this comment

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

Looks good 👍

  • I tested this: I followed the testing instructions
  • I read through the code and considered the security, stability and performance implications of the changes.
  • Includes tests for bugfixes and/or features added.
  • Includes documentation

@pomegranited pomegranited changed the title fix: make it work in library v2 fix: make it work in library v2 [FC-0076] Apr 15, 2025
Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

👍 this is working great! Thank you @DanielVZ96 .

  • I tested this by adding a new ORA2 block to a library, copying an ORA2 block from a course to a library, copying an ORA2 block from a library to a course, and editing an ORA2 block in a library and in a course.
  • I read through the code
  • I checked for accessibility issues by using my keyboard to navigate
  • Includes documentation N/A
  • User-facing strings are extracted for translation N/A

@@ -454,7 +455,7 @@ def get_base_url_path_for_course_assets(self, course_key):
"""
Returns base url path for course assets
"""
if course_key is None:
if (course_key is None) or isinstance(course_key, LibraryLocatorV2):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe use the OpaqueKey is_course property here instead, for consistency?

Suggested change
if (course_key is None) or isinstance(course_key, LibraryLocatorV2):
if (course_key is None) or not course_key.is_course:

Copy link
Contributor

Choose a reason for hiding this comment

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

Nudge @DanielVZ96 :) Does this suggestion not work?

@@ -478,6 +479,8 @@ def get_teamsets(self, course_id):
"""
Wrapper around get_team_configuration that returns team names only for display
"""
if isinstance(course_id, LibraryLocatorV2):
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto here:

Suggested change
if isinstance(course_id, LibraryLocatorV2):
if not course_id.is_course:

@pomegranited
Copy link
Contributor

@DanielVZ96 Could you please bump the version here?

Bumped version number in openassessment/init.py and package.json

@DanielVZ96
Copy link
Contributor Author

@pomegranited done!

@pomegranited pomegranited merged commit 078e789 into openedx:master Apr 17, 2025
13 checks passed
@openedx-webhooks openedx-webhooks removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Apr 17, 2025
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in Contributions Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants