-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat: migrate enterprise customer business logic to the BFF layer #1263
base: master
Are you sure you want to change the base?
Conversation
13376d0
to
c1beb74
Compare
c1beb74
to
224d886
Compare
5a47226
to
d344054
Compare
d344054
to
d5d6f17
Compare
b97a32a
to
2c11fe8
Compare
2c11fe8
to
fcd9ecd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1263 +/- ##
==========================================
- Coverage 89.59% 89.57% -0.02%
==========================================
Files 405 406 +1
Lines 8824 8809 -15
Branches 2064 2112 +48
==========================================
- Hits 7906 7891 -15
+ Misses 883 882 -1
- Partials 35 36 +1 ☔ View full report in Codecov by Sentry. |
6071f5b
to
cfa68cb
Compare
cfa68cb
to
8dfe053
Compare
@@ -13,11 +13,11 @@ async function extractEnterpriseCustomer({ | |||
queryClient, | |||
authenticatedUser, | |||
enterpriseSlug, | |||
} : ExtractEnterpriseCustomerArgs) : Promise<Types.EnterpriseCustomer> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[curious] Why was this return type removed? Without it, TypeScript won't know this function returns a promise resolving Types.EnterpriseCustomer
without it (currently shows Promise<any>
).
It looks like removing this would be OK iff the allLinkedEnterpriseCustomerUsers
was typed. As is, it looks like this property is associated with the type EnterpriseCustomerUser[]
, however it doesn't seem like the type EnterpriseCustomerUser
exists yet?
For example, defining the EnterpriseCustomerUser
type above EnterpriseLearnerData
makes it so TypeScript can properly infer the type returned for foundEnterpriseCustomerForSlug
.
export interface EnterpriseCustomerUser {
enterpriseCustomer: EnterpriseCustomer;
}
export interface EnterpriseLearnerData {
enterpriseCustomer: Types.EnterpriseCustomer | null;
activeEnterpriseCustomer: Types.EnterpriseCustomer | null;
activeEnterpriseCustomerUserRoleAssignments: any[];
allLinkedEnterpriseCustomerUsers: EnterpriseCustomerUser[];
enterpriseCustomerUserRoleAssignments: any[];
staffEnterpriseCustomer: Types.EnterpriseCustomer | null;
enterpriseFeatures: Types.EnterpriseFeatures;
shouldUpdateActiveEnterpriseCustomerUser: boolean;
}
); | ||
let enterpriseLearnerData: Types.EnterpriseLearnerData | any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[curious] Why is this type using any
? Including it largely defeats the purpose of including Types.EnterpriseLearnerData
, since it forces enterpriseLearnerData
to be any
instead of the actual type, EnterpriseLearnerData
.
let updateActiveEnterpriseCustomerUserResult: { | ||
enterpriseCustomer: any; updatedLinkedEnterpriseCustomerUsers: any; | ||
} | null = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of a code smell to use any
. Should these types by Types.EnterpriseCustomer
and Types.EnterpriseCustomerUser[]
, respectively?
staffEnterpriseCustomer: Types.EnterpriseCustomer; | ||
enterpriseCustomer: Types.EnterpriseCustomer | null; | ||
activeEnterpriseCustomer: Types.EnterpriseCustomer | null; | ||
activeEnterpriseCustomerUserRoleAssignments: any[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[clarification/consideration] Keep in mind that properties now required in EnterpriseLearnerData
must be returned/derived from the BFF-enabled cases, as well. Does activeEnterpriseCustomer
, activeEnterpriseCustomerUserRoleAssignments
, and enterpriseCustomerUserRoleAssignments
need to be included in the BFF API responses via openedx/enterprise-access#624?
Or is it expected/OK that, in the BFF-enabled cases, these data are missing? If expected, should the additional fields be made optional?
if (matchedBFFQuery) { | ||
enterpriseLearnerData = await queryClient.ensureQueryData( | ||
matchedBFFQuery({ enterpriseSlug }), | ||
); | ||
} else { | ||
enterpriseLearnerData = await queryClient.ensureQueryData( | ||
queryEnterpriseLearner(username, enterpriseSlug), | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the enterprise-learner
API is still called on the dashboard route, coming from dashboardLoader
calling extractEnterpriseCustomer
. Perhaps a similar implementation as here, where enterpriseLearnerData
is conditional depending on matchedBFFQuery
, is needed within extractEnterpriseCustomer
, too?
There might be an opportunity to make this logic more DRY across loaders, by abstracting the conditional enterpriseLearnerData
into a helper function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[example] Sample helper function:
export async function getEnterpriseLearnerQueryData({
requestUrl,
queryClient,
enterpriseSlug,
authenticatedUser,
}: ExtractEnterpriseCustomerArgs) {
// Retrieve linked enterprise customers for the current user from query cache
// or fetch from the server if not available.
const matchedBFFQuery = resolveBFFQuery(requestUrl.pathname);
if (matchedBFFQuery) {
const bffResponse = await queryClient.ensureQueryData(
matchedBFFQuery({ enterpriseSlug }),
) as Types.BaseBFFResponse;
const enterpriseLearnerData: Types.EnterpriseLearnerData = {
enterpriseCustomer: bffResponse.enterpriseCustomer,
enterpriseCustomerUserRoleAssignments: bffResponse.enterpriseCustomerUserRoleAssignments,
activeEnterpriseCustomer: bffResponse.activeEnterpriseCustomer,
activeEnterpriseCustomerUserRoleAssignments: bffResponse.activeEnterpriseCustomerUserRoleAssignments,
allLinkedEnterpriseCustomerUsers: bffResponse.allLinkedEnterpriseCustomerUsers,
staffEnterpriseCustomer: bffResponse.staffEnterpriseCustomer,
enterpriseFeatures: bffResponse.enterpriseFeatures,
shouldUpdateActiveEnterpriseCustomerUser: bffResponse.shouldUpdateActiveEnterpriseCustomerUser,
};
return enterpriseLearnerData;
}
const enterpriseLearnerData = await queryClient.ensureQueryData(
queryEnterpriseLearner(authenticatedUser.username, enterpriseSlug),
);
return enterpriseLearnerData;
}
/**
* Extracts the appropriate enterprise ID for the current user and enterprise slug.
*/
async function extractEnterpriseCustomer({
queryClient,
authenticatedUser,
enterpriseSlug,
requestUrl,
} : ExtractEnterpriseCustomerArgs) {
const enterpriseLearnerData = await getEnterpriseLearnerQueryData({
requestUrl,
queryClient,
enterpriseSlug,
authenticatedUser,
});
const {
activeEnterpriseCustomer,
allLinkedEnterpriseCustomerUsers,
staffEnterpriseCustomer,
} = enterpriseLearnerData;
// ...
}
Utilizes the BFF layer for metadata related to the enterprise customer metadata with the following serializer response update for the BFF layer within enterprise access (PR).
This PR also acts as a full cutover for the BFF layer in terms of removing any waffle or MFE config based flagging for the BFF API call. This PR refactors logic such that the determination for a BFF API call is solely dependent on the user's current route and deprecates all BFF related feature flags.
For all changes
Only if submitting a visual change