Conversation
|
Thanks for the pull request, @Kelketek! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
Kelketek
force-pushed
the
fox/pdf-authoring
branch
from
1926631 to
1847a88
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2916 +/- ##
==========================================
+ Coverage 95.51% 95.56% +0.05%
==========================================
Files 1330 1368 +38
Lines 30582 31418 +836
Branches 6934 7108 +174
==========================================
+ Hits 29211 30026 +815
- Misses 1303 1330 +27
+ Partials 68 62 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
openedx-webhooks
added
the
core contributor
Kelketek
force-pushed
the
fox/pdf-authoring
branch
2 times, most recently
from
01e348a to
129b7e5
Compare
samuelallan72
left a comment
samuelallan72
left a comment
There was a problem hiding this comment.
@Kelketek I tested this locally and found it worked as advertised, including testing with and without PDFXBLOCK_DISABLE_ALL_DOWNLOAD = True in openedx-platform settings. I've made some comments inline for consideration.
...tors/containers/VideoEditor/components/VideoSettingsModal/components/HandoutWidget/hooks.jsx
Outdated
Show resolved
Hide resolved
| }, | ||
| easyMode: { | ||
| id: 'authoring.pdfEditor.widgets.uploadWidget.easyMode', | ||
| defaultMessage: 'Easy Mode', |
There was a problem hiding this comment.
I feel like "easy mode" is slightly confusing, although I'm not sure what it should be. Something for Cassie to look at in product review. :)
src/editors/containers/PdfEditor/components/fields/UploadWidget.tsx
Outdated
Show resolved
Hide resolved
src/editors/containers/PdfEditor/components/fields/UploadWidget.tsx
Outdated
Show resolved
Hide resolved
src/editors/containers/PdfEditor/components/fields/UploadWidget.tsx
Outdated
Show resolved
Hide resolved
src/editors/containers/PdfEditor/components/sections/DownloadOptions.tsx
Outdated
Show resolved
Hide resolved
| {manualMode && <TextField label="PDF Url" id="pdf-url" name="url" />} | ||
| {!manualMode && ( | ||
| <> | ||
| <FileInput supportedFileFormats={supportedFileFormats} fileInput={fileInput} /> |
There was a problem hiding this comment.
Currently the file input behaviour here when you select a file using the "replace" button is:
- Immediately upload the new file to studio assets without confirmation. This means if you change your mind and pick a different file, or cancel the editor altogether, the file is still uploaded to studio assets.
- Ignore any previously uploaded file, leaving it in studio assets.
I don't think this behaviour matches user expectations or the expected meaning of "replace" here. I'm not sure what would be best, but some inline information to explain the behaviour, and/or changing the behaviour, might be helpful here. Eg. adding a warning about how uploads work and where they're uploaded to, and maybe only performing the upload when the editor is saved?
There was a problem hiding this comment.
@samuelallan72 This behavior matches the behavior of the video handout upload field. While I agree that the auto-upload could be surprising, I believe the reason for it makes sense here based on the following:
- The 'save' button isn't something the block can capture and modify the behavior of. That machinery is outside of the editor's control.
- The only way to determine the URL value for the field is to have performed the upload, so it must be done before the user hits save.
- No other field on a block editor requires an additional extra save action. Users expect that hitting 'save' saves everything they've done, and if they forget to hit an additional 'upload' button, they may think things broke.
One way that this field departs from the Handout widget is that there is no 'delete handout' equivalent, because the URL is a required field. Even when you select that option, though, it only sets the field 'null' and doesn't remove the file from the course assets.
There was a problem hiding this comment.
Thanks @Kelketek , that makes sense.
Perhaps just adding some informational text next to the field so users aren't surprised by the behaviour.
There was a problem hiding this comment.
The 'save' button isn't something the block can capture and modify the behavior of. That machinery is outside of the editor's control.
Ah this must be different to the legacy studio editor views then? As I was reminded of for BB-10556, in the branching xblock, we do override the save method.
There was a problem hiding this comment.
@samuelallan72 Yep, it's different. The state management and callbacks are all centralized in the redux definitions. All of the editors take the same path after the save button is hit, with a big if chain changing how the data is massaged before hitting the endpoint.
By contrast, the XBlocks loaded in the legacy view set up their own save buttons and handlers not connected to the redux code, and send a signal to close when they're done.
I'll add a blurb warning the user.
|
Some notes on how I set things up locally for testing this PR along with openedx/openedx-platform#38148 and openedx/xblocks-contrib#193 , since there were several steps. Waffle flagfor the This flag didn't work for me. Testing the frontend
Testing globally disabling the pdf download buttonI used this single-file tutor plugin: And did (This worked fine.) |
Kelketek
force-pushed
the
fox/pdf-authoring
branch
5 times, most recently
from
edaaa5d to
a2dfb41
Compare
|
@Kelketek This is looking great! I have a couple of questions from a UX perspective, mostly to understand the intention a bit better:
|
This file is the default of the PDF XBlock, and predates this MR. It links to this PDF: https://tutorial.math.lamar.edu/pdf/Trig_Cheat_Sheet.pdf . You can learn more about the document by reading the home page: https://tutorial.math.lamar.edu/ Removing it is an option but would expand the scope, since it would mean changing the behavior of the block to accommodate not having the field set and giving the user reasonable feedback about it. I think that's a reasonable improvement, but I'd like to suggest it not be part of this PR.
The source document URL is optional, and is there to give you the source the PDF is from, which might be a Powerpoint, Word Document, or other editable and/or accessible format. If we remove this from Download options, we should also remove Source Button Text, which sets the text of the source download link (perhaps it should be called 'source link text' since the source 'button' appears to just be a link when rendered, but that's what the previous version called it.) That would leave only one option in download options, whether you can download at all. However, that toggle also implies whether you can download the source version, too, which is why they're in a group. When unchecking the 'allow download' button, the source fields disable, as they won't be used. There's no point in providing a source if you're not even providing the rendered document. If you'd like an alternative arrangement (or maybe, an alternative label for the "Download Options" group?), please let me know what you think would work best. |
Kelketek
force-pushed
the
fox/pdf-authoring
branch
6 times, most recently
from
14f6985 to
1203335
Compare
Noted, and agreed. Let's look at this in future. I mocked up a version to improve clarity and flow. If these changes don’t fit within budget, I think the most impactful improvement would be updating the labels and helper text. 
|
Kelketek
force-pushed
the
fox/pdf-authoring
branch
2 times, most recently
from
2e47927 to
1f33696
Compare
|
@cassiezamparini and cc @samuelallan72 Here's the revised interface, based on both of your feedback: pdf-authoring-revised.mp4 |
Kelketek
force-pushed
the
fox/pdf-authoring
branch
2 times, most recently
from
7e59e76 to
e78610a
Compare
|
@samuelallan72 This is ready for review now. Thanks! |
There was a problem hiding this comment.
@Kelketek 👍 looks good to me now! Thanks for addressing the comments and for adding thorough tests.
I left a suggestion for consideration. Also, please update the PR title now that it's no longer a wip.
- I tested this: followed test instructions, spot checked the video editor too since some things have been moved around for that
- I read through the code
- I checked for accessibility issues
- Includes documentation
| fileHint: { | ||
| id: 'authoring.sharedComponents.uploadWidget.fileHint', | ||
| defaultMessage: 'This is the file your learners will see embedded in your course. Files are immediately ' | ||
| + 'uploaded to the file manager.', |
There was a problem hiding this comment.
Maybe something like this to clarify where it ends up?
| + 'uploaded to the file manager.', | |
| + 'uploaded to course assets (Files).', |
|
@Kelketek is it intentional that the "Original File URL" fields are readonly when the "Show PDF download link" is unchecked? |
|
Looks good @Kelketek. Thanks for making those UX changes |
Kelketek
force-pushed
the
fox/pdf-authoring
branch
from
e78610a to
5121d38
Compare
|
Thanks, @samuelallan72 . I think you're right-- I've removed the disabling of those fields. On reflection, it is conceivable that the PDF viewing of the block is really more for in-browser previewing but that the course author would want learners to download the original. I suppose that's the premise of the scope of the 'Document Block' on WGU's end, which is based on this work. I also applied your wording suggestion. This is ready for someone with commit rights to look at. |
Kelketek
force-pushed
the
fox/pdf-authoring
branch
from
5121d38 to
9a1ed3c
Compare
There was a problem hiding this comment.
@Kelketek Thanks for all the nice cleanups here!
This breaks adding PDFs to libraries (v2). Can you please take a look?
Library -> 
-> Advanced/Other -> PDF
Note that in libraries (but not courses), clicking the "PDF" button should open the editor modal without creating a new block, and pressing Cancel at that point does not add anything to the library (this is working with the other editors). I think this could be the main issue, that the PDF editor is trying to load its state from Studio before it has an ID assigned. (We want it to work the same way in course eventually, but I think it's more challenging to do so there.)
Also, the API for uploading files in libraries is different as it's scoped to the component, not the course/library. See the "Text" editor for examples.
The rest of my comments are very minor.
| }, expect.any(Function)); | ||
| }); | ||
|
|
||
| it('adds a PDF block from the advanced selection in modal as a traditional block', async () => { |
There was a problem hiding this comment.
Since the only difference between these two new PDF tests is which editor gets auto-opened after it gets created, can you please clarify that in the test description, and maybe factor out the common code so it's more obvious that 95% of these are the same?
It's also non-obvious from this test code that calling handleCreateNewCourseXBlock with category: pdf opens the legacy editor and calling it with some function opens the React editor.
| import { getConfig } from '@edx/frontend-platform'; | ||
| import { | ||
| FC, useEffect, useState, useMemo, useCallback, | ||
| FC, useEffect, useState, useMemo, useCallback, Fragment, |
There was a problem hiding this comment.
| FC, useEffect, useState, useMemo, useCallback, Fragment, | |
| FC, useEffect, useState, useMemo, useCallback, |
This change seems unnecessary ?
|
|
||
| const queryClient = useQueryClient(); | ||
| // Drop state on unmount so that when we open again we have revised state. | ||
| useEffect(() => () => { | ||
| // We use a unique ID because a component higher up in the chain remounts quickly on | ||
| // startup, and cancelling or invalidating that transaction immediately results in | ||
| // the query key being recreated immediately, and then being populated with the result | ||
| // of being canceled. | ||
| // | ||
| // Invalidating will result in the user seeing the form with stale data before it's | ||
| // refetched. Additionally, the stale flag won't get properly cleared due to the | ||
| // race conditions for the remount. | ||
| // | ||
| // Forcing a unique ID appears to be the most practical workaround. | ||
| const queryKey = ['blockData', blockId, uniqueId]; | ||
| queryClient.removeQueries({ queryKey }); | ||
| }, []); |
There was a problem hiding this comment.
This really feels like it shouldn't be necessary to me. I tried deleting all this code, and after doing so I couldn't figure out how to observe the race condition; I just saw the data loading as expected.
What do you mean by "cancelling or invalidating that transaction" ? Is this something that happens implicitly when the component is remounted?
From the docs is sounds like all you need to do is to modify your useBlockData to not use the cancellation signal provided by React Query:
By default, queries that unmount or become unused before their promises are resolved are not cancelled. This means that after the promise has resolved, the resulting data will be available in the cache. This is helpful if you've started receiving a query, but then unmount the component before it finishes. If you mount the component again and the query has not been garbage collected yet, data will be available.
However, if you consume the AbortSignal, the Promise will be cancelled (e.g. aborting the fetch) and therefore, also the Query must be cancelled. Cancelling the query will result in its state being reverted to its previous state.
There was a problem hiding this comment.
So, this was a pain in the ass to figure out, but the issue is this:
A user who is editing a PDF block might have out of date information when fiddling with it. If they edit it in another window, we want the window they're in to be up to date when they hit the edit button again. This means we want the data evicted on unmount.
In fact, though, you don't even have to be editing it in another window. The information in the query will be out of date when you save. If you open the editor again, it will still be the info it received when it first loaded your block before your edits, meaning it will appear as though you made no changes at all. This is because the mutation is separate.
However, cancelling and attempting to remove the data on unmount when the component is quickly unmounted and remounted makes it to where if the query key is the same, the error bubbles up. And if you try to just do a clean removal without abort, the request eventually finishes, so if a user is currently editing the form, the data suddenly flashes back to the old version and removes your changes.
There was a problem hiding this comment.
If they edit it in another window, we want the window they're in to be up to date when they hit the edit button again.
In such cases, if they've edited it in another window, I think it's totally acceptable that they might see a flash of old data when opening the edit form, before the newest data is loaded in. I don't think that will happen too often, nor is worth complex workarounds. Assuming the configured staleTime is somewhere around the default of five minutes, that issue would only occur if they (1) opened the editor in window A, then (2) edited the same component and saved changes in window B, and then (3) within five minutes, opened the editor again in window A.
In fact, though, you don't even have to be editing it in another window. The information in the query will be out of date when you save. If you open the editor again, it will still be the info it received when it first loaded your block before your edits, meaning it will appear as though you made no changes at all. This is because the mutation is separate.
Hmm, something is fishy here. When the editor modal is open, if you click Save, we want to run the mutation (and invalidate the query!) before the modal editor is closed (because we want to show an error and keep the editor open if the mutation/save fails). Since the editor is still open when the mutation completes and gets invalidated, React Query should trigger a refetch before you close the editor, and when you next open the editor it should already be showing the correct information. No?
You can also use optimistic updates or other techniques to push the new data into the cache when the mutation completes, so that it doesn't matter if the data is ever refetched or not - the cache will hold the latest version as soon you successfully save.
In either case, I don't think you should need this workaround.
There was a problem hiding this comment.
@bradenmacdonald I'll take another look at it, then. This might just be my general unfamiliarity with React Query-- it's my first time using it.
There was a problem hiding this comment.
Apparently when you invalidate a query, you can also specify refetchType: 'all' to force them to refetch immediately, even if the query is not mounted/active. TIL.
There was a problem hiding this comment.
@bradenmacdonald I started looking into this more:
Hmm, something is fishy here. When the editor modal is open, if you click Save, we want to run the mutation (and invalidate the query!) before the modal editor is closed (because we want to show an error and keep the editor open if the mutation/save fails). Since the editor is still open when the mutation completes and gets invalidated, React Query should trigger a refetch before you close the editor, and when you next open the editor it should already be showing the correct information. No?
No. Because the saving infrastructure here is still in Redux, and doesn't use React query. The code which should be invalidating the query is deep in the nested tangle of thunks, far from where I can pull the query client from the context, preventing me from sending the invalidation signal :/
|
|
||
| export type PartialEditorState = RecursivePartial<EditorState>; | ||
|
|
||
| export function initializeStore(preloadedState?: PartialEditorState) { |
There was a problem hiding this comment.
^ This will probably have some conflicts with #1915 which removes initializeStore and consolidates on createStore for creating the editor redux store. Not a big deal.
| `${studioEndpointUrl}/api/contentstore/v2/validate/numerical-input/` | ||
| )) satisfies UrlFunction; | ||
|
|
||
| // There's also a 'handlerUrl', but it's different for some reason. Not sure what one has to do to use the 'v2' URLs. |
There was a problem hiding this comment.
Please update the comments:
POSTing to the /handler_url/ endpoint does not call the handler directly; it returns some data that includes a URL, which you can then use to call the handler later. You can use the resulting URL to run the handler even without cookies / in an unauthenticated iframe.
POSTing to this /handler/ endpoint simply executes the handler directly, and returns the result.
So perhaps handlerUrl above should be renamed to handlerUrlUrl 😛
There was a problem hiding this comment.
@bradenmacdonald I've renamed the old one boundHandlerUrl, and the new one handlerUrl. Let me know if that works.
| // This is coded as a hard limit for handout uploads elsewhere-- seems like an arbitrary thing | ||
| // to have in the frontend, but if that's how it's done... | ||
| if (file.size > 20_000_000) { |
src/editors/supportedEditors.ts
Outdated
| [blockTypes.video_upload]: VideoUploadEditor, | ||
| // ADDED_EDITORS GO BELOW | ||
| [blockTypes.game]: GameEditor, | ||
| [blockTypes.pdf]: PdfEditor, |
There was a problem hiding this comment.
I'm still unclear on what ADDED_EDITORS GO BELOW meant or what GameEditor even is (an example I think?) but I think pdf belongs with the other four above the line //
There was a problem hiding this comment.
@bradenmacdonald I think it's for the Games block. This is something edX has been working on and it looks like they left a stub in here for now. I can move the PDF editor up.
| /> | ||
| </DataTable> | ||
| <FileInput key="generic-file-upload" fileInput={fileInputControl} supportedFileFormats={supportedFileFormats} /> | ||
| <FileInput key="generic-file-upload" fileInput={fileInputControl} supportedFileFormats={supportedFileFormats} id="generic-file-upload-field" /> |
There was a problem hiding this comment.
Are we sure there will only be one <FileTable per page, and we can hard-code an ID like this?
| </div> | ||
| )} | ||
| <FileInput key="transcript-input" fileInput={input} supportedFileFormats={['.srt']} /> | ||
| <FileInput key="transcript-input" fileInput={input} supportedFileFormats={['.srt']} id="transcript-file-input" /> |
There was a problem hiding this comment.
Is this component ever rendered multiple times in the same page?
| "start:with-theme": "paragon install-theme && npm start && npm install", | ||
| "dev": "PUBLIC_PATH=/authoring/ MFE_CONFIG_API_URL='http://localhost:8000/api/mfe_config/v1' fedx-scripts webpack-dev-server --progress --host apps.local.openedx.io", | ||
| "test": "TZ=UTC fedx-scripts jest --coverage --passWithNoTests", | ||
| "test:dev": "TZ=UTC fedx-scripts jest --coverage --watch --passWithNoTests", |
There was a problem hiding this comment.
Do you like having the coverage enabled in this mode? Isn't it slow and annoying? :p
There was a problem hiding this comment.
I do, when trying to make sure I'm getting the coverage up. It usually reruns with just the stuff I've changed, so the slowness isn't too bad. I've gone ahead and removed it, though.
There was a problem hiding this comment.
That's fine, if you like it or use it, leave it in.
Kelketek
force-pushed
the
fox/pdf-authoring
branch
3 times, most recently
from
5ffa555 to
abafb73
Compare
Kelketek
force-pushed
the
fox/pdf-authoring
branch
from
abafb73 to
da0d8fd
Compare
|
@bradenmacdonald This is ready for you again. |
|
@Kelketek Thanks! Nice work. It's much better. There is just some last issue with files in library mode. When I create a new PDF block in a library, it says to save before I can upload files. Fine*. So I save and re-open, and then it says "Unknown filename", but it should say "No file attached". If I then choose "Replace" and upload a PDF, it says "There was an error uploading your file." and the form still shows "Unknown Filename". However, back in the component's side bar under Details tab > Advanced details > Assets, I can see that the PDF was successfully uploaded and attached to the component, so I'm not sure why it's telling me that an error occurred. I can see there are no errors in the CMS logs - the upload endpoint returned 200, and there's nothing in the JS console either. 
*The "Text" block allows you to upload files even before saving in library mode, so it would be good if the PDF block eventually supported that too. I don't want to hold up this PR though; maybe add a TODO comment. You'd just need to store the new files as form data alongside the other changes from the user, rather than immediately uploading them when they're added. |
|
@bradenmacdonald Found the issue, and fixed. On my devstack, the iframe doesn't load like I would expect when in the Library preview, citing CORS issues. I'm not sure if there's anything I should do there. Embedding it in a course shows the issue there as well-- so I'd presume whatever is serving the assets is not sending the expected X-Frame-options header. |
|
@bradenmacdonald I've looked into it more and it may indeed be an issue. The URL that the Library asset view returns goes to a Studio backend URL where the studio serves up the file directly. This is NOT intended to be referenced by blocks directly, according to the docstring. The Text block embeds images using the 'path' instead, which is something like 'static/whatever.png' rather than a full URL. That's not a real path, though. When rendering the block in the course, the embedded URL is transformed in the process somewhere (need to find where, haven't gotten there yet) into the actual target asset URL. However, this process is not automatic. It looks like I'd need to add support to the block in order for it to do this-- so I'm going to need to dive into the HTML code and determine how it can be made to pull the desired asset from the right place. Likely, that will mean I need to adjust the code here to use the path instead, and then I will need to make a PR to xblock-utils to enable this transformation. I'll ping you once that's done. |
|
@bradenmacdonald This is ready, but there is a caveat. I've been able to determine that while the Text editor showed 'static/whatever.png', it actually saved it as Thus, changing the code to use the HOWEVER, the loading of the asset using the library component's preview URL still has the iframe embedding header issue. So I'm going to see if I can sensibly modify the asset preview view to allow iFraming in openedx/openedx-platform#38148 . Failing that, an alternative would be adding in PDF.js and rendering it without using an iFrame-- putting it in a vendor directory and loading it as part of the template. Update: Done! |
6 participants
Description
This pull request adds PDF Authoring.
Visual Changes
Before
pdf-old-screencast.mp4
After
pdf-block-demo.mp4
Supporting information
https://openedx.atlassian.net/wiki/spaces/OEPM/pages/5335908397/Proposal+Add+PDF+Block+to+Base+Installation
Testing instructions
Other information
These changes create much more generalizable parallel functionality to some of the functionality which already exists but is mired in Redux. They're worth examining and may be used by the next developer looking to add official MFE editing support for another block.
Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'