Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Fix bug that was causing duplicate keys in sessions #1310

Merged
merged 3 commits into from
Apr 1, 2024
Merged

Fix bug that was causing duplicate keys in sessions #1310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96a0aab
Fix bug that was causing duplicate keys in sessions created FromPrope…
lizkenyon Apr 1, 2024
2713366
Add extra fields to sessionParams type
lizkenyon Apr 1, 2024
be86307
Use session instead of prototype
lizkenyon Apr 1, 2024
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
5 changes: 5 additions & 0 deletions .changeset/curly-pears-rest.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@shopify/shopify-api": patch
---

Fix bug that was causing duplicate keys in Session when using FromPropertyArray with returnUserData = true
49 changes: 49 additions & 0 deletions packages/shopify-api/lib/session/__tests__/session.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,7 @@ describe('toPropertyArray and fromPropertyArray', () => {
session.toPropertyArray(test.returnUserData),
test.returnUserData,
);

expect(session.id).toStrictEqual(sessionCopy.id);
expect(session.shop).toStrictEqual(sessionCopy.shop);
expect(session.state).toStrictEqual(sessionCopy.state);
Expand All @@ -466,6 +467,7 @@ describe('toPropertyArray and fromPropertyArray', () => {
expect(session.onlineAccessInfo?.associated_user.id).toStrictEqual(
sessionCopy.onlineAccessInfo?.associated_user.id,
);

if (test.returnUserData) {
expect(
session.onlineAccessInfo?.associated_user.first_name,
Expand Down Expand Up @@ -498,7 +500,54 @@ describe('toPropertyArray and fromPropertyArray', () => {
).toStrictEqual(
sessionCopy.onlineAccessInfo?.associated_user.collaborator,
);
// Test that the user information is correctly moved to the associated_user object from property array
expect(sessionCopy).toEqual(
expect.not.objectContaining({
firstName: session.onlineAccessInfo?.associated_user.first_name,
}),
);
expect(sessionCopy).toEqual(
expect.not.objectContaining({
lastName: session.onlineAccessInfo?.associated_user.last_name,
}),
);
expect(sessionCopy).toEqual(
expect.not.objectContaining({
email: session.onlineAccessInfo?.associated_user.email,
}),
);
expect(sessionCopy).toEqual(
expect.not.objectContaining({
locale: session.onlineAccessInfo?.associated_user.locale,
}),
);
expect(sessionCopy).toEqual(
expect.not.objectContaining({
emailVerified:
session.onlineAccessInfo?.associated_user.email_verified,
}),
);
expect(sessionCopy).toEqual(
expect.not.objectContaining({
accountOwner:
session.onlineAccessInfo?.associated_user.account_owner,
}),
);
expect(sessionCopy).toEqual(
expect.not.objectContaining({
collaborator:
session.onlineAccessInfo?.associated_user.collaborator,
}),
);
expect(sessionCopy).toEqual(
expect.not.objectContaining({
associated_user: {id: session.onlineAccessInfo?.associated_user.id},
}),
);
} else {
expect(sessionCopy.onlineAccessInfo?.associated_user?.id).toStrictEqual(
session.onlineAccessInfo?.associated_user?.id,
);
expect(
sessionCopy.onlineAccessInfo?.associated_user.first_name,
).toBeUndefined();
Expand Down
182 changes: 80 additions & 102 deletions packages/shopify-api/lib/session/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint-disable no-fallthrough */
import {InvalidSession} from '../error';
import {OnlineAccessInfo, OnlineAccessUser} from '../auth/oauth/types';
import {OnlineAccessInfo} from '../auth/oauth/types';
import {AuthScopes} from '../auth/scopes';

import {SessionParams} from './types';
Expand All @@ -16,9 +16,6 @@ const propertiesToSave = [
'onlineAccessInfo',
];

interface AssociatedUserObject {
associated_user: Partial<OnlineAccessUser>;
}
/**
* Stores App information from logged in merchants so they can make authenticated requests to the Admin API.
*/
Expand All @@ -33,9 +30,6 @@ export class Session {
);
}

const associatedUserObj: AssociatedUserObject = {
associated_user: {},
};
const obj = Object.fromEntries(
entries
.filter(([_key, value]) => value !== null && value !== undefined)
Expand All @@ -61,103 +55,87 @@ export class Session {
default:
return [key.toLowerCase(), value];
}
})
}),
);

// Sanitize values
.map(([key, value]) => {
switch (key) {
case 'isOnline':
if (typeof value === 'string') {
return [key, value.toString().toLowerCase() === 'true'];
} else if (typeof value === 'number') {
return [key, Boolean(value)];
}
return [key, value];
case 'scope':
return [key, value.toString()];
case 'expires':
return [key, value ? new Date(Number(value)) : undefined];
case 'onlineAccessInfo':
return [
key,
{
associated_user: {
id: Number(value),
},
},
];
case 'userId':
if (returnUserData) {
return [
key,
(associatedUserObj.associated_user.id = Number(value)),
];
}
case 'firstName':
if (returnUserData) {
return [
key,
(associatedUserObj.associated_user.first_name =
String(value)),
];
}
case 'lastName':
if (returnUserData) {
return [
key,
(associatedUserObj.associated_user.last_name = String(value)),
];
}
case 'email':
if (returnUserData) {
return [
key,
(associatedUserObj.associated_user.email = String(value)),
];
}
case 'accountOwner':
if (returnUserData) {
return [
key,
(associatedUserObj.associated_user.account_owner =
Boolean(value)),
];
}
case 'locale':
if (returnUserData) {
return [
key,
(associatedUserObj.associated_user.locale = String(value)),
];
}
case 'collaborator':
if (returnUserData) {
return [
key,
(associatedUserObj.associated_user.collaborator =
Boolean(value)),
];
}
case 'emailVerified':
if (returnUserData) {
return [
key,
(associatedUserObj.associated_user.email_verified =
Boolean(value)),
];
}
// If returnUserData is false, return any user keys as passed in
default:
return [key, value];
const sessionData = {
onlineAccessInfo: {
associated_user: {},
},
} as SessionParams;
Object.entries(obj).forEach(([key, value]) => {
switch (key) {
case 'isOnline':
if (typeof value === 'string') {
sessionData[key] = value.toString().toLowerCase() === 'true';
} else if (typeof value === 'number') {
sessionData[key] = Boolean(value);
} else {
sessionData[key] = value;
}
}),
) as any;
// If the onlineAccessInfo is not present, we are using the new session info and add it to the object
if (returnUserData) {
obj.onlineAccessInfo = associatedUserObj;
}
Object.setPrototypeOf(obj, Session.prototype);
return obj;
break;
case 'scope':
sessionData[key] = value.toString();
break;
case 'expires':
sessionData[key] = value ? new Date(Number(value)) : undefined;
break;
case 'onlineAccessInfo':
sessionData.onlineAccessInfo!.associated_user.id = Number(value);
break;
case 'userId':
if (returnUserData) {
sessionData.onlineAccessInfo!.associated_user.id = Number(value);
break;
}
case 'firstName':
if (returnUserData) {
sessionData.onlineAccessInfo!.associated_user.first_name =
String(value);
break;
}
case 'lastName':
if (returnUserData) {
sessionData.onlineAccessInfo!.associated_user.last_name =
String(value);
break;
}
case 'email':
if (returnUserData) {
sessionData.onlineAccessInfo!.associated_user.email = String(value);
break;
}
case 'accountOwner':
if (returnUserData) {
sessionData.onlineAccessInfo!.associated_user.account_owner =
Boolean(value);
break;
}
case 'locale':
if (returnUserData) {
sessionData.onlineAccessInfo!.associated_user.locale =
String(value);
break;
}
case 'collaborator':
if (returnUserData) {
sessionData.onlineAccessInfo!.associated_user.collaborator =
Boolean(value);
break;
}
case 'emailVerified':
if (returnUserData) {
sessionData.onlineAccessInfo!.associated_user.email_verified =
Boolean(value);
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! One last question - can we replace setting the prototype of sessionData in line 137 with creating an actual Session instance? Not a blocker if we can't!

// Return any user keys as passed in
default:
sessionData[key] = value;
}
});
const session = new Session(sessionData);
return session;
}

/**
Expand Down
4 changes: 4 additions & 0 deletions packages/shopify-api/lib/session/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ export interface SessionParams {
* Information on the user for the session. Only present for online sessions.
*/
onlineAccessInfo?: OnlineAccessInfo | StoredOnlineAccessInfo;
/**
* Additional properties of the session allowing for extension
*/
[key: string]: any;
}

type StoredOnlineAccessInfo = Omit<OnlineAccessInfo, 'associated_user'> & {
Expand Down
Loading