feat: import course in library stepper [FC-0112] #2567
Conversation
|
Thanks for the pull request, @ChrisChV! 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. |
openedx-webhooks
added
the
open-source-contribution
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2567 +/- ##
==========================================
- Coverage 94.82% 94.82% -0.01%
==========================================
Files 1231 1234 +3
Lines 27629 27764 +135
Branches 6221 6266 +45
==========================================
+ Hits 26199 26326 +127
- Misses 1359 1380 +21
+ Partials 71 58 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rpenido
left a comment
rpenido
left a comment
There was a problem hiding this comment.
LGTM 👍
Thank you for your work, @ChrisChV!
- I tested this using the instructions from the PR
- I read through the code
- I checked for accessibility issues
- Includes documentation
Left some nits about disabling useQuery and the use of a Modal here.
src/library-authoring/import-course/stepper/ImportStepperModal.tsx
Outdated
Show resolved
Hide resolved
|
One product question (CC @edschema): we are using the Is this the expected behaviour? |
This is a good question. I want to bring this to the product team; I'll get back to you soon. Is the data structure/ permissions/ access handled in a way that we can potentially show to a user that a course has been imported into a different library? I think it makes sense to show only in the first case below, but there are some reasons why I want to talk this over: if we support converting course content to library references if a course was previously imported to a library/ whether we want to give this visibility in case a team is in process of importing all courses to libraries and are worried about unintentionally importing a course twice. Three import cases:
|
@edschema Permissions are based on the user's permissions for each course. If a user has staff permissions for a course, they can view the libraries where the course has been imported, regardless of whether they have permission to those libraries. However, this permission only extends to the library title, not the content.
So, the first and second cases would work without a problem. In the third case, the library title would still be displayed; if we don't want to display the library title, we would have to add extra validations, but it is possible. |
Hi @edschema, Is there an answer for this case? |
|
Hi @ChrisChV, thanks for checking back on this. The badge should show whether a course has been imported into this particular library. For this flow, we do not surface if a course has been imported into another library. |
|
Is the empty state import button out of scope for this PR? 
|
src/studio-home/card-item/index.tsx
Outdated
| <Form.Radio | ||
| value={itemId} | ||
| name={`select-card-item-${itemId}`} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| // Multiple | ||
| return ( | ||
| <Form.Checkbox value={itemId} /> |
There was a problem hiding this comment.
These radio buttons / checkboxes are missing accessibility labels. Screen reader users won't know what they're for.
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Thanks for those refactors! Code looks good to me, though I haven't been able to test it yet.
ChrisChV
force-pushed
the
chris/FAL-4266-import-from-course-stepper
branch
from
fa995e2 to
a1aedf5
Compare
|
@bradenmacdonald Thanks! @rpenido could you make another quick test? |
rpenido
left a comment
rpenido
left a comment
There was a problem hiding this comment.
I tested it and it is working as expected @ChrisChV! Good work!
@bradenmacdonald, you can merge it.
github-project-automation
bot
moved this from Waiting on Author
to Done
in Contributions
6 participants
Description
ENABLE_COURSE_IMPORT_IN_LIBRARYflagSupporting information
Previously importedChip, it depends on feat: get migrations info REST-API added [FC-0112] edx-platform#37558Testing instructions
ENABLE_COURSE_IMPORT_IN_LIBRARY(It is enabled by default in the .env.development)Import Course.Previously ImportedChip is in the migrated courses.Next Step.Previously Importedand a library v2 to the simple user.Previously ImportedchipOther information
TODO: Move the Import Course Button to the Import Home Page
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'