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

feat(operators): Add reactivate account button #6747

Merged
merged 8 commits into from
Jun 28, 2023

Conversation

abshierjoel
Copy link
Contributor

When an account is canceled and has suspended organization(s), it can be reactivated.

image image

Checklist

Authors and Reviewer(s), please verify the following:

  • A PR description, regardless of the triviality of this change, that communicates the value of this PR
  • Well-formatted conventional commit messages that provide context into the change
  • Documentation updated or issue created (provide link to issue/PR)
  • Signed CLA (if not already signed)
  • Feature flagged, if applicable

@abshierjoel abshierjoel requested a review from a team as a code owner June 28, 2023 14:44
@abshierjoel abshierjoel changed the title Add reactivate account button feat(operators): Add reactivate account button Jun 28, 2023
Copy link
Contributor

@wdoconnell wdoconnell left a comment

Choose a reason for hiding this comment

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

Looks good, but I had a couple minor tweak requests/suggestions.

@@ -52,7 +52,7 @@
"prettier:fix": "pretty-quick --config .prettierrc.json --write '{src,cypress}/**/*.{ts,tsx}'",
"tsc": "tsc -p ./tsconfig.json --noEmit --pretty --skipLibCheck",
"tsc:watch": "yarn tsc --watch",
"generate": "export SHA=993f6756500aebe47903a3ddaee62f9f75d207c1 && export REMOTE=https://raw.githubusercontent.com/influxdata/openapi/${SHA}/ && yarn generate-meta",
"generate": "export SHA=d05381fbcee0dd5d88833e71057a4af647e0d169 && export REMOTE=https://raw.githubusercontent.com/influxdata/openapi/${SHA}/ && yarn generate-meta",
"generate-local": "export REMOTE=../openapi/ && yarn generate-meta",
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this openapi SHA - it looks like the request body is just an object? Could we add a more specific type in openapi? I worry this is going to cause issues (or at min some warnings) with type validation on the UI side if we don't have a schema for the body.

Example:

Screen Shot 2023-06-28 at 10 47 26 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wdoconnell thanks! So what do we usually do whenever there's no request body necessary? IIRC, without a requestBody, OATS won't generate it right. But it's not strictly necessary, so that's why I ended up with object.

Copy link
Contributor

Choose a reason for hiding this comment

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

@abshierjoel Oh, in that case, just ignore what I said 😓 - I had thought this would be relied on. If there's no request body being used I think it's fine to leave it as object.

<ButtonBase
color={ComponentColor.Primary}
shape={ButtonShape.Default}
onClick={() => setReactivateOverlayVisible(!reactivateOverlayVisible)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is the convention elsewhere in this file - but should we be using !reactivateOverlayVisible? I think this is a one-way trip so really hitting the button should always set reactivateOverlayVisible to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I definitely prefer that!

@@ -0,0 +1,82 @@
import React, {FC, useContext} from 'react'
import {
Overlay,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind alpha-ordering these just for readability? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yessir 🫡

@@ -138,23 +153,42 @@ export const AccountProvider: FC<Props> = React.memo(({children}) => {
}
}, [dispatch, history, accountID])

const handleReactivateAccount = useCallback(async () => {
try {
setReactivateStatus(RemoteDataState.Loading)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is reactivateStatus being used? This pattern gets used in our Redux state-managed components to ensure that in useEffect hooks, we only trigger API calls once on page load, not when loading or when there's an error. But I don't see anything in these components that relies on this status.

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 started to remove it, but I instead kept it to disable the reactivate button once they click it. Technically, the operation is idempotent, but it's probably better if they're not smashing it, since it can be a slow operation.

@abshierjoel abshierjoel added this pull request to the merge queue Jun 28, 2023
Merged via the queue into master with commit 3bb839d Jun 28, 2023
1 check passed
@abshierjoel abshierjoel deleted the add-reactivate-account-button branch June 28, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants