EDD-19: As a download app user, I want to be able to clear my download history. #28
Conversation
Codecov Report
@@ Coverage Diff @@
## main #28 +/- ##
==========================================
+ Coverage 93.26% 93.36% +0.09%
==========================================
Files 102 103 +1
Lines 1960 1988 +28
Branches 432 433 +1
==========================================
+ Hits 1828 1856 +28
Misses 132 132
|
eudoroolivares2016
force-pushed
the
EDD-19
branch
from
65c56d1
to
a9d4aba
Compare
eudoroolivares2016
requested review from
dpesall,
macrouch,
trevorlang,
daniel-zamora and
rushgeo
| @@ -48,7 +48,7 @@ describe('DownloadHistoryHeader component', () => { | |||
| const button = screen.getByRole('button', { name: 'Clear Download History' }) | |||
| await userEvent.click(button) | |||
|
|
|||
| expect(clearDownloadHistory).toHaveBeenCalledTimes(0) | |||
| expect(clearDownloadHistory).toHaveBeenCalledTimes(1) | |||
There was a problem hiding this comment.
Add expect(clearDownloadHistory).toHaveBeenCalledWith({})
| @@ -90,6 +92,16 @@ const DownloadHistoryListItem = ({ | |||
| restartDownload({ downloadId }) | |||
| }, | |||
| icon: FaInfoCircle | |||
| }, | |||
| { | |||
| label: 'Clear Download', | |||
There was a problem hiding this comment.
This isn't going to show up when a download errored fetching links because of line 66 const shouldShowActions = state !== downloadStates.errorFetchingLinks. We need to ensure the user can also delete the download, but they shouldn't be able to perform the other actions if the state is errorFetchingLinks
| }) => { | ||
| const { downloadId = '' } = info | ||
|
|
||
| if (downloadId.length > 0) { |
There was a problem hiding this comment.
The database function also has a check for if downloadId exists, I think this function can just have a single call without caring what downloadId is database.clearDownloadHistoryDownloads(downloadId)
| */ | ||
| async clearDownloadHistoryDownloads(downloadId) { | ||
| if (downloadId) { | ||
| await this.db('downloads').delete().where({ id: downloadId }) |
There was a problem hiding this comment.
After this.db(), put each chained call on its own line
await this.db('downloads')
.delete()
.where({ id: downloadId })
| @@ -344,6 +344,80 @@ describe('EddDatabase', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| describe('clearDownloadHistoryDownloads', () => { | |||
| describe('when a downloadId is provided', () => { | |||
| test('sets the download to inactive', async () => { | |||
There was a problem hiding this comment.
'deletes the download, files and pauses'
| false | ||
| ]) | ||
|
|
||
| query.response([ |
There was a problem hiding this comment.
This response doesn't look like what pluck does, shouldn't it be ['mock-download-id1', 'mock-download-id2']?
There was a problem hiding this comment.
I think it was because pluck is doing that sql statement but, then its calling lodash.map underneath. I removed puck and opted to just call lodash directly
There was a problem hiding this comment.
I think instead of including another dependency in the file, we should perform the query with joins, and also avoid the first select query. I think this would also work
await this.db('pauses')
.delete()
.join('downloads', { 'pauses.downloadId': 'downloads.id' })
.where({
'downloads.active': false
})
| } | ||
|
|
||
| // Pluck documentation: https://knexjs.org/guide/query-builder.html#pluck | ||
| const inactiveDownloads = await this.db('downloads') |
There was a problem hiding this comment.
inactiveDownloadIds better describes what the variable is
…setupListeners; bump version
Overview
What is the feature?
Adding being able to clear items in the download history with the
Clear Download Historyand using a button on theActionsWhat is the Solution?
Adding database call and supporting function to remove items from the database
What areas of the application does this impact?
The downloadHistoryPage of the application
List impacted areas.
Testing
Reproduction steps
Add downloads, use the clear downloads button on the downloads page. On the download history page remove the downloads with the clear downloads button. Clear all downloads with the clear download history button on the header
Attachments
Please include relevant screenshots or files that would be helpful in reviewing and verifying this change.
Checklist
versionfield in package.json and rannpm install