-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix frequently used emojis list doesn't get updated when adding emojis by typing emoji code with colon #18396
Changes from 1 commit
9448c83
4f94c44
b0eeb72
f1184b8
a9e689e
9b780a5
3490d46
1efc145
a414ef0
c277e87
9521a0f
4c8af67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,12 @@ | ||
import _ from 'underscore'; | ||
import moment from 'moment'; | ||
import Onyx from 'react-native-onyx'; | ||
import Emoji from '../../assets/emojis'; | ||
import * as EmojiUtils from '../../src/libs/EmojiUtils'; | ||
import ONYXKEYS from '../../src/ONYXKEYS'; | ||
import * as User from '../../src/libs/actions/User'; | ||
import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; | ||
import * as TestHelper from '../utils/TestHelper'; | ||
|
||
describe('EmojiTest', () => { | ||
it('matches all the emojis in the list', () => { | ||
|
@@ -147,4 +153,219 @@ describe('EmojiTest', () => { | |
], | ||
}]); | ||
}); | ||
|
||
describe('update frequently used emojis', () => { | ||
let spy; | ||
|
||
beforeAll(() => { | ||
Onyx.init({keys: ONYXKEYS}); | ||
global.fetch = TestHelper.getGlobalFetchMock(); | ||
spy = jest.spyOn(User, 'updateFrequentlyUsedEmojis'); | ||
}); | ||
|
||
beforeEach(() => { | ||
spy.mockClear(); | ||
Onyx.clear(); | ||
return waitForPromisesToResolve(); | ||
}); | ||
|
||
it('should put a less frequent and recent used emoji behind', () => { | ||
const frequentlyEmojisList = [ | ||
{ | ||
code: '👋', name: 'wave', count: 2, lastUpdatedAt: 4, | ||
}, | ||
{ | ||
code: '💤', name: 'zzz', count: 2, lastUpdatedAt: 3, | ||
}, | ||
{ | ||
code: '💯', name: '100', count: 2, lastUpdatedAt: 2, | ||
}, | ||
{ | ||
code: '👿', name: 'imp', count: 2, lastUpdatedAt: 1, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lol, today I learnt, this emoji is named as imp 😄 |
||
}, | ||
]; | ||
Onyx.merge(ONYXKEYS.FREQUENTLY_USED_EMOJIS, frequentlyEmojisList); | ||
|
||
return waitForPromisesToResolve() | ||
.then(() => { | ||
const currentTime = moment().unix(); | ||
const smileEmoji = {code: '😄', name: 'smile'}; | ||
const newEmoji = [smileEmoji]; | ||
EmojiUtils.addToFrequentlyUsedEmojis(newEmoji); | ||
|
||
const expectedSmileEmoj = {...smileEmoji, count: 1, lastUpdatedAt: currentTime}; | ||
const expectedFrequentlyEmojisList = [...frequentlyEmojisList, expectedSmileEmoj]; | ||
expect(spy).toBeCalledWith(expectedFrequentlyEmojisList); | ||
}); | ||
}); | ||
|
||
it('should put more frequent and recent used emoji to the front', () => { | ||
const smileEmoji = {code: '😄', name: 'smile'}; | ||
const frequentlyEmojisList = [ | ||
{ | ||
code: '😠', name: 'angry', count: 3, lastUpdatedAt: 5, | ||
}, | ||
{ | ||
code: '👋', name: 'wave', count: 2, lastUpdatedAt: 4, | ||
}, | ||
{ | ||
code: '💤', name: 'zzz', count: 2, lastUpdatedAt: 3, | ||
}, | ||
{ | ||
code: '💯', name: '100', count: 1, lastUpdatedAt: 2, | ||
}, | ||
{...smileEmoji, count: 1, lastUpdatedAt: 1}, | ||
]; | ||
Onyx.merge(ONYXKEYS.FREQUENTLY_USED_EMOJIS, frequentlyEmojisList); | ||
|
||
return waitForPromisesToResolve() | ||
.then(() => { | ||
const currentTime = moment().unix(); | ||
const newEmoji = [smileEmoji]; | ||
EmojiUtils.addToFrequentlyUsedEmojis(newEmoji); | ||
|
||
const expectedFrequentlyEmojisList = [ | ||
frequentlyEmojisList[0], | ||
{...smileEmoji, count: 2, lastUpdatedAt: currentTime}, | ||
...frequentlyEmojisList.slice(1, -1), | ||
]; | ||
expect(spy).toBeCalledWith(expectedFrequentlyEmojisList); | ||
}); | ||
}); | ||
|
||
it('should sorted descending by count and lastUpdatedAt for multiple emoji added', () => { | ||
const smileEmoji = {code: '😄', name: 'smile'}; | ||
const zzzEmoji = {code: '💤', name: 'zzz'}; | ||
const impEmoji = {code: '👿', name: 'imp'}; | ||
const frequentlyEmojisList = [ | ||
{ | ||
code: '😠', name: 'angry', count: 3, lastUpdatedAt: 5, | ||
}, | ||
{ | ||
code: '👋', name: 'wave', count: 2, lastUpdatedAt: 4, | ||
}, | ||
{...zzzEmoji, count: 2, lastUpdatedAt: 3}, | ||
{ | ||
code: '💯', name: '100', count: 1, lastUpdatedAt: 2, | ||
}, | ||
{...smileEmoji, count: 1, lastUpdatedAt: 1}, | ||
]; | ||
Onyx.merge(ONYXKEYS.FREQUENTLY_USED_EMOJIS, frequentlyEmojisList); | ||
|
||
return waitForPromisesToResolve() | ||
.then(() => { | ||
const currentTime = moment().unix(); | ||
const newEmoji = [smileEmoji, zzzEmoji, impEmoji]; | ||
EmojiUtils.addToFrequentlyUsedEmojis(newEmoji); | ||
|
||
const expectedFrequentlyEmojisList = [ | ||
{...zzzEmoji, count: 3, lastUpdatedAt: currentTime}, | ||
frequentlyEmojisList[0], | ||
{...smileEmoji, count: 2, lastUpdatedAt: currentTime}, | ||
frequentlyEmojisList[1], | ||
{...impEmoji, count: 1, lastUpdatedAt: currentTime}, | ||
frequentlyEmojisList[3], | ||
]; | ||
expect(spy).toBeCalledWith(expectedFrequentlyEmojisList); | ||
}); | ||
}); | ||
|
||
it('if the list is full, should replaced n least used emoji from the list with the n new emoji', () => { | ||
const smileEmoji = {code: '😄', name: 'smile'}; | ||
const zzzEmoji = {code: '💤', name: 'zzz'}; | ||
const impEmoji = {code: '👿', name: 'imp'}; | ||
const bookEmoji = {code: '📚', name: 'books'}; | ||
|
||
// Given the existing full (24 items) frequently used emojis list | ||
const frequentlyEmojisList = [ | ||
{ | ||
code: '😠', name: 'angry', count: 3, lastUpdatedAt: 24, | ||
}, | ||
{ | ||
code: '👋', name: 'wave', count: 3, lastUpdatedAt: 23, | ||
}, | ||
{ | ||
code: '😡', name: 'rage', count: 3, lastUpdatedAt: 22, | ||
}, | ||
{ | ||
code: '😤', name: 'triumph', count: 3, lastUpdatedAt: 21, | ||
}, | ||
{ | ||
code: '🥱', name: 'yawning_face', count: 3, lastUpdatedAt: 20, | ||
}, | ||
{ | ||
code: '😫', name: 'tired_face', count: 3, lastUpdatedAt: 19, | ||
}, | ||
{ | ||
code: '😩', name: 'weary', count: 3, lastUpdatedAt: 18, | ||
}, | ||
{ | ||
code: '😓', name: 'sweat', count: 3, lastUpdatedAt: 17, | ||
}, | ||
{ | ||
code: '😞', name: 'disappointed', count: 3, lastUpdatedAt: 16, | ||
}, | ||
{ | ||
code: '😣', name: 'persevere', count: 3, lastUpdatedAt: 15, | ||
}, | ||
{ | ||
code: '😖', name: 'confounded', count: 3, lastUpdatedAt: 14, | ||
}, | ||
{ | ||
code: '👶', name: 'baby', count: 3, lastUpdatedAt: 13, | ||
}, | ||
{ | ||
code: '👄', name: 'lips', count: 3, lastUpdatedAt: 12, | ||
}, | ||
{ | ||
code: '🐶', name: 'dog', count: 3, lastUpdatedAt: 11, | ||
}, | ||
{ | ||
code: '🦮', name: 'guide_dog', count: 3, lastUpdatedAt: 10, | ||
}, | ||
{ | ||
code: '🐱', name: 'cat', count: 3, lastUpdatedAt: 9, | ||
}, | ||
{ | ||
code: '🐈⬛', name: 'black_cat', count: 3, lastUpdatedAt: 8, | ||
}, | ||
{ | ||
code: '🕞', name: 'clock330', count: 3, lastUpdatedAt: 7, | ||
}, | ||
{ | ||
code: '🥎', name: 'softball', count: 3, lastUpdatedAt: 6, | ||
}, | ||
{ | ||
code: '🏀', name: 'basketball', count: 3, lastUpdatedAt: 5, | ||
}, | ||
{ | ||
code: '📟', name: 'pager', count: 3, lastUpdatedAt: 4, | ||
}, | ||
{ | ||
code: '🎬', name: 'clapper', count: 3, lastUpdatedAt: 3, | ||
}, | ||
{ | ||
code: '📺', name: 'tv', count: 3, lastUpdatedAt: 2, | ||
}, | ||
{...bookEmoji, count: 3, lastUpdatedAt: 1}, | ||
]; | ||
Onyx.merge(ONYXKEYS.FREQUENTLY_USED_EMOJIS, frequentlyEmojisList); | ||
|
||
return waitForPromisesToResolve() | ||
.then(() => { | ||
// When add n (3) new emojis | ||
const currentTime = moment().unix(); | ||
const newEmoji = [bookEmoji, smileEmoji, zzzEmoji, impEmoji]; | ||
EmojiUtils.addToFrequentlyUsedEmojis(newEmoji); | ||
|
||
// Then n least used emojis from the list should be replaced | ||
const expectedFrequentlyEmojisList = [ | ||
{...bookEmoji, count: 4, lastUpdatedAt: currentTime}, | ||
...frequentlyEmojisList.slice(0, (-1 - (newEmoji.length - 1))), | ||
..._.map(newEmoji.slice(1), e => ({...e, count: 1, lastUpdatedAt: currentTime})), | ||
]; | ||
expect(spy).toBeCalledWith(expectedFrequentlyEmojisList); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should expect that FrequentlyEmojisList length doesn’t exceed the max allowed value expectedFrequentlyEmojisList.length === (CONST.EMOJI_FREQUENT_ROW_COUNT * CONST.EMOJI_NUM_PER_ROW) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not totally agree here, the length should not pass the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I got your point, but honestly I found it's weird to test an expected result. So, I add the length check to the |
||
}); | ||
}); | ||
}); | ||
}); |
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.
let's try to mimic real data as much as possible by using unix timestamp in
lastUpdatedAt
?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.
Personally, I think it's fine to have a simple number instead of unix timestamp as it's easier to read. The purpose of
lastUpdatedAt
is to show which emoji is used the most recent and I think as long as the number can represent that clearly, then it's fine.