feat: pdf block

[Kelketek]

Overview

This merge request pulls the PDF block in, extracted from a third party plugin. For now, it does not use its intended block name (pdf), and instead uses a temporary one (_pdf_extracted) as there's more work to do in order to fulfill the scope of the PDF Block Proposal. Once this work is completed, it will be renamed to pdf and be considered generally available. It uses the pdf block tag.

Some minor alterations have been made to the block in order to allow it to be added to the contribs repository. These include the addition of tests, the migration from xblock_utils to xblock.utils, cleanup of lint issues and some spot improvements to docstrings. Otherwise, functionality has been largely left unchanged as this is the first part of the proposal-- moving the block into the contribs library.

Supporting information

• https://openedx.atlassian.net/wiki/spaces/OEPM/pages/5335908397/Proposal+Add+PDF+Block+to+Base+Installation
• https://github.com/open-craft/xblock-pdf

Testing Instructions

1. Bring up a standard modern tutor devstack installation
2. Rather than adding this as a requirement in the config, we must install it manually due to conflicts in the installation order:
   1. Enter the LMS with tutor dev dc exec -it lms -- /bin/bash
   2. Install the package with pip install -e git+https://github.com/open-craft/xblocks-contrib.git@fox/xblock-pdf#egg=xblocks_contrib
   3. Exit the shell and then enter the CMS shell with tutor dev dc exec -it cmd -- /bin/bash
   4. Install the package with pip install -e git+https://github.com/open-craft/xblocks-contrib.git@fox/xblock-pdf#egg=xblocks_contrib
3. In a course, enter the advanced settings and add "_pdf_extracted" to the list of advanced blocks.
4. In the authoring interface, click on the advanced modules and select the PDF Block.
5. Edit and play with it.

Deadlines

This should preferably be merged within the next couple of weeks so that other work depending on it is not blocked. The PDF Block proposal implementation is slated for Verawood.

Author Concerns

This block is fully functional, and while a large number of instances have installed it, it's not part of the standard and it might work just as well to immediately give it its intended name, pdf, and then focus on the changes to the authoring interface (which will likely be added to the frontend authoring MFE), allowing the current interface to be used until this is ready.

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]

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:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

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]

@samuelallan72 I'm not sure how, but it seems I accidentally closed the other PR. Maybe some automation did it?

To answer your questions from there:

We were pulling these changes from the master branch. The initial proposal only focused on the initial contribution of the block as it exists in the community as a first step. The subsequent customizations (such as for Gotenberg document conversion support) are something we plan to do in a subsequent proposal/PRs.

I did find a couple of fixes that only existed in WGU's branch and applied those. One of them was your improvement to the messaging around downloads, and the other was the inclusion of JS linting. I have tentatively included the JS linting here (I had to change the configuration-- it works on the other repo mostly by accident.) The downside of adding JS linting is that we either have to exclude the existing JS files or else start cleaning them up. I've added an example cleanup of the Annotation block's JS in case the latter is the way to go, but we might want upstream to weigh in before committing.

There were minor changes to the JS in the wgu branch-- switching from var to const. However, I didn't port those over since nothing else in this repo does that, and it doesn't fail the linter, believe it or not.

[samuelallan72]

@Kelketek things mostly look good to me - the pdf xblock looks nicely ported and works for me in a devstack.

I commented inline - but my vote would be to drop the extra testing if it means changes leak into other xblocks here. That could be a good follow up in future for improved QA repository-wide.

The only other thing to add would be docs - I'm not sure where docs should canonically live for the xblocks here though, as I don't see docs for any other xblocks in this repo.

We were pulling these changes from the master branch. The initial proposal only focused on the initial contribution of the block as it exists in the community as a first step.

Got it, that makes sense. Thanks for clarifying. 👍

[Kelketek]

Thanks, @samuelallan72 . I've removed the JS linting.

As for the docs, I'm not sure where they are, but you're right that they aren't here. The docs for the included XBlocks must be somewhere else entirely. I expect we'll need to update them when we make the block 'live.'

Earlier I'd suggested something in my description:

