-
Notifications
You must be signed in to change notification settings - Fork 10
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
Adjust 'this person can edit this widget' checks on My Widgets page, locks widget instances in creator. #49
Changes from all commits
1aa3952
b4c982e
f64cd43
68d1427
bb5c3a9
ec95955
4e74db0
6bcf268
b2a4a51
6279a2f
9f8c648
1f9e022
0957c80
4f68e1b
f7f0125
7c9cabd
a89fc0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,20 @@ app.service('widgetSrv', function(selectedWidgetSrv, dateTimeServ, $q, $rootScop | |
return Materia.Coms.Json.send('widgets_get', [[id]]).then(widgets => widgets[0]) | ||
} | ||
|
||
const lockWidget = (id = null) => { | ||
const deferred = $q.defer() | ||
Materia.Coms.Json.send('widget_instance_lock', [id]).then(success => { | ||
if (success) { | ||
deferred.resolve(id) | ||
} else { | ||
deferred.reject( | ||
'This widget is currently locked, you will be able to edit this widget when it is no longer being edited by somebody else.' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This exact string is in ctrl-my-widgets.js. Can we de-duplicate it? |
||
) | ||
} | ||
}) | ||
return deferred.promise | ||
} | ||
|
||
const getWidgetsByType = (type = 'featured') => { | ||
return Materia.Coms.Json.send('widgets_get_by_type', [type]) | ||
} | ||
|
@@ -233,18 +247,29 @@ app.service('widgetSrv', function(selectedWidgetSrv, dateTimeServ, $q, $rootScop | |
} | ||
} | ||
|
||
const canBePublishedByCurrentUser = widget_id => { | ||
const deferred = $q.defer() | ||
Materia.Coms.Json.send('widget_publish_perms_verify', [widget_id]).then(response => { | ||
deferred.resolve(response) | ||
}) | ||
|
||
return deferred.promise | ||
} | ||
|
||
return { | ||
getWidgets, | ||
getWidgetsByType, | ||
getWidget, | ||
getWidgetInfo, | ||
lockWidget, | ||
sortWidgets, | ||
saveWidget, | ||
removeWidget, | ||
updateHashUrl, | ||
selectWidgetFromHashUrl, | ||
convertAvailibilityDates, | ||
copyWidget, | ||
deleteWidget | ||
deleteWidget, | ||
canBePublishedByCurrentUser | ||
} | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,6 +63,7 @@ describe('widgetSrv', () => { | |
expect(_service.getWidgetsByType).toBeDefined() | ||
expect(_service.getWidget).toBeDefined() | ||
expect(_service.getWidgetInfo).toBeDefined() | ||
expect(_service.lockWidget).toBeDefined() | ||
expect(_service.sortWidgets).toBeDefined() | ||
expect(_service.saveWidget).toBeDefined() | ||
expect(_service.removeWidget).toBeDefined() | ||
|
@@ -401,4 +402,49 @@ describe('widgetSrv', () => { | |
$scope.$digest() // processes promise | ||
expect(_selectedWidgetSrv.notifyAccessDenied).toHaveBeenCalled() | ||
}) | ||
|
||
it('rejects with a message when a widget is already locked', () => { | ||
mockSendPromiseOnce(false) | ||
|
||
let promiseSpy = jest.fn() | ||
let promiseCatch = jest.fn() | ||
_service | ||
.lockWidget(1) | ||
.then(promiseSpy) | ||
.catch(promiseCatch) | ||
$scope.$digest() // processes promise | ||
|
||
expect(Materia.Coms.Json.send).toHaveBeenCalledWith('widget_instance_lock', [1]) | ||
expect(promiseSpy).not.toHaveBeenCalled() | ||
expect(promiseCatch).toHaveBeenCalledWith( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting approach I hadn't thought of. I think it's ok as is, however one shouldn't have to test that promiseSpy wasn't called - if promiseCatch ends up getting called promiseSpy will always be skipped. FWIW jest has another way to test catching with |
||
'This widget is currently locked, you will be able to edit this widget when it is no longer being edited by somebody else.' | ||
) | ||
}) | ||
|
||
it('locks a widget', () => { | ||
mockSendPromiseOnce(true) | ||
|
||
let promiseSpy = jest.fn() | ||
let promiseCatch = jest.fn() | ||
_service | ||
.lockWidget(1) | ||
.then(promiseSpy) | ||
.catch(promiseCatch) | ||
$scope.$digest() // processes promise | ||
|
||
expect(Materia.Coms.Json.send).toHaveBeenCalledWith('widget_instance_lock', [1]) | ||
expect(promiseSpy).toHaveBeenCalledWith(1) | ||
expect(promiseCatch).not.toHaveBeenCalled() | ||
}) | ||
|
||
it('checks whether a widget can be published by the current user', () => { | ||
mockSendPromiseOnce(true) | ||
|
||
let promiseSpy = jest.fn() | ||
_service.canBePublishedByCurrentUser(1).then(promiseSpy) | ||
$scope.$digest() // processes promise | ||
|
||
expect(Materia.Coms.Json.send).toHaveBeenCalledWith('widget_publish_perms_verify', [1]) | ||
expect(promiseSpy).toHaveBeenCalledWith(true) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we should try to do lockWidget, getWidget, and checkUserPublishPerms in parallel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure - if the widget is locked then we shouldn't try to get it, and if we can't get it then we shouldn't bother checking whether the user can publish it. That, and
checkUserPublishPerms
relies on the full widget data being passed into it fromwidgetSrv.getWidget
.