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.81% 94.82%
========================================
Files 1225 1230 +5
Lines 27515 27672 +157
Branches 6036 6076 +40
========================================
+ Hits 26089 26239 +150
- Misses 1368 1375 +7
Partials 58 58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
| const [selectedCourseId, setSelectedCourseId] = useState<string>(); | ||
|
|
||
| return ( | ||
| <ModalDialog |
There was a problem hiding this comment.
From the design, it seems that this is not a Modal. The import home route is library/:libraryId/import. Maybe you can create a route like ../import/courses for it?
Also, it would make it easier to reuse the sidebar.
There was a problem hiding this comment.
The modal is a requirement in the ticket #2524, but I asked to product: #2524 (comment)
| extraFilter={extraFilter} | ||
| overrideTypesFilter={overrideTypesFilter} | ||
| > | ||
| {getConfig().ENABLE_COURSE_IMPORT_IN_LIBRARY === 'true' && ( |
There was a problem hiding this comment.
If we go for the route, I think we don't need this flag, as we can test it directly using the url.
There was a problem hiding this comment.
The env flag is a requirement in the ticket #2524
|
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. |
5 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.Other 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'