Skip to content
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

MyDownloads bug fixes #11092

Merged

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented Aug 11, 2023

Summary

This PR generally:
Cleans up/improves reactivity on the frontend
Fixes broken remove download functionality

References

Fixes #11026
Fixes #11044

my-downloads-page.mp4

Reviewer guidance

  1. Import a LOD from an existing facility
  2. Import resources from that facility over the "local network"
  3. Sort and filter requests
  4. Remove requests - removing a single or a selection on download items should work

there will be a follow up PR on Monday that manages:

  • polling for status updates on the downloads
  • error handling for errors when the source device is no longer on the network
  • ensure that the "download" icon is properly reactive both on the content cards and within the content renderer (still inconsistent)

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: medium labels Aug 11, 2023
@marcellamaki marcellamaki force-pushed the my-downloads-bug-fixes branch from 05e76a4 to 1270314 Compare August 11, 2023 20:57
@marcellamaki marcellamaki marked this pull request as ready for review August 11, 2023 21:09
@marcellamaki marcellamaki requested a review from rtibbles August 11, 2023 21:09
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some things that could be cleaned up, at least one thing that looks incorrect to me.

@marcellamaki marcellamaki force-pushed the my-downloads-bug-fixes branch from 1270314 to 6a2447d Compare August 15, 2023 20:30
@marcellamaki marcellamaki force-pushed the my-downloads-bug-fixes branch 3 times, most recently from 3d71b05 to 7e6354d Compare August 15, 2023 22:33
@marcellamaki marcellamaki force-pushed the my-downloads-bug-fixes branch from 7e6354d to 2f0dd63 Compare August 15, 2023 22:37
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main feedback addressed - additional cleanup and tweaks can happen in follow up.

@rtibbles
Copy link
Member

Test failure is due to a flaky test from another PR. Merging, will resolve elsewhere.

@rtibbles rtibbles merged commit 9e5b233 into learningequality:release-v0.16.x Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: backend Python, databases, networking, filesystem... SIZE: medium SIZE: very large
Projects
None yet
2 participants