Improve error when flagging when logged out #374
Conversation
Codecov Report
@@ Coverage Diff @@
## master #374 +/- ##
=========================================
+ Coverage 89.39% 89.4% +<.01%
=========================================
Files 121 121
Lines 4773 4776 +3
Branches 814 815 +1
=========================================
+ Hits 4267 4270 +3
Misses 506 506
Continue to review full report at Codecov.
|
|
This doesn't add a clickable login link or button yet. |
I've found the solution to this: |
Improve the error message that's shown to the user when trying to flag an annotation while logged out. Instead of showing a "404 Not Found. Either the resource you requested doesn't exist, or you are not currently authorized to see it." error from the server, show a friendlier "You must be logged in to report an annotation" message. This is done client-side by checking whether the user is logged in when they click the flag button, and if not showing an error instead of sending the flag request to the API. This is because the API doesn't respond with a unique "You must be logged in to flag" error that the client could depend on, it just returns a 404, which could be for a number of reasons (e.g. the annotation no longer exists).
55f2cfb to
cd22ae6
Compare
|
Done |
| parts.controller.flag(); | ||
| assert.calledWith(fakeAnnotationMapper.flagAnnotation, | ||
| parts.annotation); | ||
| done(); |
There was a problem hiding this comment.
The done callback here is unnecessary since this test is synchronous.
| var controller = createDirective().controller; | ||
| var err = new Error('500 Server error'); | ||
| fakeAnnotationMapper.flagAnnotation.returns(Promise.reject(err)); | ||
| controller.flag(); |
There was a problem hiding this comment.
I note that this was an issue with the existing code, but the setTimeout here could have been avoided if flag return a Promise which resolved once the flagging request had completed.
| ); | ||
| context('when the user is not logged in', function() { | ||
| beforeEach(function() { | ||
| delete fakeSession.state.userid; |
There was a problem hiding this comment.
Strictly speaking, the userid will be null rather than undefined if the user is not logged in, which is what happens when deleting a property.
|
LGTM. One minor note: I would prefer that in future we stick with native promises where possible and try to avoid mixing |
Improve the error message that's shown to the user when trying to flag
an annotation while logged out.
Instead of showing a "404 Not Found. Either the resource you requested
doesn't exist, or you are not currently authorized to see it." error
from the server, show a friendlier "You must be logged in to report an
annotation" message:
This is done client-side by checking whether the user is logged in when
they click the flag button, and if not showing an error instead of
sending the flag request to the API. This is because the API doesn't
respond with a unique "You must be logged in to flag" error that the
client could depend on, it just returns a 404, which could be for a
number of reasons (e.g. the annotation no longer exists).
I am using the(Fixed, see below)preventOpenDuplicates: trueoption to angular-toastr sothat clicking on the flag button multiple times doesn't create multiple
error messages on screen at once, but this isn't working - multiple
error messages still appear. Not sure why.