Commit 930ff477 authored by Robert Knight's avatar Robert Knight

Fix error when creating annotation if groups are not loaded

Fix an old crash [1] when attempting to create an annotation if groups have not
yet loaded. In this case there is no focused group for the new annotation to be
associated with.

The handling for now is to just ignore the request and delete the highlight in
the guest, similar to how we handle the case where the user is not logged in,
except we don't show the login panel.

[1] https://hypothesis.sentry.io/issues/4412536210/
parent 54cae019
...@@ -388,16 +388,22 @@ export class FrameSyncService { ...@@ -388,16 +388,22 @@ export class FrameSyncService {
// A new annotation, note or highlight was created in the frame // A new annotation, note or highlight was created in the frame
guestRPC.on('createAnnotation', (annot: AnnotationData) => { guestRPC.on('createAnnotation', (annot: AnnotationData) => {
// If user is not logged in, we can't really create a meaningful highlight // If user is not logged in, or groups haven't loaded yet, we can't create
// or annotation. Instead, we need to open the sidebar, show an error, // a meaningful highlight or annotation. Instead, we need to open the
// and delete the (unsaved) annotation so it gets un-selected in the // sidebar, show an error, and delete the (unsaved) annotation so it gets
// target document // un-selected in the target document
if (!this._store.isLoggedIn()) { const isLoggedIn = this._store.isLoggedIn();
const hasGroup = this._store.focusedGroup() !== null;
if (!isLoggedIn || !hasGroup) {
this._hostRPC.call('openSidebar'); this._hostRPC.call('openSidebar');
if (!isLoggedIn) {
this._store.openSidebarPanel('loginPrompt'); this._store.openSidebarPanel('loginPrompt');
}
this._guestRPC.forEach(rpc => rpc.call('deleteAnnotation', annot.$tag)); this._guestRPC.forEach(rpc => rpc.call('deleteAnnotation', annot.$tag));
return; return;
} }
this._inFrame.add(annot.$tag); this._inFrame.add(annot.$tag);
// Open the sidebar so that the user can immediately edit the draft // Open the sidebar so that the user can immediately edit the draft
......
...@@ -149,6 +149,7 @@ describe('FrameSyncService', () => { ...@@ -149,6 +149,7 @@ describe('FrameSyncService', () => {
}, },
findIDsForTags: sinon.stub().returns([]), findIDsForTags: sinon.stub().returns([]),
focusedGroup: sinon.stub().returns({ id: 'foobar' }),
hoverAnnotations: sinon.stub(), hoverAnnotations: sinon.stub(),
isLoggedIn: sinon.stub().returns(false), isLoggedIn: sinon.stub().returns(false),
openSidebarPanel: sinon.stub(), openSidebarPanel: sinon.stub(),
...@@ -583,7 +584,7 @@ describe('FrameSyncService', () => { ...@@ -583,7 +584,7 @@ describe('FrameSyncService', () => {
assert.calledWith(hostRPC().call, 'showHighlights'); assert.calledWith(hostRPC().call, 'showHighlights');
}); });
context('when an authenticated user is present', () => { context('user is logged in and there is a focused group', () => {
beforeEach(async () => { beforeEach(async () => {
frameSync.connect(); frameSync.connect();
await connectGuest(); await connectGuest();
...@@ -619,13 +620,7 @@ describe('FrameSyncService', () => { ...@@ -619,13 +620,7 @@ describe('FrameSyncService', () => {
}); });
}); });
context('when no authenticated user is present', () => { const addCommonNotReadyTests = () => {
beforeEach(async () => {
fakeStore.isLoggedIn.returns(false);
frameSync.connect();
await connectGuest();
});
it('should not create an annotation in the sidebar', () => { it('should not create an annotation in the sidebar', () => {
emitGuestEvent('createAnnotation', { $tag: 't1', target: [] }); emitGuestEvent('createAnnotation', { $tag: 't1', target: [] });
...@@ -638,17 +633,38 @@ describe('FrameSyncService', () => { ...@@ -638,17 +633,38 @@ describe('FrameSyncService', () => {
assert.calledWith(hostRPC().call, 'openSidebar'); assert.calledWith(hostRPC().call, 'openSidebar');
}); });
it('should open the login prompt panel', () => { it('should send a "deleteAnnotation" message to the frame', () => {
emitGuestEvent('createAnnotation', { $tag: 't1', target: [] }); emitGuestEvent('createAnnotation', { $tag: 't1', target: [] });
assert.calledWith(fakeStore.openSidebarPanel, 'loginPrompt'); assert.calledWith(guestRPC().call, 'deleteAnnotation');
}); });
};
it('should send a "deleteAnnotation" message to the frame', () => { context('user is not logged in', () => {
beforeEach(async () => {
fakeStore.isLoggedIn.returns(false);
frameSync.connect();
await connectGuest();
});
addCommonNotReadyTests();
it('should open the login prompt panel', () => {
emitGuestEvent('createAnnotation', { $tag: 't1', target: [] }); emitGuestEvent('createAnnotation', { $tag: 't1', target: [] });
assert.calledWith(guestRPC().call, 'deleteAnnotation'); assert.calledWith(fakeStore.openSidebarPanel, 'loginPrompt');
});
}); });
context('groups have not loaded', () => {
beforeEach(async () => {
fakeStore.isLoggedIn.returns(true);
fakeStore.focusedGroup.returns(null);
frameSync.connect();
await connectGuest();
});
addCommonNotReadyTests();
}); });
}); });
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment