-
Couldn't load subscription status.
- Fork 163
[feature] add restore section for libraries v2 #2558
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
[feature] add restore section for libraries v2 #2558
Conversation
|
Thanks for the pull request, @holaontiveros! 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. 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2558 +/- ##
==========================================
+ Coverage 94.78% 94.79% +0.01%
==========================================
Files 1225 1226 +1
Lines 27399 27480 +81
Branches 5992 6200 +208
==========================================
+ Hits 25969 26049 +80
+ Misses 1372 1360 -12
- Partials 58 71 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
64cbbb6 to
33b5386
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it on my local and it seems to be working well.
Grabacion.de.pantalla.2025-10-27.a.la.s.6.20.11.p.m.mov
Just one question and some minor comments:
- Is it expected the zip file not to be required in the form? I mean the user can complete the library creation from archive without uploading a backup file.
- Also to see the feedback when there is an error in the upload process, it is needed to download a log file to view the actual error, it would be more user friendly if the error is shown on the UI as in the figma design.
|
|
||
| {/* Loading state - show spinner in DropZone-like container */} | ||
| {restoreMutation.isPending && ( | ||
| <div |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the spinner from paragon was not used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to make it match the design and align properly, the one from paragon was creating some jumpingness while being shown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, can you please mention that in the code? e.g. TODO: replace this with Paragon spinner - didn't use for now because of ...
Yes it's expected, you may add it or you may not there's no penalization for that (eve if you opened the drozone) Yeah it could be user friendly but currently that error log it's a file stored that could contain a huge amount of info, so as that get's polished it can be improved, the designs didn't take in account the final shape of the APIs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works great. Nice work. I'm wondering if we can provide some feedback to the Paragon team for the alignment of the spinner component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just have some minor suggestions for consistency with other parts of the codebase.
|
|
||
| {/* Loading state - show spinner in DropZone-like container */} | ||
| {restoreMutation.isPending && ( | ||
| <div |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, can you please mention that in the code? e.g. TODO: replace this with Paragon spinner - didn't use for now because of ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me. @holaontiveros could you please get someone from your team to test it once more to make sure it's working correctly after the camelCase refactoring? Then ping me to merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything seems to be working well after pr comments addressed! LGTM 👍🏻
Description
This PR adds a new section for the v2 libraries which will allow the user to use a backup file while creating a new library
Useful information to include:
"Developer", and "Operator".
Supporting information
Covers the second part of #2448
Implements the second part of the views this figma design
Screenshots:
Before dropzone:

Dropzone displayed:

Loader while waiting for upload and task result:

Error state returned from the status endpoint:

Succesful upload:

Testing instructions
Assuming you have a working authoring dev environment and you have the right version of the edx-platform (latest master which includes: openedx/edx-platform#37439) and you have builded your dev image with that latest change, given that learning-core lives outsied of edx-platform
Other information
Include anything else that will help reviewers and consumers understand the change.
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).propTypes,defaultProps, andinjectIntlpatterns are not used in any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'