-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
datahub: Favorite star bug on page change #533
Conversation
Affected libs:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work thanks.
I have made some comments to make the logic clearer about what we are doing.
Also, I don't think that you properly test the new behavior.
You should add a couple of tests where the modifiedFavorites$
ticks
- contains the component record
- does not contain the component record
libs/feature/search/src/lib/favorites/favorite-star/favorite-star.component.ts
Outdated
Show resolved
Hide resolved
libs/feature/search/src/lib/favorites/favorite-star/favorite-star.component.ts
Show resolved
Hide resolved
} | ||
}) | ||
editedDs = [] | ||
}) | ||
} | ||
|
||
ngOnDestroy(): void { | ||
this.subscription.unsubscribe() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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() ......
libs/feature/search/src/lib/favorites/favorite-star/favorite-star.component.ts
Outdated
Show resolved
Hide resolved
libs/feature/search/src/lib/favorites/favorite-star/favorite-star.component.ts
Outdated
Show resolved
Hide resolved
libs/feature/search/src/lib/favorites/favorite-star/favorite-star.component.ts
Outdated
Show resolved
Hide resolved
…tar.component.ts refactor pairwise function Co-authored-by: Florent Gravin <[email protected]>
libs/feature/search/src/lib/favorites/favorite-star/favorite-star.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works well like that thanks.
The test should be modified though, to play the new scenario, and test all the possibilities.
Thanks !
libs/feature/search/src/lib/favorites/favorite-star/favorite-star.component.ts
Outdated
Show resolved
Hide resolved
libs/feature/search/src/lib/favorites/favorite-star/favorite-star.component.spec.ts
Outdated
Show resolved
Hide resolved
…tar.component.ts refactor pairwise logic Co-authored-by: Florent Gravin <[email protected]>
libs/feature/search/src/lib/favorites/favorite-star/favorite-star.component.spec.ts
Outdated
Show resolved
Hide resolved
) | ||
}) | ||
it('adds the record to myFavoritesUuid$', async () => { | ||
await expect(favoritesService.myFavoritesUuid$['_value'][0]).toBe( |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 happensmyFavoriteUuid$.next(['a'])
=> the component is updated
There was a problem hiding this comment.
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.
]) | ||
expect(favoritesService.removeFromFavorites).not.toHaveBeenCalled() | ||
}) | ||
it('increase record favorite count by one', () => { |
There was a problem hiding this comment.
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
@cmoinier all the tests were there, thanks ! |
Thanks for the rework @fgravin :) Looks good to me, merging now. |
Thanks Tobias and Florent for the help!
JIRA ISSUE
https://jira.camptocamp.com/browse/GSHDF-679
DESCRIPTION