PDF: Remove legacy view & add tests by Kelketek · Pull Request #257 · openedx/xblocks-core

Kelketek · 2026-06-05T18:28:05Z

Description

This merge request adds tests for the PDF Block initializer. It also removes the backend rendered studio view now that the authoring MFE directly supports PDF blocks.

Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.

Testing instructions

Add this package to your LMS devstack, either as a mount or as an entry in OPENEDX_EXTRA_PIP_REQUIREMENTS with a tutor dev launch afterward
Add pdf to the advanced modules list for a course
Add a PDF, and verify it loads. Edit it and verify the studio editor still works through the MFE.
In the repository root, run npm run setup and then npm run test. Verify the output shows the PDF block tests (they're likely last)

Other information

Closes #170

Merge checklist:
Check off if complete or not applicable:

Version bumped
Changelog record added
Documentation updated (not only docstrings)
Fixup commits are squashed away
Unit tests added/updated
Manual testing instructions provided
Noted any: Concerns, dependencies, migration issues, deadlines, tickets

openedx-webhooks · 2026-06-05T18:28:11Z

Thanks for the pull request, @Kelketek!

This repository is currently maintained by @openedx/axim-engineering.

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.

Kelketek · 2026-06-05T20:50:15Z

@kdmccormick I could use your advice (and perhaps preliminary review) here.

As requested, I've added JS tests for the PDF block. I noted the following:

It looks like each block has its own build configuration.
These build configurations are completely independent
I don't see much reason to use webpack and Karma here if we don't have to since we're starting fresh-- so I used raw typescript and vitest (I'd have used vite but getting it to behave correctly as a non-module wasn't working as I'd have expected)
The blocks each build their JS into a 'public' directory, whose contents are not committed.
It's not clear WHEN these builds would happen, however, outside of local development and CI. How are these blocks able to ensure their files are available where they expect?
Similarly, the Python tests aren't running these builds, so testing the views which load the compiled JS files seems to be not a thing for the rest of the blocks-- I guess they originally used Bok Choy tests for that or similar, years ago.

The other blocks do work currently, right? How are they ensuring their files are built before serving them? Am I taking a totally backward approach?

Kelketek · 2026-06-09T20:14:49Z

@kdmccormick Nevermind-- decided not to have the bootstrapper built, nor use the 'public' folder, and instead only add tests rather than a build system for the general case, since it's not really necessary here anyway.

Kelketek · 2026-06-09T20:14:57Z

@kdmccormick Oh, and-- ready for you :)

samuelallan72

On a clean clone, the tests fail with:

node:fs:1350
  const result = binding.mkdir(
                         ^

Error: ENOENT: no such file or directory, mkdir '/home/samuel/proj/opencraft/xblocks-core/xblocks_contrib/video/public/js'

I don't think this is related to this PR though, since this is for the video xblock. These tests pass if I manually created that dir. Although it passes in CI, so I'm not sure what I missed here.

Please see other comments/questions inline. :)

samuelallan72 · 2026-06-11T02:37:52Z

+      const tag = document.createElement('div')
+      tag.setAttribute('data-testid', 'jquery-loaded')
+      document.body.appendChild(tag)
+      clearInterval(id)


How does it access id here? 🤔

The scope of id is in the parent, so it's available to the child. const allows shadowing and global access if the name is defined higher. It prevents polluting outer scopes and redefinition, though.

Try this in your browser console:

const id = setInterval(() => console.log(id), 100)

@Kelketek that's so weird! I knew about the scopes, but it felt weird that it was fine accessing the variable that was defined here (after or at the same time as the function was defined). I see that this also works which feels even more wrong:

const id = setInterval(() => console.log(foo), 100) const foo = "hello";

🤔 🤔

@samuelallan72 Does it seems less weird if it's a function?

const doThing = () => { scream() setTimeout(() => doThing(), 100) } function scream () { console.log("AHHHH!") } doThing()

@Kelketek ah yes, that does seem less weird. Nice. :) I guess this is usual recursion, but just looked weird to me in this context. 👍

samuelallan72 · 2026-06-11T02:50:12Z

+  "scripts": {
+    "setup": "playwright install",
+    "check": "tsc --noEmit",
+    "test": "npm run build && vitest run",


Does the build subcommand call need to be there? Currently it fails for me because there's no build subcommand. (not sure why it succeeds in CI)

Nope, I forgot to replace it with check and only tested test-ci after I did it there.

samuelallan72 · 2026-06-11T02:51:19Z

@@ -1,58 +0,0 @@
-{% load i18n %}


Does this then drop support for openedx < ulmo?

< Verawood. The included PDF feature only arrived in Verawood, and had the MFE editing when Verawood was cut. It didn't have it when initially merged, which means no release has had the included PDF block with the backend-rendered editor enabled. Only master has had it briefly.

It is possible to override use of the PDF block with one of the older versions, though, if you need to.

samuelallan72 · 2026-06-11T03:03:18Z

+    return new Promise((resolve) => setTimeout(resolve, ms));
+}
+
+const waitFor = async <T>(func: () => T, timeout: number = 1000) => {


vitest includes a waitFor function: https://vitest.dev/api/vi.html#vi-waitfor . Can we use that instead of implementing our own?

Ah! I thought it was only available in testing-library, and I didn't want to add an entire library for one function.

samuelallan72 · 2026-06-11T03:04:04Z

+    document.body.appendChild(block)
+}
+
+const loadJsPayload = async (path: string, id?: string) => {


Could you add a comment about why we need this kind of loading? I found it confusing.

Kelketek · 2026-06-11T19:05:22Z

@samuelallan72 This is ready for another look. Thanks!

samuelallan72 · 2026-06-12T00:52:04Z

@Kelketek 👍

(please link to https://tasks.opencraft.com/browse/BB-10571 from the PR description :) )

I tested this: followed the test instructions
I read through the code
I checked for accessibility issues
Includes documentation

Kelketek · 2026-06-12T15:21:59Z

@kdmccormick Went ahead and got it an internal review here, which it's passed. Could you take a look?

Kelketek · 2026-07-07T18:13:04Z

@kdmccormick Do you have an idea when you'll be able to take a look at this? Thanks!

kdmccormick

Thanks for the PR! Generally looks great, just ~~two~~ one request.

kdmccormick · 2026-07-08T11:58:37Z

@@ -0,0 +1,19 @@
+{


It looks like the other blocks in this repo have a package.json for the block itself (not just the tests) and then use devDependencies to declare tests deps. Does that pattern seem reasonable to follow here, too? The setup script would become something like setup-tests

@kdmccormick The other blocks have an assets directory where the package.json is defined, and then that builds out and sticks the assets in the public directory, which is .gitignored. Then Karma serves the files from there to run its tests. This is a lot of indirection and doesn't quite make it clear what the scope of the script is-- it gave me the impression that we were building the JS using npm and then eventually serving that in production somehow, but I couldn't find any mechanism for doing that, unless you're aware of one?

Vitest doesn't require this additional 'build to asset directory' step, so the kind of hopping around with directories isn't needed.

Additionally, we're not building any assets for the PDF block. I attempted to build the bootstrap script from there, but there wasn't a good TypeScript config that would work consistently with the kind of in-browser stuff it does-- especially not with newer versions.

There's one JS file, which only serves as a bootstrap point. If and when we need to start building JS for display of the PDF block, I'd suggest we tackle any reconciliation then, as the build is likely to be significantly different to how we do it for other blocks.

Thanks, that all makes sense. Do you mind calling it npm run setup-tests or something that makes it's clear that it's just installing test deps?

I believe what's actually happening for the other blocks is that they get shoved into the openedx-platform static asset build (not great) and that the static asset build here is just for tests. This is something we've been meaning to untangle.

kdmccormick

LGTM pending my suggestion about setup -> setup-tests

kdmccormick · 2026-07-09T19:04:28Z

Released: https://github.com/openedx/xblocks-core/releases/tag/v0.18.0

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Jun 5, 2026

openedx-webhooks added this to Contributions Jun 5, 2026

github-project-automation Bot moved this to Needs Triage in Contributions Jun 5, 2026

Kelketek marked this pull request as draft June 5, 2026 18:28

Kelketek force-pushed the fox/js-tests branch 6 times, most recently from 3f30cc1 to 7ec98e1 Compare June 5, 2026 19:33

Kelketek changed the title ~~Fox/js tests~~ PDF JS tests Jun 5, 2026

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Jun 8, 2026

Kelketek force-pushed the fox/js-tests branch 5 times, most recently from dad29e5 to 672ef99 Compare June 9, 2026 19:57

Kelketek marked this pull request as ready for review June 9, 2026 20:13

samuelallan72 suggested changes Jun 11, 2026

View reviewed changes

Kelketek force-pushed the fox/js-tests branch from ad47259 to f6015cf Compare June 11, 2026 19:02

samuelallan72 approved these changes Jun 12, 2026

View reviewed changes

feat!: remove PDF studio view

40136f2

Kelketek force-pushed the fox/js-tests branch from f6015cf to d0156ef Compare June 12, 2026 15:05

kdmccormick self-requested a review July 1, 2026 20:37

kdmccormick changed the title ~~PDF JS tests~~ PDF: Remove legacy view & add tests Jul 8, 2026

kdmccormick reviewed Jul 8, 2026

View reviewed changes

kdmccormick approved these changes Jul 9, 2026

View reviewed changes

Kelketek force-pushed the fox/js-tests branch 2 times, most recently from 3e61b9f to 58501dd Compare July 9, 2026 18:55

Kelketek added 2 commits July 9, 2026 13:57

test: pdf block student initializer

7a4cee8

chore: bump version

140bb46

Kelketek force-pushed the fox/js-tests branch from 58501dd to 140bb46 Compare July 9, 2026 18:57

kdmccormick approved these changes Jul 9, 2026

View reviewed changes

kdmccormick merged commit 91acb70 into openedx:main Jul 9, 2026
8 of 9 checks passed

github-project-automation Bot moved this from Waiting on Author to Done in Contributions Jul 9, 2026

Kelketek deleted the fox/js-tests branch July 9, 2026 19:00

Uh oh!

Conversation

Kelketek commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Other information

Uh oh!

openedx-webhooks commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kelketek commented Jun 5, 2026

Uh oh!

Kelketek commented Jun 9, 2026

Uh oh!

Kelketek commented Jun 9, 2026

Uh oh!

samuelallan72 left a comment

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!

samuelallan72 Jun 11, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelallan72 Jun 11, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kelketek Jun 11, 2026 • 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!

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!

Kelketek commented Jun 11, 2026

Uh oh!

samuelallan72 commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kelketek commented Jun 12, 2026

Uh oh!

Kelketek commented Jul 7, 2026

Uh oh!

kdmccormick 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!

Choose a reason for hiding this comment

Uh oh!

Kelketek Jul 8, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

kdmccormick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kdmccormick commented Jul 9, 2026

Kelketek commented Jun 5, 2026 •

edited

Loading

openedx-webhooks commented Jun 5, 2026 •

edited

Loading

samuelallan72 Jun 11, 2026 •

edited

Loading

samuelallan72 Jun 11, 2026 •

edited

Loading

Kelketek Jun 11, 2026 •

edited

Loading

samuelallan72 commented Jun 12, 2026 •

edited

Loading

kdmccormick left a comment •

edited

Loading

Kelketek Jul 8, 2026 •

edited

Loading