Adding section on Data Downloads for Pending Tasks#74
Adding section on Data Downloads for Pending Tasks#74diana-villalvazo-wgu merged 1 commit intoopenedx:mainfrom
Conversation
|
Thanks for the pull request, @jacobo-dominguez-wgu! 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 @@
## main #74 +/- ##
==========================================
+ Coverage 0.00% 75.68% +75.68%
==========================================
Files 5 63 +58
Lines 7 765 +758
Branches 0 147 +147
==========================================
+ Hits 0 579 +579
- Misses 7 181 +174
- Partials 0 5 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
557f88d to
d475d43
Compare
|
can you rebase your PR against main pls? |
65129d3 to
6a754c6
Compare
|
Hi @jacobo-dominguez-wgu and @diana-villalvazo-wgu! Is this still in progress? |
wgu-jesse-stewart
left a comment
There was a problem hiding this comment.
This is great - good separation of concerns with dedicated components and hooks. Nice work. I just have a few suggestions and some questions.
- What's the polling/refresh strategy? Auto-refresh every X seconds?
- Should completed tasks eventually disappear from this list?
- Is there a maximum number of tasks displayed?
Thanks! |
src/components/PendingTasks.tsx
Outdated
| <DataTable | ||
| columns={tableColumns} | ||
| data={tasks} | ||
| isLoading={isLoading} |
There was a problem hiding this comment.
This is always be false by the time we get to this branch because the return on line 31. Is this a required prop?
There was a problem hiding this comment.
removed
src/components/PendingTasks.test.tsx
Outdated
|
|
||
| const { container } = renderWithProviders(<PendingTasks />); | ||
| const toggleButton = screen.getByRole('button'); | ||
| await waitFor(() => toggleButton.click()); |
There was a problem hiding this comment.
waitFor is meant for assertions that need to wait for async state updates, does this click event need it? Could you use await userEvent.click(toggleButton); here?
There was a problem hiding this comment.
You are right, removing all of them,
src/components/ObjectCell.tsx
Outdated
|
|
||
| const ObjectCell = ({ value }: ObjectCellProps) => { | ||
| return ( | ||
| <pre style={{ whiteSpace: 'pre-wrap', wordBreak: 'break-word' }}> |
There was a problem hiding this comment.
I don't love inline styles like this, can you find an alternative? Maybe the "text-nowrap" helper will work for your needs? https://paragon-openedx.netlify.app/foundations/css-utilities/
wgu-jesse-stewart
left a comment
There was a problem hiding this comment.
Approved, but with a few questions/comments
02151dc to
92cc116
Compare
src/data/apiHook.ts
Outdated
| export const usePendingTasks = (courseId: string) => { | ||
| return useQuery({ | ||
| queryKey: pendingTasksQueryKey.byCourse(courseId), | ||
| queryFn: async () => fetchPendingTasks(courseId), |
There was a problem hiding this comment.
should we enable this only if there is a courseId?
| Cell: DownloadCustomCell, | ||
| } | ||
| ]} | ||
| RowStatusComponent={() => null} |
There was a problem hiding this comment.
i think we can remove this prop if it is not used
src/dataDownloads/data/api.ts
Outdated
|
|
||
| export const generateReportLink = async (courseId, reportType) => { | ||
| const { data } = await getAuthenticatedHttpClient() | ||
| // TODO: Validate if url is correct once the new API endpoint is available |
There was a problem hiding this comment.
this 2 are still a todo?
| export const useGeneratedReports = (courseId: string) => ( | ||
| useQuery({ | ||
| queryKey: queryKeys.generatedReports(courseId), | ||
| queryFn: () => getGeneratedReports(courseId), |
There was a problem hiding this comment.
same do we need to add enable only when there is a courseId?
92cc116 to
fb86c48
Compare
|
@jacobo-dominguez-wgu this PR is ready to merge? or it needs to be merged after downloads page #68 ? |
It has a lot of code from that pr, it still needs to be merged after. |
fb86c48 to
1505219
Compare
1505219 to
906bea9
Compare
|
@diana-villalvazo-wgu @wgu-jesse-stewart This PR is not being blocked anymore, Can you give a final review? |
Description
This pr adds the section of Pending Tasks into the Data Downloads Tab to display the ongoing report generation tasks, in case of no tasks in progress it displays a "No tasks currently running" message.
Supporting information
It needs to be reviewed after #68 since it includes part of the code from that pr.
Closes #44
Testing instructions
npm install && npm run devPOST
http://local.openedx.io:8000/courses/:courseId:/instructor/api/list_instructor_taskswith this response data: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'