Fix: Cleanup baseViewer.hasAnnotationPermissions() and tests #685
Conversation
|
Verified that @pramodsum has signed the CLA. Thanks for the pull request! |
src/lib/viewers/BaseViewer.js
Outdated
| const { can_annotate, can_view_annotations_all, can_view_annotations_self } = permissions; | ||
|
|
||
| // eslint-disable-next-line | ||
| return can_annotate || can_view_annotations_all || can_view_annotations_self; |
There was a problem hiding this comment.
You were forcing a boolean before with !! on line 814, is ok to return undefined now if no permissions are passed in the object?
There was a problem hiding this comment.
Forcing it again now just to be sure :)
There was a problem hiding this comment.
Seems like this test file is inconsistent with other files in terms of the matchers. http://chaijs.com/api/bdd/
|
This change lgtm, but I'm curious why This patch changes it to return true if you can either leave annotations, view your own, or view all annotations. I'd like some investigation into what this was doing before before we merge this. |
| @@ -382,8 +383,8 @@ describe('lib/viewers/BaseViewer', () => { | |||
| sinon.assert.failException; | |||
| }) | |||
| .catch((error) => { | |||
| expect(error).equals(sinon.match.error); | |||
| expect(error.message).equals('message'); | |||
| expect(error).to.equal(sinon.match.error); | |||
There was a problem hiding this comment.
Can you just change this line real quick to use chai? expect(error).to.be.an('error');
|
@tonyjin looks like this was always returning true because we were passing in the file object rather than the expected permissions object. There are a lot more permissions checks within the annotations codebase which is why this may method may not have actually surfaced a bug but yea the method was never working correctly 👎 |
No description provided.