fix: loading circle + tests

[ilee2u]

This didn't work on stage when I smoke tested it, so I've fixed it up with this PR.

Screen.Recording.2024-01-11.at.2.20.12.PM.mov

[codecov-commenter]

Codecov Report

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

Comparison is base (9ce2fce) 75.75% compared to head (75159c7) 76.97%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
src/data/redux/requests/selectors.js	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
+ Coverage   75.75%   76.97%   +1.21%     
==========================================
  Files          22       22              
  Lines         297      317      +20     
  Branches       31       34       +3     
==========================================
+ Hits          225      244      +19     
- Misses         69       70       +1     
  Partials        3        3              

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

[MichaelRoytman]

Nit: Did you mean to delete this line?

[ilee2u]

No :(. I put it back :)

[MichaelRoytman]

Nice find!

I see how this fixes the bug, but I'm wondering about this as a solution. This new hook returns a function that, when called with a particular requestKey gets a status from the Redux store for that request.

What benefit do you get from wrapping your hook this way; in other words, what benefit do you get from returning a function and not the actual value? There's another way to use your original hook that doesn't require returning a function (i.e. this change), and I wonder if that would be a clearer way to get the data you need.

[ilee2u]

I honestly did this because I would otherwise get an error saying that the hook needed to return a function and not the actual value, mostly due to my naivety with Redux. If there's a better way then I'd love to know it!

[MichaelRoytman]

That makes sense!

Do you still have the error? I'd be curious to see it.

Without these changes, your hook returned the status of a request based on the requestKey argument, right? So, something like this would give you the status of the delete exam attempt request.

const status = useButtonStateFromRequestStatus(constants.RequestKeys.deleteExamAttempt))

With your current changes, you'd need to do something like this.

const getRequestStatus = useButtonStateFromRequestStatus()
const status = getRequestStatus(constants.RequestKeys.deleteExamAttempt))

Right now, the process of getting the status and passing it to the StatefulButton is sort of like this...

1. call the useButtonStateFromRequestStatus hook to get a function that will get the request status
2. call the function to get the request status
3. pass the result as a state prop to the StatefulButton

Steps 2 and 3 are happening in the same line, but they're separate steps.

I'm wondering why you now need step 1 if your original hook already did step 2 and gave you the request status value. Couldn't you proceed onto step 3? I think taking a look at how step 3 works would be a good idea. I can see why, as written, you'd need to wrap the hook in a function, but do you need to have a function call in step 3?

Taking a look at the use of useToggle could be helpful here.

[ilee2u]

Ok so I tried reproducing the error and got a different error than I recall. npm lint spits out this error:

  136:26  error  React Hook "useButtonStateFromRequestStatus" is called conditionally. React Hooks must be called in the exact same order in every component render  react-hooks/rules-of-hooks

So it seems like not doing the whole "returning the function" step causes us to break the rules of hooks, since the hook itself is being called within a conditional statement.

[MichaelRoytman]

By reproducing this error, I assume that you did the by reverting back to the current code on main?

Where is the conditional statement? I don't see that hook being called conditionally.

[ilee2u]

I thought the "error" we're talking about is the one that would occur when the useButtonStateFromRequestStatus hook just returns the actual value (i.e. 'pending', 'error', or '') instead of a function. The code on main currently also returns a function, not the actual value. I did, however, play with just returning the actual value in my last PR, and that is what I was trying to recreate.

Also I believe the conditional statement is this ternary operator here which is used to determine whether each button should be visible:

           {attemptStatus !== constants.ExamAttemptStatus.verified
              && (
                <StatefulButton
                  // The state of this button is updated based on the request status of the modifyExamAttempt
                  // api function. The change of the button's label is based on VerifyButtonProps.
                  state={useButtonStateFromRequestStatus(constants.RequestKeys.modifyExamAttempt)}
                  {...VerifyButtonProps}
                  variant="success"
                  onClick={async e => { // eslint-disable-line no-unused-vars
                    modifyExamAttempt(attemptId, constants.ExamAttemptActions.verify);
                  }}
                />
              )}

[MichaelRoytman]

Okay, I see. Where is that code coming from? I don't see it either on main or on your branch. Do you have some uncommitted code? The code that's on main shouldn't be throwing that error, because there's no conditional calling of the hook.

Here's what I was thinking.

You'd leave your hook with the original implementation, which is currently on main.

export const useButtonStateFromRequestStatus = (requestKey) => {
  const isPending = reduxHooks.useRequestIsPending(requestKey);
  const isError = reduxHooks.useRequestError(requestKey);
  return () => {
    if (isPending) {
      return 'pending';
    } if (isError) {
      return 'error';
    }
    return '';
  };
};

This hook will return the request status of a particular request.

In src/pages/ExamsPage/components/ReviewExamAttemptModal.jsx, your use of the hook would be the following.

const modifyExamAttemptRequestStatus = useButtonStateFromRequestStatus(constants.RequestKeys.modifyExamAttempt));

Now, you have the request status value and you can use it as the prop value for the StatefulButton. For example, for the verified button, you'd have the following.

<StatefulButton
  // The state of this button is updated based on the request status of the modifyExamAttempt
  // api function. The change of the button's label is based on VerifyButtonProps.
  state={modifyExamAttemptRequestStatus}
  {...VerifyButtonProps}
  variant="success"
  onClick={async e => { // eslint-disable-line no-unused-vars
    modifyExamAttempt(attemptId, constants.ExamAttemptActions.verify);
  }}
/>

The hook no longer needs to return a function that has to be called by the component, and the hook is not called conditionally, so the error you posted above should not be raised.

Does that make sense? useToggle works similarly.

[ilee2u]

Got it, I made changes similar to this and just pushed them

[MichaelRoytman]

🎉