Migrate anonymization script to react #10654
Conversation
...ponents/Tickets/TicketWorkbenches/AnonymizationTicketWorkbenchForWrt/PerformManualChecks.jsx
Outdated
Show resolved
Hide resolved
...ponents/Tickets/TicketWorkbenches/AnonymizationTicketWorkbenchForWrt/PerformManualChecks.jsx
Outdated
Show resolved
Hide resolved
app/webpacker/components/Panel/pages/AnonymizationScriptPage/index.jsx
Outdated
Show resolved
Hide resolved
app/webpacker/components/Tickets/TicketWorkbenches/AnonymizationTicketWorkbenchForWrt/index.jsx
Outdated
Show resolved
Hide resolved
danieljames-dj
force-pushed
the
anonym-script
branch
2 times, most recently
from
854423d to
5b3fe8c
Compare
danieljames-dj
force-pushed
the
anonym-script
branch
4 times, most recently
from
db318a9 to
9170f04
Compare
danieljames-dj
force-pushed
the
anonym-script
branch
from
9170f04 to
816ed5c
Compare
| const MODEL_NAME = { | ||
| [SEARCH_MODELS.user]: 'User', | ||
| [SEARCH_MODELS.person]: 'Person', | ||
| }; |
There was a problem hiding this comment.
What about using the keys of this object as a separate AVAILABLE_MODELS const (name tentative)?
Would be useful in a few places further down
|
|
||
| export default function AnonymizationScriptPage() { | ||
| const [user, setUser] = useInputState(); | ||
| const [model, setModel] = useInputState(SEARCH_MODELS.user); |
There was a problem hiding this comment.
If you have a constant list, you can just use AVAILABLE_MODELS[0] here
| model={SEARCH_MODELS.user} | ||
| <FormGroup grouped> | ||
| <div>Select where to search for</div> | ||
| {[SEARCH_MODELS.user, SEARCH_MODELS.person].map((searchModel, index) => ( |
There was a problem hiding this comment.
Here is another place where a list of models available for anon would help.
| {model === SEARCH_MODELS.user && ( | ||
| <AnonymizationTicketWorkbenchForWrt | ||
| userId={searchInput} | ||
| /> | ||
| )} | ||
| {model === SEARCH_MODELS.person && ( | ||
| <AnonymizationTicketWorkbenchForWrt | ||
| wcaId={searchInput} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
| {model === SEARCH_MODELS.user && ( | |
| <AnonymizationTicketWorkbenchForWrt | |
| userId={searchInput} | |
| /> | |
| )} | |
| {model === SEARCH_MODELS.person && ( | |
| <AnonymizationTicketWorkbenchForWrt | |
| wcaId={searchInput} | |
| /> | |
| )} | |
| <AnonymizationTicketWorkbenchForWrt | |
| userId={model === SEARCH_MODELS.user && searchInput} | |
| wcaId={model === SEARCH_MODELS.person && searchInput} | |
| /> |
There was a problem hiding this comment.
(You will need to try out whether that works as expected, though)
| const [confirmOpen, setConfirmOpen] = useState(false); | ||
| const [completed, setCompleted] = useState(false); | ||
|
|
||
| const { save, saving } = useSaveAction(); |
There was a problem hiding this comment.
I think you can use Tanstack mutation here
| import { List, ListItem } from 'semantic-ui-react'; | ||
|
|
||
| export default function PerformManualChecks() { | ||
| // TODO: Add necessary links in this page. |
There was a problem hiding this comment.
Is this TODO supposed to be here? Or did you just forget to take care of it before opening the PR?
| const anonymizationType = (data) => { | ||
| if (data.user_details && !data.person_details) { | ||
| return 'Account Only'; | ||
| } if (data.person_details && !data.user_details) { | ||
| return 'Profile Only'; | ||
| } if (data.user_details && data.person_details) { | ||
| return 'Account & Profile'; | ||
| } | ||
| return 'Unknown'; | ||
| }; |
There was a problem hiding this comment.
Just a minor comment, because it's not my power to decide: I am confused about the terminology here. In the entry point to the script (further up, where you have a search input) you call it "User" and "Person". Here, you suddenly talk about "Profile" and "Account". Maybe this PR is a good chance to clean up some terminology?
| } = useQuery({ | ||
| queryKey: ['anonymizeDetails', userId, wcaId], | ||
| queryFn: () => getDetailsBeforeAnonymization(userId, wcaId), | ||
| enabled: Boolean(userId || wcaId), |
There was a problem hiding this comment.
I think userId || wcaId is already enough in this case, because of the || operator
| <Container> | ||
| <Header>Anonymization Dashboard</Header> | ||
|
|
||
| <Header as="h5">Step 1: Verify details to be anonymized</Header> | ||
| <VerifyAnonymizeDetails data={data} /> | ||
|
|
||
| <Header as="h5">Step 2: Review system generated checks</Header> | ||
| <ReviewSystemGeneratedChecks data={data} /> | ||
|
|
||
| <Header as="h5">Step 3: Perform manual checks</Header> | ||
| <PerformManualChecks data={data} /> | ||
|
|
||
| <Header as="h5">Step 4: Anonymize Action</Header> | ||
| <AnonymizeAction data={data} userId={userId} wcaId={wcaId} /> | ||
|
|
||
| </Container> |
There was a problem hiding this comment.
Not sure if this is something that you would be interested in, but SemUI has this lovely Step component which would improve UX here a lot: https://react.semantic-ui.com/elements/step/
There was a problem hiding this comment.
(I do appreciate that this is a WRT-only panel though, which means only ~0.005% of website users end up here. So I won't block this PR just because of this. But may still be useful though!)
There was a problem hiding this comment.
Can you please give me a short summary of where these methods come from, and how much (if anything) was copy/pasted from the previous anon logic?
I can see that you deleted the existing model file that held all the backend steps logic, but I don't want to compare exactly each line from the old file and this file, especially since it seems that you have changed the flow a little bit
Currently there are two pages of anonymization - one for anonymizing profile (in rails) and another for anonymizing account (in react). This PR merges both to a single react script.