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

fix: CCX LTI configuration compatibility #391

Merged

Conversation

kuipumu
Copy link
Contributor

@kuipumu kuipumu commented Jul 10, 2023

Description

This PR adds compatibility fix for CCXs and external reusable configurations to the LTI consumer XBlock. This also includes changes to previously added fixes for CCXs using the LTI consumer XBlock. This changes allow the LTI consumer XBlock to sync values from the main CCX LTI configuration model to all of it's children CCX LTI configurations and vice versa.

Type of Change

  • Remove previous CCX fixes.
  • Add model_to_dict function to utils module.
  • Add LtiConfiguration sync_configurations method.
  • Modify LtiConfiguration save method.
  • Include tests for all added and modified code.

Testing

  1. Create a course and add a LTI consumer XBlock.
  2. Create a CCX of the previously created course.
  3. Launch the CCX course unit with the LTI consumer XBlock.
  4. Go to http://localhost:18000/admin/lti_consumer/lticonfiguration/.
  5. Both the main LTI configuration and it's children CCX configuration should have the same values.
  6. Update any value on the main LTI configuration.
  7. Go to the children CCX LTI configuration.
  8. The values from the main LTI configuration should be reflected.
  9. Delete both LTI configurations.
  10. Launch the CCX course unit with the LTI consumer XBlock.
  11. Launch the course unit with the main LTI consumer XBlock.
  12. Go back to the LTI configuration admin and search for both newly created configurations.
  13. Both LTI configurations values should be synced.

Co-authored-by: Squirrel18 <daniel.quiroga@edunext.co>
Co-authored-by: alexjmpb <alexander.mendoza@edunext.co>
@openedx-webhooks
Copy link

openedx-webhooks commented Jul 10, 2023

Thanks for the pull request, @kuipumu! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 10, 2023
@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 11, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02% 🎉

Comparison is base (1d781f9) 97.89% compared to head (227cb98) 97.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
+ Coverage   97.89%   97.91%   +0.02%     
==========================================
  Files          77       77              
  Lines        6399     6475      +76     
==========================================
+ Hits         6264     6340      +76     
  Misses        135      135              
Flag Coverage Δ
unittests 97.91% <100.00%> (+0.02%) ⬆️

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

Files Changed Coverage Δ
lti_consumer/__init__.py 100.00% <100.00%> (ø)
lti_consumer/models.py 92.15% <100.00%> (+0.40%) ⬆️
lti_consumer/tests/unit/test_models.py 100.00% <100.00%> (ø)
lti_consumer/tests/unit/test_utils.py 100.00% <100.00%> (ø)
lti_consumer/utils.py 91.37% <100.00%> (+0.99%) ⬆️

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

@mphilbrick211
Copy link

Hi @kuipumu! Is this ready for review/merge? Thanks!

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 12, 2023
@kuipumu
Copy link
Contributor Author

kuipumu commented Jul 12, 2023

@mphilbrick211 Yes, this is ready for review/merge

@mphilbrick211
Copy link

Hi @chennighan2u and @openedx/masters-devs-cosmonauts! Could someone please review and merge this for us? Thanks!

@mphilbrick211
Copy link

Hi @kuipumu! Some branch conflicts have popped up while waiting for review - would you mind taking a look?

@kuipumu
Copy link
Contributor Author

kuipumu commented Aug 3, 2023

Hi @mphilbrick211, I added a commit to resolve the branch conflicts

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Aug 7, 2023
Copy link
Contributor

@Zacharis278 Zacharis278 left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍

@kuipumu before we merge can you add a changelog entry and update init.py with the new version?

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Aug 9, 2023
@kuipumu
Copy link
Contributor Author

kuipumu commented Aug 11, 2023

@Zacharis278 Great!, I just added a commit to bump the version an update the changelog

@mphilbrick211
Copy link

Hi @kuipumu! Looks like some branch conflicts have popped up. Would you mind taking a look? Thanks!

@kuipumu
Copy link
Contributor Author

kuipumu commented Aug 20, 2023

Hi @mphilbrick211, I pushed a commit to resolve the conflicts

@Zacharis278 Zacharis278 merged commit 3de9bcb into openedx:master Aug 21, 2023
7 checks passed
@openedx-webhooks
Copy link

@kuipumu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

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.

None yet

4 participants