Artist insights 2.0 - Some artists with 1 badge don't have insights displayed #2125
Conversation
peril-staging
bot
added
the
Version: Patch
|
🎉 And thanks for including screenshots!! Could you add a little description of what the change was/why you had to make it? It's not totally clear to me! (for reference @ashkan18 has some good PR description formats for this 😄i.e. https://github.com/artsy/vortex/pull/28) |
| if (!hasSections(this.props.artist)) { | ||
| if ( | ||
| !hasSections(this.props.artist) && | ||
| this.props.artist.insights.length === 0 |
There was a problem hiding this comment.
Can we add a test that would have caught this case?
|
Deploy preview for artsy-reaction ready! Built with commit cf3a98d |
| if (!hasSections(this.props.artist)) { | ||
| if ( | ||
| (!hasSections(this.props.artist) && !this.props.artist.insights) || | ||
| this.props.artist.insights.length === 0 |
There was a problem hiding this comment.
Is it possible for hasSections(this.props.artist) to be true but this.props.artist.insights to be null? Because then you might get a cannot read property 'length' of null error here.
There was a problem hiding this comment.
Had my parentheses in the wrong place my bad!
| it("renders selected career achievements if no auction results or partner highlights", async () => { | ||
| ArtistFixture.auctionResults = null | ||
| ArtistFixture.highlights.partners = null | ||
| wrapper = await getWrapper(ArtistFixture) |
There was a problem hiding this comment.
For good data hygiene we should avoid mutating the ArtistFixture in these tests. Although the mutations can't escape outside of the bounds of this file (because of the way jest executes test files in isolation) it could cause confusing problems if people add or modify tests here in the future.
The idiomatic and safe thing to do would be to construct a new version of the artist fixture using object spread syntax, e.g.
wrapper = await getWrapper({
...ArtistFixture,
auctionResults: null,
highlights: {
...ArtistFixture.highlights,
partners: null
}
})
|
🚀 PR was released in v15.2.2 🚀 |
Problem:
Blek le rat has 1 badge and it displays, but Dustin Yellin also has one badge (reviewed by ArtForum) and his Artist Insights isn't displayed.
Solution:
Check to display the SelectedCareerAchievements component included a check on artist.insights.length, but there was a return inside the component's render that didn't include this check and was returning for artists with insights, so I added it there as well.
Before:
After:
