Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

datahub: Favorite star bug on page change #533

Merged
merged 9 commits into from
Jul 28, 2023
Merged

datahub: Favorite star bug on page change #533

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
537c423
(fix): favorite star bug on page change
cmoinier Jul 13, 2023
8cf81bd
(chore): format:write
cmoinier Jul 13, 2023
42b4921
Update libs/feature/search/src/lib/favorites/favorite-star/favorite-s…
cmoinier Jul 17, 2023
d60f240
(fix): refactor after reviews + add tests
cmoinier Jul 17, 2023
8f954ae
Update libs/feature/search/src/lib/favorites/favorite-star/favorite-s…
cmoinier Jul 17, 2023
2a1ec01
(fix): tests
cmoinier Jul 23, 2023
97b99a8
(tests): edit tests
cmoinier Jul 27, 2023
e12d662
test(favorite): work scenario
fgravin Jul 28, 2023
60ed20f
chore(prettier): exclude /docs cache & lint
fgravin Jul 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 65 additions & 29 deletions libs/feature/search/src/lib/favorites/favorite-star/favorite-star.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,54 +115,90 @@ describe('FavoriteStarComponent', () => {
describe('if record is not part of favorite', () => {
beforeEach(() => {
;(favoritesService as any).myFavoritesUuid$.next([
'aaa',
'bbb',
'ccc',
component.record.uuid,
])
starToggle.newValue.emit(true)
fixture.detectChanges()
})
it('adds record to favorites', () => {
expect(favoritesService.addToFavorites).toHaveBeenCalledWith([
component.record.uuid,
])
expect(favoritesService.removeFromFavorites).not.toHaveBeenCalled()
describe('if change is made from this component', () => {
beforeEach(() => {
starToggle.newValue.emit(true)
fixture.detectChanges()
})
it('adds record to favorites', () => {
cmoinier marked this conversation as resolved.
Show resolved Hide resolved
expect(favoritesService.addToFavorites).toHaveBeenCalledWith([
component.record.uuid,
])
expect(favoritesService.removeFromFavorites).not.toHaveBeenCalled()
})
it('increase record favorite count by one', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn I forgot to commit one comment

While this test is a consequence of myFavoritesUuid$ tick.
Both things are decorrelated and should be tested separatly.

When I click on the star => here you test that the functions have been called

  • When the record is already in favorite
  • When the record is not in favorites
    When favorites has been updated => here you test that the amount is correct
  • My record have been updated
  • My record haven't been updated

expect(favoriteCountEl.textContent).toEqual(
(component.record.favoriteCount + 1).toString()
)
})
it('adds the record to myFavoritesUuid$', async () => {
await expect(favoritesService.myFavoritesUuid$['_value'][0]).toBe(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what does this test actually test.
BTW, Observables are a flow and should not be tested as an instantaneous value (which is hacky cause it's not the public API).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It tests if the record is present in the myFavoriteUuid after the user clicks on the star. It's important especially when it ticks from another component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so how do I test if myFavoriteUuid properly recorded that the record is in the user's favorite? I didn't have any other idea

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

component.record.uuid = 'a'

  • myFavoriteUuid$.next(['b']) => nothing happens
  • myFavoriteUuid$.next(['a']) => the component is updated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It tests if the record is present in the myFavoriteUuid after the user clicks on the star. It's important especially when it ticks from another component.

yes but you did the myFavoritesUuid$.next([component.record.uuid]) in the beforeEach, which is exactly what you test in there.

component.record.uuid
)
})
})
it('increase record favorite count by one', () => {
expect(favoriteCountEl.textContent).toEqual(
(component.record.favoriteCount + 1).toString()
)
describe('if change is made from another component', () => {
it('adds the record to myFavoritesUuid$', async () => {
await expect(favoritesService.myFavoritesUuid$['_value'][0]).toBe(
component.record.uuid
)
})
})
})
describe('if record is part of favorite', () => {
beforeEach(() => {
;(favoritesService as any).myFavoritesUuid$.next([
'aaa',
'bbb',
component.record.uuid,
])
starToggle.newValue.emit(false)
fixture.detectChanges()
})
it('removes record from favorites', () => {
expect(favoritesService.removeFromFavorites).toHaveBeenCalledWith([
component.record.uuid,
])
expect(favoritesService.addToFavorites).not.toHaveBeenCalled()
describe('if change is made from this component', () => {
beforeEach(() => {
component.record = {
...RECORDS_SUMMARY_FIXTURE[0],
favoriteCount: 42,
}
;(favoritesService as any).myFavoritesUuid$.next(['aaa'])
starToggle.newValue.emit(false)
fixture.detectChanges()
})
it('removes record from favorites', () => {
expect(favoritesService.removeFromFavorites).toHaveBeenCalledWith([
component.record.uuid,
])
expect(favoritesService.addToFavorites).not.toHaveBeenCalled()
})
it('decrease record favorite count by one', () => {
expect(favoriteCountEl.textContent).toEqual(
(component.record.favoriteCount - 1).toString()
)
})
})
it('decrease record favorite count by one', () => {
expect(favoriteCountEl.textContent).toEqual(
(component.record.favoriteCount - 1).toString()
)
describe('if change is made from another component', () => {
beforeEach(() => {
component.record = {
...RECORDS_SUMMARY_FIXTURE[0],
favoriteCount: 42,
}
;(favoritesService as any).myFavoritesUuid$.next(['aaa'])
fixture.detectChanges()
})
it('removes the record from myFavoritesUuid$', async () => {
await expect(
favoritesService.myFavoritesUuid$['_value'][0]
).not.toBe(component.record.uuid)
})
})
})

describe('two subsequent changes', () => {
beforeEach(() => {
;(favoritesService as any).myFavoritesUuid$.next([
'aaa',
'bbb',
component.record.uuid,
])
;(favoritesService as any).myFavoritesUuid$.next(['aaa'])
starToggle.newValue.emit(false)
starToggle.newValue.emit(true)
fixture.detectChanges()
Expand Down
23 changes: 19 additions & 4 deletions libs/feature/search/src/lib/favorites/favorite-star/favorite-star.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
} from '@angular/core'
import { FavoritesService } from '../favorites.service'
import { MetadataRecord } from '@geonetwork-ui/util/shared'
import { map } from 'rxjs/operators'
import { first, map, pairwise, take } from 'rxjs/operators'
import { AuthService } from '@geonetwork-ui/feature/auth'
import tippy from 'tippy.js'
import { TranslateService } from '@ngx-translate/core'
Expand Down Expand Up @@ -49,6 +49,7 @@ export class FavoriteStarComponent implements AfterViewInit, OnDestroy {
@ViewChild(StarToggleComponent, { read: ElementRef })
starToggleRef: ElementRef
subscription: Subscription
countSubscription: Subscription

get hasFavoriteCount() {
return this.favoriteCount !== null
Expand All @@ -74,10 +75,27 @@ export class FavoriteStarComponent implements AfterViewInit, OnDestroy {
})
}
})
this.countSubscription = this.favoritesService.myFavoritesUuid$
cmoinier marked this conversation as resolved.
Show resolved Hide resolved
.pipe(pairwise())
.subscribe(([oldFavs, newFavs]) => {
const editedFavs = (
oldFavs.length < newFavs.length
? newFavs.slice(-1)
: oldFavs.filter((fav) => !newFavs.includes(fav))
)[0]
if (this.hasFavoriteCount && editedFavs === this.record.uuid) {
if (newFavs.includes(editedFavs)) {
this.favoriteCount += 1
} else {
this.favoriteCount += -1
}
}
})
}

ngOnDestroy(): void {
this.subscription.unsubscribe()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, one subscription is enough, you could use subscription.add() to all subscribes that you want to unsubscribe from.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did try that. For some reason the subscription isn't working (as in, nothing happens, no update on directly clicked record nor on the other page) unless it's in its own subscription. I agree this is not the best way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work, what did you try ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried the normal way : this.subscription.add() on this.favoritesService.myFavoritesUuid$.pipe() ......

this.countSubscription.unsubscribe()
}

toggleFavorite(isFavorite) {
Expand All @@ -87,9 +105,6 @@ export class FavoriteStarComponent implements AfterViewInit, OnDestroy {
: this.favoritesService.removeFromFavorites([this.record.uuid])
).subscribe({
complete: () => {
if (this.hasFavoriteCount) {
this.favoriteCount += isFavorite ? 1 : -1
}
this.loading = false
this.changeDetector.detectChanges()
},
Expand Down