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

feat(translation): introduce cache busting logic #4240

Merged
merged 16 commits into from
Oct 27, 2020
Merged

feat(translation): introduce cache busting logic #4240

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
adcbe6f
feat(translation): introduce cache busting logic
IgnacioBecerra Oct 15, 2020
7ec46be
feat(translation): updated cache busting logic
IgnacioBecerra Oct 16, 2020
20563b1
feat(translation): adding tests
IgnacioBecerra Oct 19, 2020
4a69b12
feat(translation): implemented tests for cache busting logic
IgnacioBecerra Oct 19, 2020
8ec32fe
Merge branch 'master' into translation-cache-busting
IgnacioBecerra Oct 19, 2020
7bfe950
feat(translation): date mock testing
IgnacioBecerra Oct 21, 2020
fc025e3
feat(translation): embedded unique ID to response
IgnacioBecerra Oct 22, 2020
ccb566c
feat(translation): resolved conflict and merge
IgnacioBecerra Oct 26, 2020
d18b6ee
feat(translation): changed string
IgnacioBecerra Oct 26, 2020
07c7970
Merge branch 'master' into translation-cache-busting
IgnacioBecerra Oct 26, 2020
3246da0
feat(translation): removed changing id
IgnacioBecerra Oct 26, 2020
0484e64
feat(translation): removed dynamic imports
IgnacioBecerra Oct 27, 2020
84670da
feat(translation): removed unique id from production
IgnacioBecerra Oct 27, 2020
9188268
feat(translation): added cache property
IgnacioBecerra Oct 27, 2020
e3cb32d
Merge branch 'master' into translation-cache-busting
IgnacioBecerra Oct 27, 2020
f8128e7
Merge branch 'master' into translation-cache-busting
kodiakhq[bot] Oct 27, 2020
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
12 changes: 7 additions & 5 deletions packages/services/src/services/Translation/Translation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,11 @@ class TranslationAPI {
.then(response => this.transformData(response.data))
.then(data => {
data['timestamp'] = new Date().getTime();
IgnacioBecerra marked this conversation as resolved.
Show resolved Hide resolved
sessionStorage.setItem(
`${_sessionTranslationKey}-${country}-${lang}`,
JSON.stringify(data)
);
(data['id'] = 'TRANSLATION_FRESH'),
sessionStorage.setItem(
`${_sessionTranslationKey}-${country}-${lang}`,
JSON.stringify(data)
);
return data;
});
}
Expand Down Expand Up @@ -188,14 +189,15 @@ class TranslationAPI {
return;
}

const currentTime = new Date().getTime(),
const currentTime = Date.now(),
timeDiff = currentTime - session.timestamp;

if (timeDiff > _twoHours) {
sessionStorage.removeItem(key);
return;
}

session['id'] = 'TRANSLATION_OLD';
return session;
}
}
Expand Down
44 changes: 41 additions & 3 deletions packages/services/src/services/Translation/__tests__/Translation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ describe('TranslationAPI', () => {

const elseResponse = await TranslationAPI.getTranslation({});

// done since current test only focuses on body
delete responseSuccess.id;
delete response.id;
delete elseResponse.id;

expect(elseResponse).toEqual(responseSuccess);

expect(mockAxios.get).toHaveBeenCalledWith(fetchUrl, {
Expand All @@ -104,18 +109,25 @@ describe('TranslationAPI', () => {
});

it('should return a json with a recent timestamp', async () => {
const mockDate = new Date('2019');
const _Date = Date;
global.Date = jest.fn(() => mockDate);
global.Date.now = _Date.now;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you try: https://jestjs.io/docs/en/snapshot-testing#2-tests-should-be-deterministic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using that and many other variations before using this approach, but unfortunately this one was the only one that actually had any effect on response. Not sure what is causing it though, the implementations don't differ much from another.

IgnacioBecerra marked this conversation as resolved.
Show resolved Hide resolved

// using very old cached session
sessionStorageMock.setItem(
'dds-translation-us-en',
JSON.stringify(oldSession)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
JSON.stringify(oldSession)
JSON.stringify(Object.assign(oldSession, { id: uniqueId }))

);

const previousSession = JSON.parse(
sessionStorageMock.getItem('dds-translation-us-en')
);

expect(previousSession.id).toEqual('TRANSLATION_OLD');

// reinitializing import
const TranslationAPI = (await import('../Translation')).default;
Copy link
Contributor

Choose a reason for hiding this comment

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

I won't block proceeding this PR with my comment here, but I'd rather see resetting things done via mocks. The behavior of dynamic import tends to be dependent on build toolchain and we may have high chance to get an undefined behavior if it's used as a mean for "resetting".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see, I'll be applying the same logic for Locale API, so I'll use mocks there and also add it here. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Any update here...? Thanks!


const response = await TranslationAPI.getTranslation({
lc: 'en',
cc: 'us',
Expand All @@ -131,12 +143,38 @@ describe('TranslationAPI', () => {
// should contain timestamp
expect(response).toHaveProperty('timestamp');

// should not equal old timestamp
expect(previousSession.timestamp).not.toEqual(response.timestamp);
// should equal mock timestamp
expect(response.timestamp).toEqual(mockDate.valueOf());
IgnacioBecerra marked this conversation as resolved.
Show resolved Hide resolved

// id should have changed
expect(response.id).not.toEqual(previousSession.id);
expect(response.id).toEqual('TRANSLATION_FRESH');

// timestamps should have at least a two hour difference
const timeDiff = response.timestamp - previousSession.timestamp,
_twoHours = 60 * 60 * 2000;
expect(timeDiff).toBeGreaterThan(_twoHours);
});

it('timestamp should not change if within two hours', async () => {
// using recent cached session
sessionStorageMock.setItem(
'dds-translation-us-en',
JSON.stringify(responseSuccess)
);

const previousSession = JSON.parse(
sessionStorageMock.getItem('dds-translation-us-en')
);

// reinitializing import
const TranslationAPI = (await import('../Translation')).default;
const response = await TranslationAPI.getTranslation({
lc: 'en',
cc: 'us',
});

// timestamp should remain unchanged
expect(response.timestamp).toEqual(previousSession.timestamp);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have some details on what this case tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use responseSuccess that was just recently refreshed as the cached data. Since it's very recent (just ran), then running the API again shouldn't change the cached data again -- it simply retrieves it. Here we just check if the timestamp remains the same from the previous session.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, given we don't (and won't) change the timestamp of existing cache, probably we wanted to simply see if the unique ID of response equals to one of previousSession?

});
3 changes: 2 additions & 1 deletion packages/services/src/services/Translation/__tests__/data/timestamp_response.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1504,6 +1504,7 @@
"sitenav": "Site navigation",
"welcomeback": "Welcome back"
},
"timestamp": 1337
"timestamp": 1337,
"id": "TRANSLATION_OLD"
}