Skip to content

Commit

Permalink
Add unauthorized download state
Browse files Browse the repository at this point in the history
When a user downloads a file to a location that they do not have access to, it currently just says "Downloading... 0%". This is to add a download state to indicate that the user doesn't have access to the director they are trying to download into.

Commit list:
- Add new download state
- Update state if directory is unauthorized
- Update electron download state change
- Add 'Unauthorized' download state to 'clear completed downloads' reducer
- Add isUnauthorized getter
- Add unauthorized state as part of condition for hiding progress display
- Add isUnauthorized to condition for rendering a download item
- Set unauthorized as a stop condition for downloads
- Set getL10nId for unauthorized downloads
- Add 'unauthorized' as a style class for finished download items
- Add downloadUnauthorized locale id for all languages
- Update tests
- Fix styling

Fixes brave#7747
  • Loading branch information
syuan100 authored and bsclifton committed Nov 13, 2017
1 parent fe63aae commit 9be39e7
Show file tree
Hide file tree
Showing 10 changed files with 33 additions and 12 deletions.
2 changes: 1 addition & 1 deletion app/browser/electronDownloadItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const progressDownloadItems = () => {
}

module.exports.updateElectronDownloadItem = (win, downloadId, item, state) => {
if (state === downloadStates.INTERRUPTED || state === downloadStates.CANCELLED || state === downloadStates.COMPLETED) {
if (state === downloadStates.INTERRUPTED || state === downloadStates.CANCELLED || state === downloadStates.UNAUTHORIZED || state === downloadStates.COMPLETED) {
if (app.dock && state === downloadStates.COMPLETED) {
app.dock.downloadFinished(item.getSavePath())
}
Expand Down
2 changes: 1 addition & 1 deletion app/browser/reducers/downloadsReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ const downloadsReducer = (state, action) => {
if (state.get('downloads')) {
const downloads = state.get('downloads')
.filter((download) =>
![downloadStates.COMPLETED, downloadStates.INTERRUPTED, downloadStates.CANCELLED].includes(download.get('state')))
![downloadStates.COMPLETED, downloadStates.INTERRUPTED, downloadStates.UNAUTHORIZED, downloadStates.CANCELLED].includes(download.get('state')))
state = state.set('downloads', downloads)
}
break
Expand Down
3 changes: 2 additions & 1 deletion app/extensions/brave/locales/en-US/downloads.properties
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ downloadDeleteConfirmation=Delete?
downloadInProgress=Downloading: {{downloadPercent}}
downloadInProgressUnknownTotal=Downloading…
downloadInterrupted=Interrupted
downloadUnauthorized=Failed - Unauthorized access to download location
downloadLocalFile=Local file
downloadOpenPath.title=Open Folder Path
downloadPause.title=Pause Download
Expand All @@ -16,4 +17,4 @@ downloadRemoveFromList.title=Remove Download From List
downloadResume.title=Resume Download
downloads=Downloads
downloadViewAll=View All
noDownloads=There are no downloads
noDownloads=There are no downloads
9 changes: 9 additions & 0 deletions app/filtering.js
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,15 @@ function registerForDownloadListener (session) {
updateElectronDownloadItem(downloadId, item, downloadStates.CANCELLED)
appActions.mergeDownloadDetail(downloadId)
})

// Change state if download path is protected
const fs = require('fs')
fs.access(path.dirname(item.getSavePath()), fs.constants.R_OK | fs.constants.W_OK, (err) => {
if (err) {
const state = downloadStates.UNAUTHORIZED
updateDownloadState(win, downloadId, item, state)
}
})
})

