feat: auto setting active linked enterprise

[alex-sheehan-edx]

https://2u-internal.atlassian.net/browse/ENT-8038

For all changes

• Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

• Ensure to attach screenshots
• Ensure to have UX team confirm screenshots

[codecov]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (cc98d50) 85.33% compared to head (ff2820a) 85.34%.

❗ Current head ff2820a differs from pull request most recent head 80ebfb1. Consider uploading reports for the commit 80ebfb1 to get more accurate results

Files	Patch %	Lines
src/components/App/index.jsx	0.00%	3 Missing ⚠️
src/data/services/LmsApiService.js	89.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1153      +/-   ##
==========================================
+ Coverage   85.33%   85.34%   +0.01%     
==========================================
  Files         495      496       +1     
  Lines       10652    10694      +42     
  Branches     2232     2239       +7     
==========================================
+ Hits         9090     9127      +37     
- Misses       1520     1525       +5     
  Partials       42       42              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[adamstankiewicz]

nit: likely don't necessarily need to pass currentEnterpriseId. This hook function itself gets enterpriseId as an argument, and it'd be available within updateActiveEnterpriseAndRefreshJWT without it as an explicit function argument.

[alex-sheehan-edx]

I didn't realize useCallback's get access to arguments passed to the hook!

[adamstankiewicz]

[curious, non-blocking] Might consider what scope is involved in refactoring this hook a bit to utilize @tanstack/react-query? What benefits might we get from React Query in relation to the scope? If the benefits are not significant in this particular use case, OK to not utilize React Query here.

E.g., removes boilerplate around needing to manage our own loading states, etc.

[adamstankiewicz]

[curious] I'm wondering if we need the loginRefresh call here anymore now that we're no longer making assumptions of JWT role ordering. That is, I believe we were calling loginRefresh because our prior assumption that the JWT roles would be re-ordered.

Given that this hook no longer cares about the JWT roles, could we nix the call to login_refresh?

[alex-sheehan-edx]

oh yeah, that's a great point! It's all done properly now 😄

[adamstankiewicz]

nit: might recommend using async/await over .then chaining.

[adamstankiewicz]

[suggestion] might be able to trim/simplify this logic down a bit by relying on .find(...) instead of explicitly looping through each record to find the active=true?

static async getActiveLinkedEnterprise(username) {
    const response = await this.fetchEnterpriseLearnerData({ username });
    const transformedResponse = camelCaseObject(response.data);
    const enterprisesForLearner = transformedResponse.results;
    const activeLinkedEnterprise = enterprisesForLearner.find(enterprise => enterprise.active);
    if (!activeLinkedEnterprise) {
      logError(`${username} does not have any active linked enterprise customers`);
    }
}

[adamstankiewicz]

[curious] When would data be undefined? Do we need to use the optional chaining you have here? If we do, when data is undefined, results would also be undefined and would subsequently throw a JS error on line 431 when trying to access results.length.

[alex-sheehan-edx]

I think I was mostly just following the pattern in the file- but I agree I can't think of a reason why it would be undefined.

[adamstankiewicz]

If there is no active enterprise customer (super edge case, one should always exist), likely should return undefined or null instead of empty string.

[adamstankiewicz]

nit: generally a good idea to return everything from useQuery's output in useUpdateActiveEnterpriseForUser (as opposed to cherry-picking just isLoading). That said, if you also adopt useMutation here per the above suggestion (non-blocking), then this comment isn't as relevant (since there'd be 2 loading states, 2 error states, etc.).

[adamstankiewicz]

Would recommend using the onError callback function passed to useQuery for any error logging as opposed to handling your own conditional here.

[adamstankiewicz]

Treat queryKey like dependency arrays. Any data that is utilized by queryFn should likely be part of the query key.

For example, if this query is specific to the given username, that username should be part of the query key. Rationale: the API call for a specific username is stored in the query cache based on the queryKey; without the username in the query key, useQuery will think it has the API data still cached even if that username has since changed.

[alex-sheehan-edx]

makes sense!

[adamstankiewicz]

nit: consider creating a query key factory similar to learnerCreditManagementQueryKeys. See https://tkdodo.eu/blog/effective-react-query-keys#use-query-key-factories.

That said, totally fine to defer on this feedback for if/when we take on the work to migrate existing API calls to useQuery where query keys will become more important across the application.

[adamstankiewicz]

Typically, you wouldn't use .then chaining when using async and await, e.g.:

const activeLinkedEnterprise = await LmsApiService.getActiveLinkedEnterprise(username);
if (activeLinkedEnterprise.uuid !== enterpriseId) { ... }

[adamstankiewicz]

[suggestion] Generally, POST API calls are done with useMutation; useQuery is intended for reading data, not updating. While what you have should work, you might consider making use of useQuery for the GET request and useMutation for the POST request.

Main rationale is to avoid side effects that change server data within a useQuery (kind of similar to avoiding side effects in a GET API endpoint's implementation).

Something along the lines of the following (consider it pseudocode):

const useUpdateActiveEnterpriseForUser = ({ enterpriseId, user }) => {
  // Sets up POST call to update active enterprise.
  const { mutateAsync, isLoading: isUpdatingActiveEnterprise } = useMutation({
    mutationFn: (enterpriseId) => await LmsApiService.updateUserActiveEnterprise(enterpriseId);
    onError: (error) => {
      logError(`Could not set active enterprise for username ${username}, failed with error: ${error}`);
    },
  });

  // Determines active enterprise customer for authenticated user
  const { username } = user;
  const {
    data: activeEnterpriseCustomer,
    isLoading: isLoadingActiveEnterprise,
    ...restActiveLinkedCustomer
  } = useQuery({
    queryKey: ['enterprise', 'activeLinkedEnterpriseCustomer', username],
    queryFn: async () => await LmsApiService.getActiveLinkedEnterprise(username),
    onSuccess: (activeLinkedEnterprise) => {
      // Successfully retrieved learners active linked enterprise; if it differs from the viewed enterprise, make the viewed enterprise active.
      if (activeLinkedEnterprise.uuid !== enterpriseId) {
        await mutateAsync(enterpriseId);
      }
    },
    onError: (error) => {
      logError(`Could not fetch active enterprises for username ${username}, failed with error: ${error}`);
    },
  });

  return {
    isLoading: isLoadingActiveEnterprise || isUpdatingActiveEnterprise,
  };

[alex-sheehan-edx]

ahh I see, on successfully querying the active linked org, we use mutateAsync.. ok ok

[alex-sheehan-edx]

we might have to tweak with the deprecation of the callbacks