fix: closing does not not mark current notices as read

src/app/actions/notices.ts

@@ -1,6 +1,6 @@
 import { StatefulNotice } from 'app/lmem/notice';
 import Tab from 'app/lmem/tab';
-import { BaseAction, ErrorAction, TabAction, TabMeta } from '.';
+import { ActionMeta, BaseAction, TabAction } from '.';
 
 export interface NoticesFoundAction extends TabAction {
   type: 'NOTICES_FOUND';
@@ -105,44 +105,23 @@ export interface UnfoldNoticeAction extends BaseAction {
 
 export const unfoldNotice = (id: number): UnfoldNoticeAction => ({
   type: 'UNFOLD_NOTICE',
-  payload: id,
-  meta: { sendToBackground: true }
+  payload: id
 });
 
 export interface MarkNoticeReadAction extends BaseAction {
   type: 'MARK_NOTICE_READ';
   payload: number;
 }
 
-export const markNoticeRead = (id: number): MarkNoticeReadAction => ({
+export const markNoticeRead = (
+  id: number,
+  meta?: ActionMeta
+): MarkNoticeReadAction => ({
   type: 'MARK_NOTICE_READ',
-  payload: id
-});
-
-export interface NoticesUpdatedAction extends TabAction {
-  type: 'NOTICES_UPDATED';
-  payload: StatefulNotice[];
-}
-
-export const noticesUpdated = (
-  payload: StatefulNotice[],
-  meta: TabMeta
-): NoticesUpdatedAction => ({
-  type: 'NOTICES_UPDATED',
-  payload,
+  payload: id,
   meta
 });
 
-export interface UpdateNoticeFailedAction extends ErrorAction {
-  type: 'UPDATE_NOTICE_FAILED';
-}
-
-export const updateNoticesFailed = (e: Error): UpdateNoticeFailedAction => ({
-  type: 'UPDATE_NOTICE_FAILED',
-  payload: e,
-  error: true
-});
-
 export interface ResourceLinkClickedAction extends BaseAction {
   type: 'NOTICE/RESOURCE_LINK_CLICKED';
   payload: number;

src/app/actions/ui.ts

@@ -27,8 +27,7 @@ export interface CloseAction extends BaseAction {
 }
 export const close = (cause: CloseCause): CloseAction => ({
   type: 'CLOSE',
-  payload: { cause },
-  meta: { sendToBackground: true }
+  payload: { cause }
 });
 
 export interface CloseFailedAction extends ErrorAction {
@@ -46,8 +45,7 @@ export interface ClosedAction extends BaseAction {
 }
 export const closed = (cause: CloseCause): ClosedAction => ({
   type: 'CLOSED',
-  payload: { cause },
-  meta: { sendToBackground: true }
+  payload: { cause }
 });
 
 export const TOGGLE_UI = 'TOGGLE_UI';

test/app/reducers.ts → src/app/background/reducers/prefs.reducer.spec.ts

@@ -1,23 +1,16 @@
-import chai from 'chai';
+import { expect } from 'chai';
 import prefsReducer from 'app/background/reducers/prefs.reducer';
-import resourcesReducer from 'app/background/reducers/resources.reducer';
-import { receivedMatchingContexts } from 'app/actions/refreshMatchingContexts';
 import {
   dismissNotice,
   likeNotice,
   unlikeNotice,
   dislikeNotice,
   undislikeNotice,
   undismissNotice,
-  unfoldNotice
+  markNoticeRead
 } from 'app/actions/notices';
-import { MatchingContext } from 'app/lmem/matchingContext';
-
-const expect = chai.expect;
 
 describe('prefsReducer reducer', function() {
-  const installationDetails = { version: '0.1', reason: 'install' };
-
   it('dismiss notice', () => {
     const action = dismissNotice(1);
 
@@ -121,7 +114,7 @@ describe('prefsReducer reducer', function() {
   });
 
   it('read notice', () => {
-    const action = unfoldNotice(42);
+    const action = markNoticeRead(42);
 
     const nextState = prefsReducer(
       {

src/app/background/reducers/prefs.reducer.ts

@@ -66,10 +66,10 @@ function prefsReducer(state: PrefsState = initialPrefs, action: AppAction) {
       }
       return state;
 
-    case 'UNFOLD_NOTICE':
+    case 'MARK_NOTICE_READ':
       return {
         ...state,
-        readNotices: R.uniq(R.concat(state.readNotices, [action.payload]))
+        readNotices: R.concat(state.readNotices, [action.payload])
       };
 
     case 'TOS_ACCEPTED':

src/app/background/reducers/subscriptions.reducer.spec.ts

@@ -1,16 +1,17 @@
-/* eslint-disable no-unused-expressions */
+/* eslint-disable no-unused-expressions, @typescript-eslint/ban-ts-ignore */
 import { expect } from 'chai';
-import { CloseCause } from 'app/lmem/ui';
-import { closed, subscribe, SubscribeAction } from 'app/actions';
+import { subscribe, SubscribeAction } from 'app/actions';
 import { generateContributor } from 'test/fakers/generateContributor';
 import subscriptionsReducer, {
   SubscriptionsState
 } from './subscriptions.reducer';
 
+const unknownAction = { type: 'UNKNOWN' };
+
 describe('background > reducers > subscriptions', function() {
   it('is empty initially', () => {
-    expect(subscriptionsReducer(undefined, closed(CloseCause.CloseButton))).to
-      .be.empty;
+    // @ts-ignore
+    expect(subscriptionsReducer(undefined, unknownAction)).to.be.empty;
   });
   it('saves subscriptions', () => {
     const state: SubscriptionsState = [1, 2, 3];

src/app/background/sagas/badge.ts

@@ -1,20 +1,49 @@
 import { SagaIterator } from 'redux-saga';
 import { takeLatest, call, select } from 'redux-saga/effects';
-import { NoticesUpdatedAction } from '../../actions/notices';
-import { BadgeTheme, updateBadge, resetBadge } from '../../lmem/badge';
-import { TabAction } from '../../actions';
+import { Notice } from 'app/lmem/notice';
+import { BadgeTheme, updateBadge, resetBadge } from 'app/lmem/badge';
+import {
+  FeedbackOnNoticeAction,
+  MarkNoticeReadAction,
+  NoticesFoundAction,
+  badgeResetFailed,
+  badgeUpdateFailed,
+  AppAction,
+  TabAction
+} from 'app/actions';
+import { ReceivedAction } from 'webext/createMessageHandler';
 import { getNoticesToDisplay } from '../selectors/prefs';
-import { badgeResetFailed, badgeUpdateFailed } from '../../actions/badge';
 
+const noticeFromId = (id: number) => ({ id } as Notice);
+
+type BadgeImpactingAction = (
+  | MarkNoticeReadAction
+  | NoticesFoundAction
+  | FeedbackOnNoticeAction) &
+  ReceivedAction;
 export const updateBadgeSaga = (badgeTheme: BadgeTheme) =>
-  function*({
-    payload: notices,
-    meta: { tab }
-  }: NoticesUpdatedAction): SagaIterator {
+  function*(action: BadgeImpactingAction): SagaIterator {
     try {
+      const notices =
+        action.type === 'NOTICES_FOUND'
+          ? (action as NoticesFoundAction).payload.notices
+          : [
+              noticeFromId(
+                action.type === 'MARK_NOTICE_READ'
+                  ? (action as MarkNoticeReadAction).payload
+                  : (action as FeedbackOnNoticeAction).payload.id
+              )
+            ];
+
+      /* FIXME (JAR): I don't like this because we don't really have real notices in `notices`
+         but we may have { id: number } objects.
+         It works because we only filter based on id, and then count them,
+         but this is fragile and Im' not hayppy with it.
+         I think we should maintain a list of actives notices for each tab. */
+
       const noticesToDisplay = yield select(getNoticesToDisplay(notices));
 
-      yield call(updateBadge, noticesToDisplay, badgeTheme, tab.id);
+      yield call(updateBadge, noticesToDisplay, badgeTheme, action.meta.tab.id);
     } catch (e) {
       badgeUpdateFailed(e);
     }
@@ -28,11 +57,17 @@ export function* resetBadgeSaga({ meta: { tab } }: TabAction): SagaIterator {
   }
 }
 
+const isTabChangedAction = (action: AppAction): boolean =>
+  action.type === 'BROWSER/TAB_CREATED' ||
+  action.type === 'BROWSER/TAB_UPDATED';
+
+const isActionImpactingBadge = (action: AppAction): boolean =>
+  action.type === 'MARK_NOTICE_READ' ||
+  action.type === 'NOTICES_FOUND' ||
+  action.type === 'FEEDBACK_ON_NOTICE';
+
 export default (badgeTheme: BadgeTheme) =>
   function* tabRootSaga() {
-    yield takeLatest('NOTICES_UPDATED', updateBadgeSaga(badgeTheme));
-    yield takeLatest(
-      ['BROWSER/TAB_CREATED', 'BROWSER/TAB_UPDATED'],
-      resetBadgeSaga
-    );
+    yield takeLatest(isActionImpactingBadge, updateBadgeSaga(badgeTheme));
+    yield takeLatest(isTabChangedAction, resetBadgeSaga);
   };

src/app/background/sagas/tab.ts

@@ -2,11 +2,7 @@ import { put, takeEvery, select, call, all, take } from 'redux-saga/effects';
 import * as R from 'ramda';
 import { isToday } from 'date-fns';
 import { TAB_CREATED, TAB_UPDATED } from 'app/constants/browser/tabs';
-import {
-  CONTEXT_TRIGGERED,
-  MATCH_CONTEXT,
-  NOTICES_FOUND
-} from 'app/constants/ActionTypes';
+import { CONTEXT_TRIGGERED, MATCH_CONTEXT } from 'app/constants/ActionTypes';
 import {
   init,
   contextTriggered,
@@ -18,11 +14,8 @@ import {
   contextNotTriggered,
   noNoticesDisplayed,
   noticesFound,
-  noticesUpdated,
   MatchContextAction,
   ContextTriggeredAction,
-  NoticesFoundAction,
-  BaseAction,
   TabAction,
   TabCreatedAction,
   TabUpdatedAction,
@@ -134,7 +127,7 @@ const waitForTabReadySaga = (tab: Tab) =>
     );
   };
 
-export function* publishToTabSaga(action: TabAction) {
+export function* sendToTabSaga(action: TabAction) {
   const tab = action.meta.tab;
   const tabs: { [id: string]: Tab } = yield select(getTabs);
   if (!tabs[tab.id]) return;
@@ -144,12 +137,8 @@ export function* publishToTabSaga(action: TabAction) {
   sendToTab(tab.id, action);
 }
 
-export function* updateNoticesSaga({
-  payload: { notices },
-  meta: { tab }
-}: NoticesFoundAction) {
-  yield put(noticesUpdated(notices, { tab }));
-}
+const shouldActionBeSentToTab = (action: AppAction) =>
+  Boolean(action.meta && action.meta.sendToTab);
 
 export default function* tabRootSaga() {
   const contentCode: string = yield call(
@@ -161,10 +150,5 @@ export default function* tabRootSaga() {
   yield takeEvery([TAB_CREATED, TAB_UPDATED], tabSaga(executeTabContentScript));
   yield takeEvery(MATCH_CONTEXT, matchContextSaga);
   yield takeEvery(CONTEXT_TRIGGERED, contextTriggeredSaga);
-  yield takeEvery(
-    (action: BaseAction) => Boolean(action.meta && action.meta.sendToTab),
-    publishToTabSaga
-  );
-
-  yield takeEvery(NOTICES_FOUND, updateNoticesSaga);
+  yield takeEvery(shouldActionBeSentToTab, sendToTabSaga);
 }

src/app/content/sagas/notices.ts

@@ -1,51 +1,48 @@
 import { all, put, select, takeLatest, takeEvery } from 'redux-saga/effects';
-import { getNotices, getTab, hasNoticesToDisplay } from '../selectors';
-import {
-  noticesUpdated,
-  updateNoticesFailed,
-  markNoticeRead,
-  UnfoldNoticeAction
-} from 'app/actions/notices';
+import { getNotices, hasNoticesToDisplay } from '../selectors';
+import { markNoticeRead, UnfoldNoticeAction } from 'app/actions/notices';
 import { close } from 'app/actions/ui';
 import { CLOSED } from 'app/constants/ActionTypes';
 import { StatefulNotice } from 'app/lmem/notice';
 import { CloseCause } from '../../lmem/ui';
-import { AppAction } from '../../actions';
+import { AppAction, createErrorAction } from '../../actions';
 
-export function* updateNoticesSaga() {
+export function* closeIfNoMoreNoticeToDisplaySaga() {
   try {
-    const notices = yield select(getNotices);
-    const tab = yield select(getTab);
-    yield put(noticesUpdated(notices, { tab, sendToBackground: true }));
-
     const hasNotices = yield select(hasNoticesToDisplay);
     if (!hasNotices) {
       yield put(close(CloseCause.NoMoreNotice));
     }
   } catch (e) {
-    yield put(updateNoticesFailed(e));
+    yield put(createErrorAction()(e));
   }
 }
 
 function* markNoticesReadSaga() {
   const notices = yield select(getNotices);
-  yield all(notices.map(({ id }: StatefulNotice) => put(markNoticeRead(id))));
+  yield all(
+    notices.map(({ id }: StatefulNotice) =>
+      put(markNoticeRead(id, { sendToBackground: true }))
+    )
+  );
 }
 
 function* markNoticeReadSaga(unfoldNoticeAction: UnfoldNoticeAction) {
-  yield put(markNoticeRead(unfoldNoticeAction.payload));
+  yield put(
+    markNoticeRead(unfoldNoticeAction.payload, { sendToBackground: true })
+  );
 }
 
 export const isClosedByButtonAction = (action: AppAction) =>
   action.type === CLOSED && action.payload.cause === CloseCause.CloseButton;
 
+export const isChangeOnNoticeAction = (action: AppAction) =>
+  action.type === 'MARK_NOTICE_READ' || action.type === 'FEEDBACK_ON_NOTICE';
+
 export default function* noticesRootSaga() {
   yield all([
     // FIXME change all strings to constants because it’s a pain the ass to refactor (i.e. rename)
-    yield takeLatest(
-      ['MARK_NOTICE_READ', 'FEEDBACK_ON_NOTICE'],
-      updateNoticesSaga
-    ),
+    yield takeLatest(isChangeOnNoticeAction, closeIfNoMoreNoticeToDisplaySaga),
     yield takeLatest(isClosedByButtonAction, markNoticesReadSaga),
     yield takeEvery('UNFOLD_NOTICE', markNoticeReadSaga)
   ]);

src/webext/assocTabIfNotGiven.ts

@@ -1,8 +1,15 @@
 import * as R from 'ramda';
 import { Action } from 'redux';
 
+type ActionWithTab<A> = A & { meta: { tab: chrome.tabs.Tab } };
+
 const assocTabIfNotGiven = (tab?: chrome.tabs.Tab) => <A extends Action>(
   action: A
-): A => R.over(R.lensPath(['meta', 'tab']), R.defaultTo(tab), action);
+): ActionWithTab<A> =>
+  R.over(
+    R.lensPath(['meta', 'tab']),
+    R.defaultTo(tab),
+    action
+  ) as ActionWithTab<A>;
 
 export default assocTabIfNotGiven;