item.on('done', function (e, state) {
Expand Down
8 changes: 6 additions & 2 deletions app/renderer/components/download/downloadItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ class DownloadItem extends React.Component {
return this.props.downloadState === downloadStates.INTERRUPTED
}

get isUnauthorized () {
return this.props.downloadState === downloadStates.UNAUTHORIZED
}

get isInProgress () {
return this.props.downloadState === downloadStates.IN_PROGRESS
}
Expand Down Expand Up @@ -132,7 +136,7 @@ class DownloadItem extends React.Component {
width: this.props.percentageComplete
}

if (this.isCancelled || this.isInterrupted) {
if (this.isCancelled || this.isInterrupted || this.isUnauthorized) {
progressStyle.display = 'none'
} else if (this.props.isPendingState) {
l10nStateArgs.downloadPercent = this.props.percentageComplete
Expand Down Expand Up @@ -269,7 +273,7 @@ class DownloadItem extends React.Component {
: null
}
{
this.isCancelled || this.isInterrupted || this.isCompleted || this.isPaused || this.isInProgress
this.isCancelled || this.isInterrupted || this.isUnauthorized || this.isCompleted || this.isPaused || this.isInProgress
? <div className='downloadState' data-l10n-id={this.props.statel10n} data-l10n-args={JSON.stringify(l10nStateArgs)} />
: null
}
Expand Down
3 changes: 2 additions & 1 deletion js/constants/downloadStates.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ const downloadStates = {
PAUSED: _,
COMPLETED: _,
CANCELLED: _,
INTERRUPTED: _
INTERRUPTED: _,
UNAUTHORIZED: _
}

module.exports = mapValuesByKeys(downloadStates)
4 changes: 3 additions & 1 deletion js/state/downloadUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const downloadStates = require('../constants/downloadStates')
const domUtil = require('../../app/renderer/lib/domUtil')

const pendingStates = [downloadStates.IN_PROGRESS, downloadStates.PAUSED]
const stopStates = [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED]
const stopStates = [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.UNAUTHORIZED, downloadStates.COMPLETED]
const notErrorStates = [downloadStates.IN_PROGRESS, downloadStates.PAUSED, downloadStates.COMPLETED]

const downloadIsInState = (download, list) =>
Expand Down Expand Up @@ -42,6 +42,8 @@ const getL10nId = (download) => {
return 'downloadInterrupted'
case downloadStates.CANCELLED:
return 'downloadCancelled'
case downloadStates.UNAUTHORIZED:
return 'downloadUnauthorized'
case downloadStates.IN_PROGRESS:
if (!download.get('totalBytes')) {
return 'downloadInProgressUnknownTotal'
Expand Down
2 changes: 1 addition & 1 deletion less/downloadBar.less
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
}
}

&.completed, &.interrupted, &.cancelled {
&.completed, &.interrupted, &.cancelled, &.unauthorized {
background-color: #e6e6e6;
.downloadState {
font-weight: bold;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ describe('downloadItem component', function () {
appActions.downloadActionPerformed.restore()
})

testButton('[data-test-id="redownloadButton"]', [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED], function (button) {
testButton('[data-test-id="redownloadButton"]', [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.UNAUTHORIZED, downloadStates.COMPLETED], function (button) {
const spy = sinon.spy(appActions, 'downloadRedownloaded')
button.simulate('click')
assert(spy.withArgs(downloadId).calledOnce)
Expand All @@ -160,7 +160,7 @@ describe('downloadItem component', function () {
appActions.downloadRevealed.restore()
})

testButton('[data-test-id="deleteButton"]', [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED], function (button) {
testButton('[data-test-id="deleteButton"]', [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.UNAUTHORIZED, downloadStates.COMPLETED], function (button) {
const spy = sinon.spy(appActions, 'showDownloadDeleteConfirmation')
try {
// Confirmation should NOT be visible by default
Expand All @@ -175,15 +175,15 @@ describe('downloadItem component', function () {
})
})

if ([downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED].includes(state)) {
if ([downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.UNAUTHORIZED, downloadStates.COMPLETED].includes(state)) {
describe(`${state} download item when delete button has been clicked`, function () {
before(function () {
downloadId = uuid.v4()
appStore.state = appStateDownload(state, downloadId, true)
result = mount(<DownloadItem downloadId={downloadId} />)
})

testButton('[data-test-id="confirmDeleteButton"]', [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED], function (button) {
testButton('[data-test-id="confirmDeleteButton"]', [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.UNAUTHORIZED, downloadStates.COMPLETED], function (button) {
const spy = sinon.spy(appActions, 'downloadDeleted')
try {
// Accepting confirmation should delete the item
Expand Down
4 changes: 4 additions & 0 deletions test/unit/app/sessionStoreTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,10 @@ describe('sessionStore unit tests', function () {
result = sessionStore.cleanAppData(data, false)
assert.equal(result.getIn(['downloads', 'entry1', 'state']), downloadStates.CANCELLED)

data = getEntry(downloadStates.UNAUTHORIZED)
result = sessionStore.cleanAppData(data, false)
assert.equal(result.getIn(['downloads', 'entry1', 'state']), downloadStates.UNAUTHORIZED)

data = getEntry(downloadStates.PENDING)
result = sessionStore.cleanAppData(data, false)
assert.equal(result.getIn(['downloads', 'entry1', 'state']), downloadStates.PENDING)
Expand Down

0 comments on commit 9be39e7

Please sign in to comment.