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.

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

refactor: Remove usages of Persistence First/Last Seen Times #876

Open
wants to merge 2 commits into
base: refactor/SQDSDKS-6347-migrate-persistence-to-store
Choose a base branch
from

Conversation

alexs-mparticle
Copy link
Collaborator

Instructions

  1. PR target branch should be against development
  2. PR title name should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-title-check.yml
  3. PR branch prefix should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-branch-check-name.yml

Summary

  • Remove usages of Persistence First/Last Seen Times

Testing Plan

  • Was this tested locally? If not, explain why.
  • {explain how this has been tested, and what, if any, additional testing should be done}

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

Comment on lines +3115 to +3117
expect(
mParticle.getInstance()._Store.getFirstSeenTime('current')
).to.equal(null);
Copy link
Member

Choose a reason for hiding this comment

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

Can you try the following? Comment out sid and les from the cookies above (line 3095-3096 in this commit)

Does the test in 3115 fail?

I think it would. Because normally init will call identify, and the reason it is not calling it here, and you have to manually call it below this line, is because there is an sid, and/or les. If you step through, I think it will skip the identify call because of that.

If the above is in fact the case, we should add a comment above mParticle.init to explain why it isn't called, to help future developers out.

Comment on lines +1050 to +1052
store.persistenceData['previous-set-mpid'] = {
fst: 100
}
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason you are setting fst for previous-set-mpid this way directly on store.persistenceData instead of how you are setting it for current-mpid and previous-mpid below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is so we can test the possibility of the persistenceData having an existing value, whereas the other tests cases are going through the setters. Perhaps I can move the previous-set-mpid tests higher up so that it's in context?

Copy link
Member

Choose a reason for hiding this comment

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

that sounds fine to me

@@ -1147,6 +1166,29 @@ describe('Store', () => {
expect(fromPersistence[testMPID].lst).to.be.ok;
expect(fromPersistence[testMPID].lst).to.equal(54321);
});

it('returns current time for the current user', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('returns current time for the current user', () => {
it('returns current time for lastSeenTime for the current user', () => {

Comment on lines 1868 to 1869
previous: {},
cu: 'current',
Copy link
Member

Choose a reason for hiding this comment

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

This applies to all the tests you are changing. (And future updates).

I'd say instead of giving the sample MPIDs here previous and current, we shold call it PREVIOUS_MPID/previous_mpid and CURRENT_MPID/current_mpid. This maes it more clear to someone who is reading this code that the key is an MPID.


done();
});

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@@ -3110,7 +3110,13 @@ describe('identity', function() {
setCookie(workspaceCookieName, cookies, true);
mParticle.config.useCookieStorage = true;

mParticle.init(apiKey, mParticle.config);

// As part of init, there is a call to Identity.Identify. However, in this test case,
Copy link
Member

Choose a reason for hiding this comment

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

👍

@alexs-mparticle alexs-mparticle added wait to merge refactor Changes to the structure of the code backlog labels Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog refactor Changes to the structure of the code wait to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants