code fixes and review

CODE_REVIEW.md

@@ -0,0 +1,40 @@
+Problems/ Code smells:- all fixed
+
+- Unsubscribe not done in `book.search.component.ts`
+- Unused methods `ngOnInit` in `total-count.component.ts`
+- Empty `total-count.component.scss` file can be removed
+- `alt` attribute is not added for `img` tags
+- Test cases are failing
+- Descriptions are not matching with the specs provided
+
+Improvements:
+
+- Loader experience on search - fixed
+- Error handling - fixed
+- Safely accessing books collection properties with `?` in html templates to avoid app crashes due to `cannot read property of undefined` - fixed
+- Using declarative pattern for datastreams with `async` pipe in templates when possible.- fixed
+- changeDetection: `OnPush` should be used where ever applicable
+- `Validating query string` - fixed
+  - query string encoding (if query string has special characters, few of the API calls may not be processed) encodeURIComponent
+  - `Validating whitespaces` for search action (unnecessary api calls can be avoided, if we trim the search key)
+- Keeping API URLs and other constant values in a separate `constants` file.
+- `Type checking` is missing for method aurguments
+- Full material design - fixed (partially)
+- Responsive behaviour
+- Semantic HTML tags can be used.
+- Imports can be segregated properly (example: angular, third party, NgRx & app imports etc.)
+- Custom result can be retured in catch block when api fails, for better user experience. Example return an empty observable array - of([]) / of('') etc. and show info message accordingly according to result count. [books.service.ts]
+
+Accessibility:
+
+scanner: - fixed
+
+- couple of color contrast issues in header and book.search.component
+
+manual:- fixed
+
+- `JavaScript` anchor tag is not accessible using tab navigation, as it does not have `href` attribute, to make it accessible we need to add `tabindex`
+- `Want to read` does not have `aria-label` with respect to each book.
+- `aria-labels` are not added for buttons and images
+- `aria-hidden="true"` added for icon tags to hide content from screen readers as they have seperate button tags to access
+- `aria-disabled="true"` for disabled buttons

apps/okreads/browser/src/app/app.component.scss

