diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md new file mode 100644 index 0000000..4da2d23 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/apps/okreads/browser/src/app/app.component.scss b/apps/okreads/browser/src/app/app.component.scss index cb6f025..57966fd 100644 --- a/apps/okreads/browser/src/app/app.component.scss +++ b/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; } diff --git a/libs/books/data-access/src/lib/+state/books.effects.spec.ts b/libs/books/data-access/src/lib/+state/books.effects.spec.ts index 8ad6979..bc5d1cb 100644 --- a/libs/books/data-access/src/lib/+state/books.effects.spec.ts +++ b/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; @@ -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); + }); }); }); diff --git a/libs/books/data-access/src/lib/+state/books.effects.ts b/libs/books/data-access/src/lib/+state/books.effects.ts index a4ab643..3617911 100644 --- a/libs/books/data-access/src/lib/+state/books.effects.ts +++ b/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'; diff --git a/libs/books/data-access/src/lib/+state/books.reducer.spec.ts b/libs/books/data-access/src/lib/+state/books.reducer.spec.ts index 92f3eb7..8d3f910 100644 --- a/libs/books/data-access/src/lib/+state/books.reducer.spec.ts +++ b/libs/books/data-access/src/lib/+state/books.reducer.spec.ts @@ -1,11 +1,13 @@ 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); @@ -13,13 +15,38 @@ describe('Books Reducer', () => { 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); }); diff --git a/libs/books/data-access/src/lib/+state/books.reducer.ts b/libs/books/data-access/src/lib/+state/books.reducer.ts index ee6cb9f..95c7923 100644 --- a/libs/books/data-access/src/lib/+state/books.reducer.ts +++ b/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 { loaded: boolean; - error?: string | null; + error?: any | null; searchTerm?: string; } @@ -19,7 +23,7 @@ export interface BooksPartialState { export const booksAdapter: EntityAdapter = createEntityAdapter(); 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) { diff --git a/libs/books/data-access/src/lib/+state/books.selectors.spec.ts b/libs/books/data-access/src/lib/+state/books.selectors.spec.ts index 79f1b0e..86c9a98 100644 --- a/libs/books/data-access/src/lib/+state/books.selectors.spec.ts +++ b/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", () => { diff --git a/libs/books/data-access/src/lib/+state/reading-list.effects.spec.ts b/libs/books/data-access/src/lib/+state/reading-list.effects.spec.ts index 466f896..0c11d30 100644 --- a/libs/books/data-access/src/lib/+state/reading-list.effects.spec.ts +++ b/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; @@ -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); + }); }); }); diff --git a/libs/books/data-access/src/lib/+state/reading-list.effects.ts b/libs/books/data-access/src/lib/+state/reading-list.effects.ts index 129a0e4..b965da7 100644 --- a/libs/books/data-access/src/lib/+state/reading-list.effects.ts +++ b/libs/books/data-access/src/lib/+state/reading-list.effects.ts @@ -1,8 +1,11 @@ import { Injectable } from '@angular/core'; import { HttpClient } from '@angular/common/http'; -import { Actions, createEffect, ofType, OnInitEffects } from '@ngrx/effects'; + import { of } from 'rxjs'; import { catchError, concatMap, exhaustMap, map } from 'rxjs/operators'; + +import { Actions, createEffect, ofType, OnInitEffects } from '@ngrx/effects'; + import { ReadingListItem } from '@tmo/shared/models'; import * as ReadingListActions from './reading-list.actions'; @@ -13,12 +16,8 @@ export class ReadingListEffects implements OnInitEffects { ofType(ReadingListActions.init), exhaustMap(() => this.http.get('/api/reading-list').pipe( - map((data) => - ReadingListActions.loadReadingListSuccess({ list: data }) - ), - catchError((error) => - of(ReadingListActions.loadReadingListError({ error })) - ) + map(data => ReadingListActions.loadReadingListSuccess({ list: data })), + catchError(error => of(ReadingListActions.loadReadingListError({ error }))) ) ) ) @@ -43,12 +42,8 @@ export class ReadingListEffects implements OnInitEffects { ofType(ReadingListActions.removeFromReadingList), concatMap(({ item }) => this.http.delete(`/api/reading-list/${item.bookId}`).pipe( - map(() => - ReadingListActions.confirmedRemoveFromReadingList({ item }) - ), - catchError(() => - of(ReadingListActions.failedRemoveFromReadingList({ item })) - ) + map(() => ReadingListActions.confirmedRemoveFromReadingList({ item })), + catchError(() => of(ReadingListActions.failedRemoveFromReadingList({ item }))) ) ) ) @@ -58,5 +53,5 @@ export class ReadingListEffects implements OnInitEffects { return ReadingListActions.init(); } - constructor(private actions$: Actions, private http: HttpClient) {} + constructor(private actions$: Actions, private http: HttpClient) { } } diff --git a/libs/books/data-access/src/lib/+state/reading-list.reducer.spec.ts b/libs/books/data-access/src/lib/+state/reading-list.reducer.spec.ts index b83eb1c..f5dea53 100644 --- a/libs/books/data-access/src/lib/+state/reading-list.reducer.spec.ts +++ b/libs/books/data-access/src/lib/+state/reading-list.reducer.spec.ts @@ -6,9 +6,10 @@ import { State } from './reading-list.reducer'; import { createBook, createReadingListItem } from '@tmo/shared/testing'; +import { ReadingListItem } from '@tmo/shared/models'; -describe('Books Reducer', () => { - describe('valid Books actions', () => { +describe('ReadingList Reducer', () => { + describe('valid reading list actions', () => { let state: State; beforeEach(() => { @@ -18,7 +19,7 @@ describe('Books Reducer', () => { ); }); - it('loadBooksSuccess should load books from reading list', () => { + it('should load books from reading list on loadReadingListSuccess', () => { const list = [ createReadingListItem('A'), createReadingListItem('B'), @@ -32,7 +33,35 @@ describe('Books Reducer', () => { expect(result.ids.length).toEqual(3); }); - it('failedAddToReadingList should undo book addition to the state', () => { + it('init should load initial state', () => { + const action = ReadingListActions.init(); + + const result: State = reducer(initialState, action); + + expect(result.loaded).toBe(false); + expect(result.ids.length).toEqual(0); + }); + + it('should add book to reading list on addToReadingList', () => { + const action = ReadingListActions.addToReadingList({ + book: createBook('B') + }); + + const result: State = reducer(state, action); + + expect(result.ids).toEqual(['A', 'B']); + }); + + it('should undo book addition to the state on removeFromReadingList', () => { + const item: ReadingListItem = createReadingListItem('B'); + const action = ReadingListActions.removeFromReadingList({ item }); + + const result: State = reducer(state, action); + + expect(result.ids).toEqual(['A']); + }); + + it('should undo book addition to the state on failedAddToReadingList', () => { const action = ReadingListActions.failedAddToReadingList({ book: createBook('B') }); @@ -42,7 +71,7 @@ describe('Books Reducer', () => { expect(result.ids).toEqual(['A']); }); - it('failedRemoveFromReadingList should undo book removal from the state', () => { + it('should undo book removal from the state on failedRemoveFromReadingList', () => { const action = ReadingListActions.failedRemoveFromReadingList({ item: createReadingListItem('C') }); @@ -51,6 +80,16 @@ describe('Books Reducer', () => { expect(result.ids).toEqual(['A', 'B', 'C']); }); + + it('should display error on loadReadingListError', () => { + const action = ReadingListActions.loadReadingListError({ + error: 'Internal server error' + }); + + const result: State = reducer(state, action); + + expect(result.error).toEqual('Internal server error'); + }); }); describe('unknown action', () => { diff --git a/libs/books/data-access/src/lib/+state/reading-list.reducer.ts b/libs/books/data-access/src/lib/+state/reading-list.reducer.ts index cb0b5f8..d75d02a 100644 --- a/libs/books/data-access/src/lib/+state/reading-list.reducer.ts +++ b/libs/books/data-access/src/lib/+state/reading-list.reducer.ts @@ -52,6 +52,15 @@ const readingListReducer = createReducer( ), on(ReadingListActions.removeFromReadingList, (state, action) => readingListAdapter.removeOne(action.item.bookId, state) + ), + on(ReadingListActions.failedAddToReadingList, (state, action) => + readingListAdapter.removeOne(action.book.id, state) + ), + on(ReadingListActions.failedRemoveFromReadingList, (state, action) => + readingListAdapter.addOne( + { bookId: action.item.bookId, ...action.item }, + state + ) ) ); diff --git a/libs/books/data-access/src/lib/+state/reading-list.selectors.spec.ts b/libs/books/data-access/src/lib/+state/reading-list.selectors.spec.ts index c8b9fd2..f54f193 100644 --- a/libs/books/data-access/src/lib/+state/reading-list.selectors.spec.ts +++ b/libs/books/data-access/src/lib/+state/reading-list.selectors.spec.ts @@ -35,17 +35,22 @@ describe('ReadingList Selectors', () => { }); describe('Books Selectors', () => { - it('getReadingList() should return the list of Books', () => { + it('should return the list of Books on getReadingList', () => { const results = ToReadSelectors.getReadingList(state); expect(results.length).toBe(3); - expect(results.map(x => x.bookId)).toEqual(['A', 'B', 'C']); + expect(results.map(book => book.bookId)).toEqual(['A', 'B', 'C']); }); - it("getTotalUnread() should return the current 'loaded' status", () => { + it("should return the current 'loaded' status on getTotalUnread", () => { const result = ToReadSelectors.getTotalUnread(state); expect(result).toBe(3); }); + + it("should return the current 'loaded' status on getTotalUnread", () => { + const result = ToReadSelectors.getAllBooks(state); + expect(result.length).toBe(3); + }); }); }); diff --git a/libs/books/data-access/src/lib/+state/reading-list.selectors.ts b/libs/books/data-access/src/lib/+state/reading-list.selectors.ts index 8db6fe4..e6c933d 100644 --- a/libs/books/data-access/src/lib/+state/reading-list.selectors.ts +++ b/libs/books/data-access/src/lib/+state/reading-list.selectors.ts @@ -35,7 +35,7 @@ export const getAllBooks = createSelector< Record, ReadingListBook[] >(getBooks, getReadingListEntities, (books, entities) => { - return books.map(b => ({ ...b, isAdded: Boolean(entities[b.id]) })); + return books.map(book => ({ ...book, isAdded: Boolean(entities[book.id]) })); }); export const getReadingList = createSelector(getReadingListState, selectAll); diff --git a/libs/books/data-access/src/lib/books-data-access.module.ts b/libs/books/data-access/src/lib/books-data-access.module.ts index 714f61a..ba4182d 100644 --- a/libs/books/data-access/src/lib/books-data-access.module.ts +++ b/libs/books/data-access/src/lib/books-data-access.module.ts @@ -1,7 +1,9 @@ import { NgModule } from '@angular/core'; import { CommonModule } from '@angular/common'; + import { StoreModule } from '@ngrx/store'; import { EffectsModule } from '@ngrx/effects'; + import * as fromBooks from './+state/books.reducer'; import { BooksEffects } from './+state/books.effects'; import * as fromReadingList from './+state/reading-list.reducer'; diff --git a/libs/books/feature/src/lib/book-search/book-search.component.html b/libs/books/feature/src/lib/book-search/book-search.component.html index 18ba309..631ddda 100644 --- a/libs/books/feature/src/lib/book-search/book-search.component.html +++ b/libs/books/feature/src/lib/book-search/book-search.component.html @@ -1,56 +1,94 @@
-
-
-
-
- {{ b.title }} -
-
-
- -
-