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

Id 367 get user registration from sam #4716

Merged
merged 23 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2ec08f8
wip get registered date from sam
Shakespeared Mar 9, 2024
1b71e1e
get registration date from sam
Shakespeared Mar 12, 2024
e720f8d
streamline and store samUser in state
Shakespeared Mar 12, 2024
1de9e4b
wip add unit tests
Shakespeared Mar 14, 2024
bcce5cf
Merge branch 'dev' into id-367-get-user-registration-from-sam
Shakespeared Mar 14, 2024
36e3096
add test for failure case
Shakespeared Mar 18, 2024
24c9125
remove leftover rex code
Shakespeared Mar 18, 2024
0c43b3c
Merge branch 'dev' into id-367-get-user-registration-from-sam
Shakespeared Mar 19, 2024
b4ddadc
reorder const for consistency
Shakespeared Mar 19, 2024
6223873
fix tests after merge from dev
Shakespeared Mar 19, 2024
ef7e880
fix wonky imports
Shakespeared Mar 19, 2024
b964235
fix one more test
Shakespeared Mar 19, 2024
ce22711
make mock return values more realistic
Shakespeared Mar 19, 2024
0218e8d
fully initialize samUserResponse in state
Shakespeared Mar 19, 2024
a4479d5
Merge branch 'dev' into id-367-get-user-registration-from-sam
Shakespeared Mar 19, 2024
a1bef94
add explanatory comment
Shakespeared Mar 19, 2024
bb7bd0a
handle if registeredAt is null in the sam db
Shakespeared Mar 20, 2024
b7dd27f
simplify after updates in Sam
Shakespeared Mar 25, 2024
5dbf432
put back ui default to epoch
Shakespeared Mar 26, 2024
6815a64
allow initialization of samuser with undefined
Shakespeared Mar 26, 2024
dcb76f4
Merge branch 'dev' into id-367-get-user-registration-from-sam
Shakespeared Mar 26, 2024
534a5a9
nit remove extra undefined on optional fields
Shakespeared Mar 26, 2024
d00f514
Merge branch 'dev' into id-367-get-user-registration-from-sam
Shakespeared Apr 2, 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
1 change: 0 additions & 1 deletion config/alpha.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
"leoUrlRoot": "https://leonardo.dsde-alpha.broadinstitute.org",
"orchestrationUrlRoot": "https://firecloud-orchestration.dsde-alpha.broadinstitute.org",
"rawlsUrlRoot": "https://rawls.dsde-alpha.broadinstitute.org",
"rexUrlRoot": "https://terra-rex-alpha.appspot.com",
"samUrlRoot": "https://sam.dsde-alpha.broadinstitute.org",
"shibbolethUrlRoot": "https://broad-shibboleth-prod.appspot.com/dev",
"workspaceManagerUrlRoot": "https://workspace.dsde-alpha.broadinstitute.org",
Expand Down
1 change: 0 additions & 1 deletion config/dev.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
"leoUrlRoot": "https://leonardo.dsde-dev.broadinstitute.org",
"orchestrationUrlRoot": "https://firecloud-orchestration.dsde-dev.broadinstitute.org",
"rawlsUrlRoot": "https://rawls.dsde-dev.broadinstitute.org",
"rexUrlRoot": "https://terra-rex-dev.appspot.com",
"samUrlRoot": "https://sam.dsde-dev.broadinstitute.org",
"shibbolethUrlRoot": "https://broad-shibboleth-prod.appspot.com/dev",
"workspaceManagerUrlRoot": "https://workspace.dsde-dev.broadinstitute.org",
Expand Down
1 change: 0 additions & 1 deletion config/prod.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
"leoUrlRoot": "https://notebooks.firecloud.org",
"orchestrationUrlRoot": "https://api.firecloud.org",
"rawlsUrlRoot": "https://rawls.dsde-prod.broadinstitute.org",
"rexUrlRoot": "https://terra-rex-prod.appspot.com",
"samUrlRoot": "https://sam.dsde-prod.broadinstitute.org",
"shibbolethUrlRoot": "https://broad-shibboleth-prod.appspot.com",
"workspaceManagerUrlRoot": "https://workspace.dsde-prod.broadinstitute.org",
Expand Down
1 change: 0 additions & 1 deletion config/staging.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
"leoUrlRoot": "https://leonardo.dsde-staging.broadinstitute.org",
"orchestrationUrlRoot": "https://firecloud-orchestration.dsde-staging.broadinstitute.org",
"rawlsUrlRoot": "https://rawls.dsde-staging.broadinstitute.org",
"rexUrlRoot": "https://terra-rex-staging.appspot.com",
"samUrlRoot": "https://sam.dsde-staging.broadinstitute.org",
"shibbolethUrlRoot": "https://broad-shibboleth-prod.appspot.com/dev",
"workspaceManagerUrlRoot": "https://workspace.dsde-staging.broadinstitute.org",
Expand Down
29 changes: 19 additions & 10 deletions src/auth/auth.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { parseJSON } from 'date-fns/fp';
import jwtDecode, { JwtPayload } from 'jwt-decode';
import _ from 'lodash/fp';
import {
Expand Down Expand Up @@ -446,8 +445,14 @@ authStore.subscribe(
withErrorIgnoring(async (state: AuthState, oldState: AuthState) => {
if (!oldState.termsOfService.permitsSystemUsage && state.termsOfService.permitsSystemUsage) {
if (window.Appcues) {
window.Appcues.identify(userStore.get().terraUser.id!, {
dateJoined: parseJSON((await Ajax().User.firstTimestamp()).timestamp).getTime(),
const { terraUser, samUser } = userStore.get();
// for Sam users who have been invited but not yet registered
// and for a set of users who didn't have registration dates to migrate into Sam
// registeredAt may be null in the Sam db. In that case, default to epoch (1970) instead
// so the survey won't be immediately displayed
const dateJoined = samUser.registeredAt ? samUser.registeredAt.getTime() : new Date('1970-01-01').getTime();
window.Appcues.identify(terraUser.id!, {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also just rely on the createdAt date, since its not nullable in Sam's DB. That might be preferable, just in case there's a real createdAt, but not a registeredAt, for some reason.

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 don't think we want to user createdAt because then a user who is invited, waits a month to register, then comes to Terra would immediately be shown the survey

tlangs marked this conversation as resolved.
Show resolved Hide resolved
dateJoined,
});
window.Appcues.on('all', captureAppcuesEvent);
}
Expand Down Expand Up @@ -492,19 +497,23 @@ export const loadTerraUser = async (): Promise<void> => {
const getAttributes = Ajax().User.getUserAttributes();
const getTermsOfService = Ajax().TermsOfService.getUserTermsOfServiceDetails();
const getEnterpriseFeatures = Ajax().User.getEnterpriseFeatures();
const [profile, terraUserAllowances, terraUserAttributes, termsOfService, enterpriseFeatures] = await Promise.all([
getProfile,
getAllowances,
getAttributes,
getTermsOfService,
getEnterpriseFeatures,
]);
const getSamUser = Ajax().User.getSamUserResponse();
const [profile, terraUserAllowances, terraUserAttributes, termsOfService, enterpriseFeatures, samUser] =
await Promise.all([
getProfile,
getAllowances,
getAttributes,
getTermsOfService,
getEnterpriseFeatures,
getSamUser,
]);
clearNotification(sessionTimeoutProps.id);
userStore.update((state: TerraUserState) => ({
...state,
profile,
terraUserAttributes,
enterpriseFeatures,
samUser,
}));
authStore.update((state: AuthState) => ({
...state,
Expand Down
184 changes: 184 additions & 0 deletions src/auth/login.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
import { DeepPartial } from '@terra-ui-packages/core-utils';
import { asMockedFn } from '@terra-ui-packages/test-utils';
import { act } from '@testing-library/react';
import { loadTerraUser } from 'src/auth/auth';
import { Ajax } from 'src/libs/ajax';
import { GroupRole } from 'src/libs/ajax/Groups';
import { SamUserTermsOfServiceDetails } from 'src/libs/ajax/TermsOfService';
import { SamUserResponse } from 'src/libs/ajax/User';
import { TerraUserState, userStore } from 'src/libs/state';

jest.mock('src/libs/ajax');

type AjaxExports = typeof import('src/libs/ajax');
type AjaxContract = ReturnType<AjaxExports['Ajax']>;

jest.mock('react-notifications-component', () => {
return {
Store: {
addNotification: jest.fn(),
removeNotification: jest.fn(),
},
};
});

const samUserDate = new Date('1970-01-01');

const mockSamUserResponse: SamUserResponse = {
id: 'testId',
googleSubjectId: 'testGoogleSubjectId',
email: 'testEmail',
azureB2CId: 'testAzureB2CId',
allowed: true,
createdAt: samUserDate,
registeredAt: samUserDate,
updatedAt: samUserDate,
};

const mockSamUserTermsOfServiceDetails: SamUserTermsOfServiceDetails = {
latestAcceptedVersion: '1234',
acceptedOn: samUserDate,
permitsSystemUsage: true,
isCurrentVersion: true,
};

const mockTerraUserProfile = {
firstName: 'testFirstName',
lastName: 'testLastName',
institute: 'testInstitute',
contactEmail: 'testContactEmail',
title: 'testTitle',
department: 'testDepartment',
interestInTerra: 'testInterestInTerra',
programLocationCity: 'testProgramLocationCity',
programLocationState: 'testProgramLocationState',
programLocationCountry: 'testProgramLocationCountry',
researchArea: 'testResearchArea',
starredWorkspaces: 'testStarredWorkspaces',
};
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!


const testSamUserAllowancesDetails = {
enabled: true,
termsOfService: true,
};

const testSamUserAllowances = {
allowed: true,
details: testSamUserAllowancesDetails,
};

const mockNihDatasetPermission = {
name: 'testNihDatasetPermissionName',
authorized: true,
};

const mockOrchestrationNihStatusResponse = {
linkedNihUsername: 'testLinkedNihUsername',
datasetPermissions: mockNihDatasetPermission,
linkExpireTime: 1234,
};

const mockCurrentUserGroupMembership = {
groupEmail: 'testGroupEmail',
groupName: 'testGroupName',
role: 'member' as GroupRole,
};

// TODO centralize Ajax mock setup so it can be reused across tests
describe('a request to load a terra user', () => {
// reset userStore state before each test
beforeEach(() => {
userStore.reset;
});
describe('when successful', () => {
// Arrange (shared between tests for the success case)
const getUserAllowancesFunction = jest.fn().mockResolvedValue(testSamUserAllowances);
const getUserAttributesFunction = jest.fn().mockResolvedValue({ marketingConsent: false });
const getUserTermsOfServiceDetailsFunction = jest.fn().mockResolvedValue(mockSamUserTermsOfServiceDetails);
const getEnterpriseFeaturesFunction = jest.fn().mockResolvedValue([]);
const getSamUserResponseFunction = jest.fn().mockResolvedValue(mockSamUserResponse);
const getNihStatusFunction = jest.fn().mockResolvedValue(mockOrchestrationNihStatusResponse);
const getFenceStatusFunction = jest.fn().mockResolvedValue({});

asMockedFn(Ajax).mockImplementation(
() =>
({
User: {
getUserAllowances: getUserAllowancesFunction,
getUserAttributes: getUserAttributesFunction,
getUserTermsOfServiceDetails: getUserTermsOfServiceDetailsFunction,
getEnterpriseFeatures: getEnterpriseFeaturesFunction,
getSamUserResponse: getSamUserResponseFunction,
getNihStatus: getNihStatusFunction,
getFenceStatus: getFenceStatusFunction,
profile: {
get: jest.fn().mockReturnValue(mockTerraUserProfile),
},
},
TermsOfService: {
getUserTermsOfServiceDetails: jest.fn().mockReturnValue({}),
},
Groups: {
list: jest.fn().mockReturnValue([mockCurrentUserGroupMembership]),
},
} as DeepPartial<AjaxContract> as AjaxContract)
);
Shakespeared marked this conversation as resolved.
Show resolved Hide resolved
it('should include a samUserResponse', async () => {
// Act
await act(() => loadTerraUser());

// Assert
expect(getSamUserResponseFunction).toHaveBeenCalled();
});
it('should update the samUser in state', async () => {
// Act
await act(() => loadTerraUser());

let samUser;
await act(async () => {
samUser = await getSamUserResponseFunction.mock.results[0].value;
});
userStore.update((state: TerraUserState) => ({
...state,
samUser,
}));
// Assert
expect(getSamUserResponseFunction).toHaveBeenCalled();
expect(userStore.get().samUser).toEqual(mockSamUserResponse);
});
describe('when not successful', () => {
it('should fail with an error', async () => {
// // Arrange
// mock a failure to get samUserResponse
const getSamUserResponseFunction = jest.fn().mockRejectedValue(new Error('unknown'));

asMockedFn(Ajax).mockImplementation(
() =>
({
User: {
getUserAllowances: getUserAllowancesFunction,
getUserAttributes: getUserAttributesFunction,
getUserTermsOfServiceDetails: getUserTermsOfServiceDetailsFunction,
getEnterpriseFeatures: getEnterpriseFeaturesFunction,
getSamUserResponse: getSamUserResponseFunction,
profile: {
get: jest.fn().mockReturnValue(mockTerraUserProfile),
},
},
TermsOfService: {
getUserTermsOfServiceDetails: jest.fn().mockReturnValue({}),
},
} as DeepPartial<AjaxContract> as AjaxContract)
);
// Act, Assert
// this expect.assertions is here to prevent the test from passing if the error is not thrown
expect.assertions(1);
Shakespeared marked this conversation as resolved.
Show resolved Hide resolved
try {
await act(() => loadTerraUser());
} catch (error) {
expect(error).toEqual(new Error('unknown'));
}
});
});
});
});
35 changes: 11 additions & 24 deletions src/libs/ajax/User.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import _ from 'lodash/fp';
import * as qs from 'qs';
import { authOpts, fetchBond, fetchOrchestration, fetchRex, fetchSam, jsonBody } from 'src/libs/ajax/ajax-common';
import { getTerraUser, TerraUserProfile } from 'src/libs/state';
import * as Utils from 'src/libs/utils';
import { authOpts, fetchBond, fetchOrchestration, fetchSam, jsonBody } from 'src/libs/ajax/ajax-common';
import { TerraUserProfile } from 'src/libs/state';

export interface SamUserRegistrationStatusResponse {
userSubjectId: string;
Expand Down Expand Up @@ -154,19 +153,15 @@ export interface SamInviteUserResponse {
userEmail: string;
}

export interface RexFirstTimestampResponse {
timestamp: Date;
}

export interface SamUserResponse {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the undefined so we can initialize with an empty object in state

id: string;
id: string | undefined;
googleSubjectId?: string;
email: string;
email: string | undefined;
azureB2CId?: string;
allowed: boolean;
createdAt: Date;
registeredAt?: Date;
updatedAt: Date;
allowed: boolean | undefined;
createdAt: Date | undefined;
registeredAt: Date | undefined;
updatedAt: Date | undefined;
}

export type SamUserAttributes = {
Expand All @@ -179,15 +174,6 @@ export type SamUserAttributesRequest = {

export type OrchestrationUserRegistrationRequest = object;

// TODO: Remove this as a part of https://broadworkbench.atlassian.net/browse/ID-460
const getFirstTimeStamp = Utils.memoizeAsync(
async (token): Promise<RexFirstTimestampResponse> => {
const res = await fetchRex('firstTimestamps/record', _.mergeAll([authOpts(token), { method: 'POST' }]));
return res.json();
},
{ keyFn: (...args) => JSON.stringify(args) }
) as (token: string) => Promise<RexFirstTimestampResponse>;

export const User = (signal?: AbortSignal) => {
return {
getStatus: async (): Promise<SamUserRegistrationStatusResponse> => {
Expand Down Expand Up @@ -281,8 +267,9 @@ export const User = (signal?: AbortSignal) => {
return res.json();
},

firstTimestamp: (): Promise<RexFirstTimestampResponse> => {
return getFirstTimeStamp(getTerraUser().token!);
getSamUserResponse: async (): Promise<SamUserResponse> => {
const res = await fetchSam('api/users/v2/self', _.mergeAll([authOpts(), { method: 'GET' }]));
return res.json();
},

getNihStatus: async (): Promise<OrchestrationNihStatusResponse | undefined> => {
Expand Down
5 changes: 0 additions & 5 deletions src/libs/ajax/ajax-common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,11 +258,6 @@ export const fetchOrchestration = _.flow(
withRetryAfterReloadingExpiredAuthToken
)(fetchOk);

export const fetchRex = _.flow(
withUrlPrefix(`${getConfig().rexUrlRoot}/api/`),
withRetryAfterReloadingExpiredAuthToken
)(fetchOk);

export const fetchBond = _.flow(
withUrlPrefix(`${getConfig().bondUrlRoot}/`),
withRetryAfterReloadingExpiredAuthToken
Expand Down
12 changes: 12 additions & 0 deletions src/libs/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
NihDatasetPermission,
SamUserAllowances,
SamUserAttributes,
SamUserResponse,
} from 'src/libs/ajax/User';
import { getLocalStorage, getSessionStorage, staticStorageSlot } from 'src/libs/browser-storage';
import type { WorkspaceWrapper } from 'src/workspaces/utils';
Expand Down Expand Up @@ -133,6 +134,7 @@ export interface TerraUserState {
terraUser: TerraUser;
terraUserAttributes: SamUserAttributes;
enterpriseFeatures: string[];
samUser: SamUserResponse;
}

/**
Expand Down Expand Up @@ -167,6 +169,16 @@ export const userStore: Atom<TerraUserState> = atom<TerraUserState>({
marketingConsent: true,
},
enterpriseFeatures: [],
samUser: {
id: undefined,
googleSubjectId: undefined,
email: undefined,
azureB2CId: undefined,
allowed: undefined,
createdAt: undefined,
registeredAt: undefined,
updatedAt: undefined,
},
});

export const getTerraUser = (): TerraUser => userStore.get().terraUser;
Expand Down
Loading
Loading