@@ -14,7 +14,7 @@ h1 {
   position: sticky;
   top: 0;
   z-index: 100;
-  background-color: $pink-accent;
+  background-color: $pink-dark;
   color: $white;
   padding: $spacing-3xs $spacing-xs;
 
@@ -26,7 +26,7 @@ h1 {
 .header--content {
   display: flex;
 
-  > * {
+  >* {
     display: flex;
     align-items: center;
   }

libs/books/data-access/src/lib/+state/books.effects.spec.ts

@@ -1,12 +1,15 @@
 import { TestBed } from '@angular/core/testing';
+import { HttpTestingController } from '@angular/common/http/testing';
+
+import { ReplaySubject } from 'rxjs';
+
 import { provideMockActions } from '@ngrx/effects/testing';
 import { provideMockStore } from '@ngrx/store/testing';
-import { ReplaySubject } from 'rxjs';
+
 import { createBook, SharedTestingModule } from '@tmo/shared/testing';
 
 import { BooksEffects } from './books.effects';
 import * as BooksActions from './books.actions';
-import { HttpTestingController } from '@angular/common/http/testing';
 
 describe('BooksEffects', () => {
   let actions: ReplaySubject<any>;
@@ -41,5 +44,21 @@ describe('BooksEffects', () => {
 
       httpMock.expectOne('/api/books/search?q=').flush([createBook('A')]);
     });
+
+    it('should throw error', done => {
+      const error = new ErrorEvent('error');
+      actions = new ReplaySubject();
+      actions.next(BooksActions.searchBooks({ term: '#' }));
+
+      effects.searchBooks$
+      .subscribe(action => {
+        expect(action.type).toEqual(
+          BooksActions.searchBooksFailure(error).type
+        );
+        done();
+      });
+
+      httpMock.expectOne('/api/books/search?q=#').error(error);
+    });
   });
 });

libs/books/data-access/src/lib/+state/books.effects.ts

@@ -1,8 +1,11 @@
 import { Injectable } from '@angular/core';
 import { HttpClient } from '@angular/common/http';
+
 import { of } from 'rxjs';
 import { catchError, map, switchMap } from 'rxjs/operators';
+
 import { Actions, createEffect, ofType } from '@ngrx/effects';
+
 import { Book } from '@tmo/shared/models';
 import * as BooksActions from './books.actions';
 

libs/books/data-access/src/lib/+state/books.reducer.spec.ts

@@ -1,25 +1,52 @@
 import { initialState, reducer, State } from './books.reducer';
 import * as BooksActions from './books.actions';
+
 import { createBook } from '@tmo/shared/testing';
+import { Book } from '@tmo/shared/models';
 
 describe('Books Reducer', () => {
   describe('valid Books actions', () => {
     it('loadBooksSuccess should return set the list of known Books', () => {
-      const books = [createBook('A'), createBook('B'), createBook('C')];
+      const books: Book[] = [createBook('A'), createBook('B'), createBook('C')];
       const action = BooksActions.searchBooksSuccess({ books });
 
       const result: State = reducer(initialState, action);
 
       expect(result.loaded).toBe(true);
       expect(result.ids.length).toBe(3);
     });
+
+    it('should return state when searchBooks action dispatched', () => {
+      const action = BooksActions.searchBooks({ term: 'JavaScript' });
+
+      const result: State = reducer(initialState, action);
+
+      expect(result.loaded).toBe(false);
+      expect(result.searchTerm).toBe('JavaScript');
+    });
+
+    it('should show error message when searchBooksFailure action dispatched', () => {
+      const action = BooksActions.searchBooksFailure({ error: 'Missing serach term' });
+
+      const result: State = reducer(initialState, action);
+
+      expect(result.error).toBe('Missing serach term');
+    });
+
+    it('should return the initial state when clearSearch action dispatched', () => {
+      const action = BooksActions.clearSearch();
+
+      const result: State = reducer(initialState, action);
+
+      expect(result.ids.length).toBe(0);
+    });
   });
 
   describe('unknown action', () => {
     it('should return the previous state', () => {
       const action = {} as any;
 
-      const result = reducer(initialState, action);
+      const result: State = reducer(initialState, action);
 
       expect(result).toBe(initialState);
     });

libs/books/data-access/src/lib/+state/books.reducer.ts

@@ -1,14 +1,18 @@
+import {
+  createEntityAdapter,
+  EntityAdapter,
+  EntityState
+} from '@ngrx/entity';
 import { Action, createReducer, on } from '@ngrx/store';
-import { createEntityAdapter, EntityAdapter, EntityState } from '@ngrx/entity';
-import { Book } from '@tmo/shared/models';
 
+import { Book } from '@tmo/shared/models';
 import * as BooksActions from './books.actions';
 
 export const BOOKS_FEATURE_KEY = 'books';
 
 export interface State extends EntityState<Book> {
   loaded: boolean;
-  error?: string | null;
+  error?: any | null;
   searchTerm?: string;
 }
 
@@ -19,7 +23,7 @@ export interface BooksPartialState {
 export const booksAdapter: EntityAdapter<Book> = createEntityAdapter<Book>();
 
 export const initialState: State = booksAdapter.getInitialState({
-  loaded: false
+  loaded: true
 });
 
 const booksReducer = createReducer(
@@ -36,11 +40,16 @@ const booksReducer = createReducer(
       loaded: true
     })
   ),
-  on(BooksActions.searchBooksFailure, (state, { error }) => ({
+  on(BooksActions.searchBooksFailure, (state, { error }) => booksAdapter.removeAll({
     ...state,
-    error
+    error,
+    loaded: true
   })),
-  on(BooksActions.clearSearch, state => booksAdapter.removeAll(state))
+  on(BooksActions.clearSearch, state => booksAdapter.removeAll({
+    ...state,
+    error: null,
+    loaded: true
+  }))
 );
 
 export function reducer(state: State | undefined, action: Action) {

libs/books/data-access/src/lib/+state/books.selectors.spec.ts

@@ -1,5 +1,6 @@
 import { booksAdapter, initialState } from './books.reducer';
 import * as BooksSelectors from './books.selectors';
+
 import { createBook } from '@tmo/shared/testing';
 
 describe('Books Selectors', () => {
@@ -23,7 +24,7 @@ describe('Books Selectors', () => {
       const results = BooksSelectors.getBooks(state);
 
       expect(results.length).toBe(3);
-      expect(results.map(x => x.id)).toEqual(['A', 'B', 'C']);
+      expect(results.map(book => book.id)).toEqual(['A', 'B', 'C']);
     });
 
     it("getBooksLoaded() should return the current 'loaded' status", () => {

libs/books/data-access/src/lib/+state/reading-list.effects.spec.ts

@@ -1,12 +1,19 @@
 import { TestBed } from '@angular/core/testing';
+import { HttpTestingController } from '@angular/common/http/testing';
+
 import { ReplaySubject } from 'rxjs';
+
 import { provideMockActions } from '@ngrx/effects/testing';
 import { provideMockStore } from '@ngrx/store/testing';
-import { HttpTestingController } from '@angular/common/http/testing';
 
-import { SharedTestingModule } from '@tmo/shared/testing';
+import {
+  createBook,
+  createReadingListItem,
+  SharedTestingModule
+} from '@tmo/shared/testing';
 import { ReadingListEffects } from './reading-list.effects';
 import * as ReadingListActions from './reading-list.actions';
+import { Book, ReadingListItem } from '@tmo/shared/models';
 
 describe('ToReadEffects', () => {
   let actions: ReplaySubject<any>;
@@ -28,6 +35,11 @@ describe('ToReadEffects', () => {
   });
 
   describe('loadReadingList$', () => {
+    it('should call ngrxOnInitEffects', () => {
+      effects.ngrxOnInitEffects();
+      expect(effects.ngrxOnInitEffects().type).toEqual('[Reading List] Initialize');
+    });
+
     it('should work', done => {
       actions = new ReplaySubject();
       actions.next(ReadingListActions.init());
@@ -41,5 +53,94 @@ describe('ToReadEffects', () => {
 
       httpMock.expectOne('/api/reading-list').flush([]);
     });
+
+    it('should dispatch loadReadingListError when api throws an error', done => {
+      const error = new ErrorEvent('error');
+      actions = new ReplaySubject();
+      actions.next(ReadingListActions.init());
+
+      const readingListAction = ReadingListActions.loadReadingListError(error);
+      effects.loadReadingList$
+      .subscribe(action => {
+        expect(action.type).toEqual(readingListAction.type);
+        done();
+      });
+
+      httpMock.expectOne('/api/reading-list').error(error);
+    });
+  });
+
+  describe('addBook$', () => {
+    it('should add a book to reading list', done => {
+      const book: Book = createBook('A');
+      actions = new ReplaySubject();
+      actions.next(ReadingListActions.addToReadingList({ book: book }));
+
+      effects.addBook$
+      .subscribe(action => {
+        expect(action).toEqual(
+          ReadingListActions.confirmedAddToReadingList({ book })
+        );
+        done();
+      });
+
+      httpMock.expectOne({ url: '/api/reading-list', method: 'post' }).flush([]);
+    });
+
+    it('should dispatch failedAddToReadingList when api throws an error', done => {
+      const book: Book = createBook('A');
+      actions = new ReplaySubject();
+      actions.next(ReadingListActions.addToReadingList({ book: book }));
+
+      effects.addBook$
+      .subscribe(action => {
+        expect(action).toEqual(
+          ReadingListActions.failedAddToReadingList({ book })
+        );
+        done();
+      });
+
+      httpMock.expectOne({ url: '/api/reading-list', method: 'post' }).error(null);
+    });
+  });
+
+  describe('removeBook$', () => {
+    it('should remove book from reading list', done => {
+      const item: ReadingListItem = createReadingListItem('A');
+      actions = new ReplaySubject();
+      actions.next(ReadingListActions.removeFromReadingList({ item }));
+
+      effects.removeBook$
+      .subscribe(action => {
+        expect(action).toEqual(
+          ReadingListActions.confirmedRemoveFromReadingList({ item })
+        );
+        done();
+      });
+
+      httpMock.expectOne({
+        url: `/api/reading-list/${item.bookId}`,
+        method: 'delete'
+      }).flush([]);
+    });
+
+    it('should dispatch failedRemoveFromReadingList when api throws an error', done => {
+      const item: ReadingListItem = createReadingListItem('A');
+      actions = new ReplaySubject();
+      actions.next(ReadingListActions.removeFromReadingList({ item }));
+
+      effects.removeBook$
+      .subscribe(action => {
+        expect(action).toEqual(
+          ReadingListActions.failedRemoveFromReadingList({ item })
+        );
+        done();
+      });
+
+      httpMock.expectOne({
+        url: `/api/reading-list/${item.bookId}`,
+        method: 'delete'
+      }).error(null);
+    });
   });
 });