feat: Import Course Details Page [FC-0112]#2664
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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. 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2664 +/- ##
==========================================
+ Coverage 94.86% 94.88% +0.01%
==========================================
Files 1233 1236 +3
Lines 27940 28102 +162
Branches 6337 6392 +55
==========================================
+ Hits 26505 26664 +159
- Misses 1364 1367 +3
Partials 71 71 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ChrisChV
changed the title
After the import process is completed, it is required to refetch migration block info else the page shows 0 components.
navinkarkera
left a comment
navinkarkera
left a comment
There was a problem hiding this comment.
@ChrisChV I pushed two commits:
- Replace button with link in course card, to navigate user to import details page - Link allows user to see url before clicking and use other options like open in new window/tab.
- The block info was not being fetched in production mode after the import process is completed in async mode. You need to run cms-worker locally and set
CELERY_EAGER_MODE=Falseto test this. It takes a lot of time to set it up so I just fixed the issue and committed it here.
Other than the above issues (should be fixed now), everything looks good. Thanks for the great work!
| <IconButton | ||
| src={ArrowForwardIos} | ||
| alt={intl.formatMessage(messages.courseImportNavigateAlt)} | ||
| onClick={() => navigate(`${courseImport.source.key}/${courseImport.taskUuid}`)} | ||
| /> |
There was a problem hiding this comment.
Having a link is better if the button is only doing navigation, allows user to see url before clicking, open in a new tab etc.
| <IconButton | |
| src={ArrowForwardIos} | |
| alt={intl.formatMessage(messages.courseImportNavigateAlt)} | |
| onClick={() => navigate(`${courseImport.source.key}/${courseImport.taskUuid}`)} | |
| /> | |
| <Link | |
| // window.location.href is required due to browser mistaking course id and taskUuid | |
| // combination as an external link to be opened with an external application. | |
| to={`${window.location.href}/${courseImport.source.key}/${courseImport.taskUuid}`} | |
| aria-label={intl.formatMessage(messages.courseImportNavigateAlt)} | |
| className="text-primary-500" | |
| > | |
| <Icon src={ArrowForwardIos} /> | |
| </Link> | |
PS: You'll need to remove useNavigate, IconButton etc. as they will be unused after above change.
There was a problem hiding this comment.
+1. Generally if it's navigating to a new page it should be a link (even if it looks visually like a button), and if it's doing some action on the current page without changing the URL it should be a button (even if it's styled to look like a link).
|
@navinkarkera Thanks! |
|
|
||
| // In any state other than "in progress", it is no longer necessary | ||
| // to keep refreshing the task status. | ||
| if (enableRefeshState && migrationStatus !== 'In Progress') { | ||
| setEnableRefreshState(false); | ||
| } |
There was a problem hiding this comment.
Why can't we just write
const enableRefeshState = (migrationStatus === 'In Progress');?
There was a problem hiding this comment.
This is because enableRefeshState is used to enable or disable the refetch of useModulestoreMigrationStatus, and that same query is needed to set the value of enableRefeshState.
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
I haven't tested this, but the code looks good now. Thanks!
github-project-automation
Bot
moved this from Waiting on Author
to Done
in Contributions
5 participants
Description
Partial
Failed
Success
In Progress
Supporting information
Testing instructions
make migratein the cms shellOther information
TODO:
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'