Skip to content

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

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


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