feat: new editor design by rpenido · Pull Request #642 · openedx/xblock-lti-consumer

rpenido · 2026-04-09T03:17:50Z

This PR implements a new Edit dialog for the LTI Consumer block

Testing instructions

Private ref: FAL-4322

openedx-webhooks · 2026-04-09T03:17:56Z

Thanks for the pull request, @rpenido!

This repository is currently maintained by @Faraz32123.

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.

Details

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.

navinkarkera

@rpenido The editing experience is much better now!

Left some comments about minor typos and small bugs caught by AI.

I noticed that on clicking fullscreen button the modal goes to left side as seen in the screenshot and the Previous button gets misaligned. Is this expected?

navinkarkera · 2026-04-13T09:59:13Z

+      // Transform the custom_parameters field into a list
+      if ("custom_parameters" in submitData.values) {
+        const customParameters = submitData.values.custom_parameters;
+        submitData.values.custom_parameters = JSON.parse(customParameters);


We should probably use try-catch block for JSON.parse of user input.

navinkarkera · 2026-04-13T09:59:17Z

+      // Transform the lti_1p3_redirect_uris field into a list
+      if ("lti_1p3_redirect_uris" in submitData.values) {
+        const redirectUris = submitData.values.lti_1p3_redirect_uris;
+        submitData.values.lti_1p3_redirect_uris = JSON.parse(redirectUris);


navinkarkera · 2026-04-13T10:01:24Z

+
+      runtime.notify("save", { state: "start" });
+
+      $.ajax({


Missing contentType: 'application/json'.

navinkarkera · 2026-04-13T10:09:25Z

    Arguments:
        course_key (opaque_keys.edx.locator.CourseLocator): Course Key
    """
+    return True


Ops! Thanks!

rpenido · 2026-04-22T22:40:51Z

Thanks for the review @navinkarkera!

Also, can we use include tag to split the html file into smaller chunks?

I tried this, but I couldn't make it work. I think the issue is the Template path, since it seems to be from the CMS, not this package. I tried several path variations and locations without success. Could you just help me check if I'm missing anything super obvious?

I'm getting an error in the tests because I'm not mocking i18n correctly. I also tried many mocks and magic mocks without success. Could you please also help me there? 😬

navinkarkera · 2026-04-25T11:13:36Z

I'm getting an error in the tests because I'm not mocking i18n correctly. I also tried many mocks and magic mocks without success. Could you please also help me there? 😬

@rpenido I pushed a fix for this as well as some minor things like duplicate bindings and label mismatch. Hope it is not a problem.

I tried this, but I couldn't make it work. I think the issue is the Template path, since it seems to be from the CMS, not this package. I tried several path variations and locations without success. Could you just help me check if I'm missing anything super obvious?

Checking now.

codecov · 2026-04-25T11:17:36Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.69%. Comparing base (0b254d9) to head (8fed472).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #642      +/-   ##
==========================================
+ Coverage   97.64%   97.69%   +0.05%     
==========================================
  Files          84       84              
  Lines        7650     7778     +128     
==========================================
+ Hits         7470     7599     +129     
+ Misses        180      179       -1

Flag	Coverage Δ
unittests	`97.69% <100.00%> (+0.05%)`	⬆️

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.

navinkarkera · 2026-04-25T12:03:17Z

@rpenido Pushed one more commit to split html file, I had to render them separately due to how ResourceLoader works. But it works and is cleaner.

navinkarkera

@rpenido Just found one bug, other than the PR looks good. Thanks!

Co-authored-by: Navin Karkera <navin@disroot.org>

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 9, 2026

openedx-webhooks added this to Contributions Apr 9, 2026

github-project-automation Bot moved this to Needs Triage in Contributions Apr 9, 2026

rpenido force-pushed the rpenido/FAL-4322/new-editor-design branch from 782ff8f to 36ce6a2 Compare April 9, 2026 03:20

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Apr 9, 2026

navinkarkera reviewed Apr 13, 2026

View reviewed changes

rpenido force-pushed the rpenido/FAL-4322/new-editor-design branch 23 times, most recently from 7d8ddc8 to 4039060 Compare April 17, 2026 22:53

fix: remove description from editable_fields if pii_sharing is disabled

afbd926

fix(studio): fix studio editor interactions

80636ab

refactor(studio): split LTI studio template into partials

40c7532

navinkarkera approved these changes Apr 25, 2026

View reviewed changes

rpenido force-pushed the rpenido/FAL-4322/new-editor-design branch 7 times, most recently from 580f9fc to dfe410a Compare April 29, 2026 03:05

feat: hide fields if external config selected

46d4567

rpenido force-pushed the rpenido/FAL-4322/new-editor-design branch 2 times, most recently from 3f4ae31 to c27f1d3 Compare April 29, 2026 03:52

test: improve coverage

8510fc2

rpenido force-pushed the rpenido/FAL-4322/new-editor-design branch from c27f1d3 to 8510fc2 Compare April 29, 2026 04:11

rpenido added 2 commits April 29, 2026 01:11

Merge branch 'master' into rpenido/FAL-4322/new-editor-design

9d531c8

test: remove unused code

54b932d

rpenido requested review from feanil and navinkarkera April 29, 2026 04:25

rpenido added 2 commits April 29, 2026 02:43

fix: lti-configuration-datails visibility logic

273f7f4

chore: redeploy sandbox

7967617

navinkarkera approved these changes Apr 29, 2026

View reviewed changes

Comment thread lti_consumer/static/js/xblock_studio_view.js

rpenido and others added 3 commits April 29, 2026 09:20

fix: reset visibility of field-group-lti-configuration-details

d557a93

Co-authored-by: Navin Karkera <navin@disroot.org>

fix: prev/next buttons with external config

d6f76de

fix: visibility for EXTERNAL_MULTIPLE_LAUNCH_URLS_ENABLED

8fed472

Conversation

rpenido commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing instructions

Uh oh!

openedx-webhooks commented Apr 9, 2026

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rpenido commented Apr 22, 2026

Uh oh!

navinkarkera commented Apr 25, 2026

Uh oh!

codecov Bot commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

navinkarkera commented Apr 25, 2026

Uh oh!

navinkarkera left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rpenido commented Apr 9, 2026 •

edited

Loading

codecov Bot commented Apr 25, 2026 •

edited

Loading

navinkarkera left a comment •

edited

Loading