Commit 2fef71a3 authored by Robert Knight's avatar Robert Knight

Fix `groups.focused()` error if localStorage is empty when client loads

If the client started and local storage did not contain a saved
last-viewed group ID, then `groups.load()` would attempt to call
`store.focusGroup(null)`. As a result subsequent calls to
`groups.focused()` would return null and code in various places in the
application expects that there will always be a focused group.

Fix the problem by not setting the focus group after groups are fetched
unless the previously-focused group was set and exists in the new
groups list.

Also improve the behavior of `store.focusGroup` by making it leave the
focused group untouched if the specified group has not been loaded, but
log an error so we'll know (via Sentry) if it happens again.

Fixes #750
parent b14859ec
......@@ -62,12 +62,12 @@ function groups($rootScope, store, api, isSidebar, localStorage, serviceUrl, ses
params.document_uri = uri;
}
return api.groups.list(params);
}).then(gs => {
}).then(groups => {
var isFirstLoad = store.allGroups().length === 0;
var prevFocusedGroup = localStorage.getItem(STORAGE_KEY);
store.loadGroups(gs);
if (isFirstLoad) {
store.loadGroups(groups);
if (isFirstLoad && groups.some(g => g.id === prevFocusedGroup)) {
store.focusGroup(prevFocusedGroup);
}
......
......@@ -132,6 +132,16 @@ describe('groups', function() {
});
});
[null, 'some-group-id'].forEach(groupId => {
it('does not set the focused group if not present in the groups list', () => {
var svc = service();
fakeLocalStorage.getItem.returns(groupId);
return svc.load().then(() => {
assert.notCalled(fakeStore.focusGroup);
});
});
});
context('in the sidebar', () => {
it('waits for the document URL to be determined', () => {
var svc = service();
......
......@@ -21,7 +21,11 @@ function init() {
const update = {
FOCUS_GROUP(state, action) {
const group = state.groups.find(g => g.id === action.id);
return { focusedGroupId: group ? action.id : null };
if (!group) {
console.error(`Attempted to focus group ${action.id} which is not loaded`);
return {};
}
return { focusedGroupId: action.id };
},
LOAD_GROUPS(state, action) {
......
......@@ -21,16 +21,30 @@ describe('sidebar.store.modules.groups', () => {
});
describe('focusGroup', () => {
beforeEach(() => {
sinon.stub(console, 'error');
});
afterEach(() => {
console.error.restore();
});
it('updates the focused group if valid', () => {
store.loadGroups([publicGroup]);
store.focusGroup(publicGroup.id);
assert.equal(store.getState().focusedGroupId, publicGroup.id);
assert.notCalled(console.error);
});
it('does not set the focused group if invalid', () => {
it('does not update focused group if not valid', () => {
store.loadGroups([publicGroup]);
store.focusGroup(privateGroup.id);
assert.equal(store.getState().focusedGroupId, null);
assert.equal(store.getState().focusedGroupId, publicGroup.id);
assert.called(console.error);
});
});
......
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