This block is fully functional, and while a large number of instances have installed it, it's not part of the standard and it might work just as well to immediately give it its intended name, pdf, and then focus on the changes to the authoring interface (which will likely be added to the frontend authoring MFE), allowing the current interface to be used until this is ready.

I feel like we should only do this if there's an accompanying docs PR, which I'm happy to provide if pointed in the right direction (or even just told 'yes, but go find where the docs should go'-- I can hunt things down if necessary). It could then be updated when the frontend is updated.

[kdmccormick]

Thanks for the PR @Kelketek ! Glad Open edX users will have this basic functionality out of the box going forward.

@feanil and I talked through this today and came to the conclusion that although PDFBlock indeed does belong in the base installation (per your product proposal), we'd like to keep it one degree less-coupled than things like ProblemBlock and HtmlBlock, which the platform literally cannot run without. Our thinking is that you could still add PDFBlock to this repo, but as a new top-level directory (so xblocks_contrib and xblock_pdf would be peer folders, both published to PyPI by the xblocks-contrib package). That would give us the flexibility to move xblock_pdf out if we ever had to, without actually modifying how consumers import the block.

How does that sound? We'd apply the same pattern to other bundled-in-but-not-literally-required blocks, such as xblock_drag_and_drop_v2 and xblock_lti_consumer (Aximprovements had listed them out here, under xblocks-extra-preloaded: openedx/public-engineering#317 (comment)).

Lastly, regarding maintenance-- I appreciate you offering to maintain this new block, and I think it makes sense for someone at OpenCraft to have CC rights on xblocks-contrib (do you have someone in mind?) but I think it would be mechanically easier for Aximprovements to continue maintaining the xblocks-contrib repo as a whole. In exchange, would OpenCraft be willing to take one of these currently-unmaintained repos instead? https://discuss.openedx.org/t/seeking-new-maintainers/18268

FYI @openedx/axim-aximprovements

[Kelketek]

@kdmccormick

I'm not sure I entirely understand how the publishing works for that-- are we saying that the repository holds multiple, individually published PyPi packages that are published separately via CI, but which have common testing tools (linting, tests, etc) OR do you mean 'The single xblocks-contrib package on PyPi will enable you to do both import xblocks_contrib and import xblock_pdf?

If the latter, I'm not sure I've encountered a package that has multiple top-level available imports. Do you have an example where this has been done so I can implement it?

As for your other questions, on CC rights/responsibilities, I don't have an immediate answer to that but I'm pinging @bradenmacdonald who may have some ideas.

[kdmccormick]

'The single xblocks-contrib package on PyPi will enable you to do both import xblocks_contrib and import xblock_pdf?

Yep, this. It's a pretty common pattern in the Python world. Here's an example: https://github.com/openedx/openedx-calc/blob/master/setup.py#L132-L135 . pip install openedx-calc gives you both import calc and import symmath.

[Kelketek]

@kdmccormick Oh, that's rad! I'll get on it, then.

[kdmccormick]

Thanks Fox!

[Kelketek]

@kdmccormick @feanil

I've factored out the PDF block into its own top-level module as requested. Please give it a look and let me know if there's anything else you need. :)

[samuelallan72]

👍 @Kelketek thanks for this, looks good and works on my devstack.

I'd still like to see docs included, even if it's porting the original readme to xblock_pdf/README.md. But I'll leave that up to you, if you prefer to keep it for a follow-up somewhere.

Please also link to the jira ticket from the PR description. :)

• I tested this: followed test instructions
• I read through the code
• I checked for accessibility issues
• Includes documentation (to be added later possibly)

[kdmccormick]

Cool, thanks, one of us will leave a full review soon and then we can get this merged. Before we merge, let us know about the maintenance question.

[kdmccormick]

do you expect more development or do you think this is Ready to Use in its current state?

[Kelketek]

@kdmccormick It's definitely useable as-is today, though the proposal indicated that we need to improve the frontend. We've determined that it will likely require adding authoring capability to frontend-app-authoring for it in order to satisfy the requirements effectively.

However, people use it all the time already. We could promote it to Ready to Use, but I didn't want to presume since we haven't yet done that frontend work. The frontend work wouldn't change the way the fields work in any meaningful way, as far as I can tell.

[kdmccormick]

ok, your call, just make a note to come update this if you leave it as In Dev for now

[Kelketek]

@kdmccormick I think I'd like to make it Ready to Use, then. Follow-up questions:

1. Should I change the name from _pdf_extracted to pdf?
2. If so, is there a recommended place to document this PDF's existence and functionality? As @samuelallan72 mentioned, this PR doesn't include any documentation primarily because there doesn't seem to be any documentation for the other blocks here, either.

[kdmccormick]

@Kelketek great!

1. yes, please do that regardless, the _extracted suffixes are meant to be for the blocks which we're extracting out of openedx-platform and have given temporary tag names to just for testing.
2. The rest of the these blocks are documented in the "Educators" tree of the docs.openedx.org repo. I think it's fair to do the same for PDF, since it will now be enabled for all educators. How about adding a page as a child of this one? https://docs.openedx.org/en/latest/educators/navigation/content_creation_management.html

[Kelketek]

Thanks, @kdmccormick . I've updated the reference and added relevant comments. @samuelallan72 I'm scheduling a follow-up ticket for next sprint to add the documentation there.

[Kelketek]

@kdmccormick The docs for this were merged.

[Kelketek]

@kdmccormick I wanted to give you an update here on what we're thinking for maintainership/contribution in response to your question. We discussed internally and decided that it would be good if I be nominated to join the CCs, take maintainership of web-fragments, and then get commit access here. I think I'd be a good candidate for commit access here since I'll have brought in the PDF block and I was the original author of Polls/Surveys, so this seems like a natural place for me to pitch in.

Braden will be starting a nomination thread for me soon. Let me know if you have any objections here, or if that works for you.

[kdmccormick]

thanks @Kelketek , that sounds great!

[Kelketek]

@kdmccormick I have rebased this PR and also refactored the tests to use functional style since I realized that using the old Junit-style unittest classes wasn't necessary here and we're using Pytest.

Now that I'm onboarding as a CC with maintainership rights to web fragments, and commit access here, the ownership issue should be resolved. Please let me know if there's anything else you need from me here.

[kdmccormick]

Looks great. Thanks Fox!

[kdmccormick]

I noticed there are no JS tests. I realize there are no JS tests in the source block, either. And this repo doesn't have any JS tests yet for you to follow the pattern of, but it will shortly once we merge #63 .

Could I ask you to swing back any time in the 1-6 month window and add a couple trivial JS tests to the PDFBlock? The JS is very minimal, so all we'd need is a smoke-test-level thing to ensure that the block doesn't fully break as we do routine upgrades. I wrote up an issue for it here: #170 ?

[Kelketek]

@kdmccormick Yep, can do!

[kdmccormick]

Released in v0.11.0: https://github.com/openedx/xblocks-contrib/releases/tag/v0.11.0

[Kelketek]

@kdmccormick Are you planning to create the platform version bump PR or should I? Is it usually the maintainer's responsibility or the contributor's? Or is there not a convention?

[kdmccormick]

It varies. If you'd like to see in platform now, go ahead in bump it and I can approve. Otherwise ,the Aximprovements team will be releasing new versions and upgrading them into platform soon enough, as part of the ongoing extraction work.

[Kelketek]

@kdmccormick Looks like the inclusion of xblocks-contrib is not pinned and is updated when a pip-tools upgrade is done, so I'm guessing there's some process (Renovate?) which will be handling this.

[kdmccormick]

For platform, requirements are upgraded on a weekly basis in bulk PR, although sometimes it takes a bit longer to merge the PR when there are breaking changes in some of those requirements. All that said, it'll passively be upgraded within the next couple weeks if you do nothing now. If you want it rolled out now, you can just updated the pin to ==0.11.0 in all the .txt files.

[Kelketek]

Thanks @kdmccormick -- I've thrown a PR up there and pinged you on it. :)