-
Notifications
You must be signed in to change notification settings - Fork 11
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: incorporate enterprise customer logic into BFF #624
Conversation
40d339f
to
65bc967
Compare
@@ -222,13 +227,41 @@ def _initialize_enterprise_customer_users(self): | |||
self.enterprise_customer_uuid, | |||
self.enterprise_customer_slug, | |||
) | |||
|
|||
# sets the active enterprise customer in the LMS |
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] given the concerns with CSRF, and your mentioned plan to keep setting the active ECU via the learner portal MFE like we do today, is the intent to remove this logic for the time being?
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 intend to keep PARTS of this logic in place because it handles the data transformation that is currently done in the frontend. What the frontend does once this and the frontend related PR merges is dependent on if the active flag is changed, and the route the customer is on.
If the customer enters the learner portal from a non-bff route, it falls back to the existing logic today which does the data transformation in the frontend and sets the active customer. If the enterprise customer enters the learner portal from a BFF enabled route, the BFF layer intends to complete the data transformation related to the active enterprise customer (if the active enterprise customer changes) and pass a flag to the frontend to actually set the active enterprise customer. Long term, we intend on creating a usable API to set the active enterprise customer within the BFF and not depend on the frontend to set the active enterprise customer once the rollout of the BFF is complete across all learner portal. routes.
@@ -222,13 +227,41 @@ def _initialize_enterprise_customer_users(self): | |||
self.enterprise_customer_uuid, | |||
self.enterprise_customer_slug, | |||
) | |||
|
|||
# sets the active enterprise customer in the LMS |
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.
[consideration] Related, if we do keep the setting of the active ECU in the frontend for now, we should consider the implications for caching on the enterprise-learner
API, called by the BFF layer. If the frontend starts consuming the enterprise customer metadata returned by the BFF but the frontend mutates the active
ECU, will the BFF be returning stale cached data with the previous active
ECU? Do you think we may need to remove TieredCache
caching on the enterprise-learner
call in the BFF layer, such that if/when the frontend mutates the active
ECU, the BFF's enterprise customer metadata reflects that change sooner than the current 10 minutes cache timeout?
Alternatively, another way to mitigate might be to move the enterprise-learner
call from TieredCache
to RequestCache
, so it's good for the duration of the single request, but not persisted beyond the single request (i.e., it'll re-fetch on subsequent calls).
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 this is resolved by triggering the invalidation on the cache if we are updating the active enterprise customer.
If the customers initial point of entry is on a non-bff enabled route and the requested enterprise customer (slug) is not the same as the users current active enterprise customer, and they traverse to a bff enabled route, the request value's slug should be the same one we retrieve for the enterprise customer user. We would have already updated the active customer from the non-bff enabled route, so the active enterprise customer would be the same as the enterprise customer user retrieved from the slug.
If the customer's initial point of entry is on a bff enabled route, route and the requested enterprise customer (slug) is not the same as the users current active enterprise customer, we would optimistically update the return value from the enterprise customer user (which would be set in the frontend query cache), set the active enterprise customer to the requested enterprise customer, and invalidate the cache. The returned should_update_active_enterprise_customer_user
field would update the active enterprise customer to the requested enterprise customer in the frontend. The next call on a bff enabled route would result in a new call and updated query cache with the requested enterprise customer.
If something in my logic seems off lmk but I believe the invalidation of the cache handles it. 👍🏽
b23dc3b
to
3846230
Compare
3846230
to
96e5cfb
Compare
if should_update_active_enterprise_customer_user := transformed_data.get( | ||
'should_update_active_enterprise_customer_user', | ||
): | ||
enterprise_customer = transformed_data.get('enterprise_customer') | ||
enterprise_customer_uuid = enterprise_customer.get('uuid') | ||
try: | ||
invalidate_enterprise_customer_users(username=self.request.user.username) | ||
transformed_data.update({ | ||
'active_enterprise_customer': enterprise_customer, | ||
'all_linked_enterprise_customer_users': [ | ||
{ | ||
**enterprise_customer_user, | ||
"active": ( | ||
enterprise_customer_user["enterprise_customer"]["uuid"] == enterprise_customer_uuid | ||
) | ||
} | ||
for enterprise_customer_user in transformed_data.get('all_linked_enterprise_customer_users', []) | ||
] | ||
}) |
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] If the BFF is not yet calling the POST /select/active
API to mutate the active ECU, is it appropriate for the BFF to do in-memory data transforms related to the active enterprise customer, besides returning the added should_update_active_enterprise_customer_user
to tell the frontend when to call /select/active
?
For example, at the time of this request when should_update_active_enterprise_customer_user
is truthy, the active ECU has not yet been changed; it takes the frontend to make the POST (with an optimistic query cache update on the frontend), and then when the BFF API is next refetched, it should reflect the changed active ECU. Despite the active ECU not changing at the time of the BFF request, the transformed_data
is largely be mutated as if it were.
Rather than calling invalidate_enterprise_customer_users
and subsequently doing data transforms a bit prematurely, given the frontend is still going to be calling /select/active
for the time being, I wonder if we should effectively change the cache timeout for the enterprise-learner
API call within the BFF layer to 0
(aka effectively removing persisted TieredCache
).
However, because the enterprise-learner
is potentially called twice (i.e., once during the @permission_required
check and again during the handler data loading), we should still have some caching on it to avoid duplicative requests.
There might be an opportunity to change the caching the enterprise-learner
API request to use RequestCache
instead of TieredCache
so the cached response is only valid for the duration of the request (i.e., response not persisted in cache across multiple requests).
That way, BFF is responsible for only telling the Learner Portal when to call /select/active
per should_update_active_enterprise_customer_user
and the next request to the BFF API will re-fetch enterprise-learner
to get the updated active ECU in its response.
Let me know if you want to 🦆 about this approach further.
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.
Request cache implemented 👍🏽 and the learner portal behavior is what we expect to see for setting an active customer and correctly returning the enterprise customer we expect
96e5cfb
to
f50740e
Compare
4ef3486
to
ef5cf82
Compare
@@ -222,13 +226,42 @@ def _initialize_enterprise_customer_users(self): | |||
self.enterprise_customer_uuid, | |||
self.enterprise_customer_slug, | |||
) | |||
|
|||
# Sets the active enterprise customer metadata in the LMS | |||
if should_update_active_enterprise_customer_user := transformed_data.get( |
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.
Similar question remains here around whether we should be doing any in-memory mutation of metadata based on should_update_active_enterprise_customer_user
, given the BFF APIs are not actually mutating the active ECU yet, the Learner Portal MFE still is.
Flow:
- Learner visits
/test-enterprise-A
, while their active ECU is fortest-enterprise-B
. - Learner Portal calls BFF on dashboard route.
- BFF returns that
should_update_active_enterprise_customer_user: True
for/test-enterprise-A
.- The active ECU is still the same as it was upon the original request, i.e.
test-enterprise-B
.
- The active ECU is still the same as it was upon the original request, i.e.
- Learner Portal calls
/select/active
to change the active ECU for the session. - Only on subsequent refetch to the BFF APIs should the
active_enterprise_customer
be returned astest-enterprise-A
, not the original request to the BFF.
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.
[follow-up] Ah, I'm understanding the current state of how this interacts with the corresponding learner portal PR.
// The logic to ensure active enterprise customer user is done in the BFF, but updating the
// active enterprise customer is still required
While these 2 pieces (the BFF API + learner portal) are intended to play nicely together, they should be developed as independent applications with minimal coupling. That is, when relying on frontend mutation via select/active
to update the active ECU, it's a bit odd to assume the backend API return the expected mutated data prior to actually mutating the data. Consider if the API is called in isolation via Postman, where the /select/active
is not yet called (since it's only called with Learner Portal).
If we mutate the in-memory data here in the Postman-only use case, the data representing the active ECU is technically misleading as the /select/active
API was not yet called.
2cc86ae
to
2a9888e
Compare
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.
LGTM, with a non-blocking nit. Nice job!
2a9888e
to
b14c4e7
Compare
Serializes fields in the BFF context to always return
all_linked_enterprise_customer_users
andshould_update_active_enterprise_customer_user
. The intention is to eventually removeshould_update_active_enterprise_customer_user
and take advantage of a net new API call to select an active enterprise customer. This work will be completed as part of a seperate ticket.It modifies the enterprise customer user object to update the metadata to the set active, and invalidate the enterprise customer user query cache. We return
should_update_active_enterprise_customer_user
back to the user to set the active enterprise customer in the frontend. We invalidate the query to ensure the next call to the BFF would return the now active enterprise customer without transformations on the enterprise customer user object in the BFF layer.Jira:
ENT-XXXX
Merge checklist:
./manage.py makemigrations
has been runPost merge: