Migate Run Validators page to react #10652
Conversation
danieljames-dj
force-pushed
the
run-validators
branch
from
900e926 to
f0412cb
Compare
| validators = params.require(:selectedValidators).split(',').map(&:constantize) | ||
| apply_fix_when_possible = params.require(:applyFixWhenPossible) | ||
|
|
||
| results_validator = ResultsValidators::CompetitionsResultsValidator.new( | ||
| validators, | ||
| check_real_results: true, | ||
| apply_fixes: apply_fix_when_possible, | ||
| ) | ||
| results_validator.validate(competition_ids) |
There was a problem hiding this comment.
As useful as constantize is, you gotta be careful with when and how you use it because it can potentially lead to vulnerabilities. In this context, I'm wondering: Why didn't you use the existing result_validation_form.rb that is also used in the previous running_validators hook?
| const { | ||
| data: competitionList, isLoading, isError, refetch, | ||
| } = useQuery({ | ||
| queryKey: ['competitionCountInRange'], |
There was a problem hiding this comment.
The startDate and endDate need to be included in the queryKey. Otherwise, the query will never be refetched when the dates change, and the old number will persist.
There was a problem hiding this comment.
For your basic understanding, you can think of queryKey as a cache key that controls the "cache" of the network traffic, to make sure that the data is only refetched when needed. (The real implementation is a bit more complex than that, but as a rule of thumb the cache comparison works.)
|
|
||
| export default async function getCompetitionList(startDate, endDate, maxLimit) { | ||
| const { data } = await fetchJsonOrError( | ||
| `${apiV0Urls.competitions.listIndex}?start=${startDate}&end=${endDate}&per_page=${maxLimit}`, |
There was a problem hiding this comment.
Why are you not using the admin_validation_competitions_path URL from the old jQuery form?
Don't get me wrong, I would love it if we can switch to (semi-)public APIs, but I just really want to make sure they return the same results.
There was a problem hiding this comment.
One advantage of a custom path would be that you can serialize only the names, and avoid sending (or even loading from the DB) any unnecessary extra data.
| useEffect(() => { | ||
| setRange({ startDate, endDate }); | ||
| if (enableCompetitionListFetch) { | ||
| refetch(); | ||
| } | ||
| }, [startDate, endDate, setRange, enableCompetitionListFetch, refetch]); |
There was a problem hiding this comment.
This hook definitely needs to go. react-query should only be refetched in rare circumstances, but right here is a basic use-case which should be fixed by adjusting the query key as noted in my comment above.
| <div> | ||
| {`The checks will run for ${competitionList.length}${ | ||
| competitionList.length >= MAX_COMPETITIONS_PER_QUERY ? '+' : '' | ||
| } competitions`} | ||
| </div> |
There was a problem hiding this comment.
Do you really need this wrapping div? In case of doubt, a <> fragment should do
|
|
||
| return ( | ||
| <> | ||
| <Form onSubmit={runValidators}> |
There was a problem hiding this comment.
Ah! I think what you want are React-Query Mutations: https://tanstack.com/query/latest/docs/framework/react/guides/mutations
There was a problem hiding this comment.
(This also explains why you hard-coded enabled: false above, which definitely still is an anti-pattern.)
| <Form.Field | ||
| label="Competition ID(s)" | ||
| control={IdWcaSearch} | ||
| name="competitionIds" | ||
| value={selectedCompetitionIds} | ||
| onChange={setSelectedCompetitionIds} | ||
| model={SEARCH_MODELS.competition} | ||
| multiple | ||
| required | ||
| /> |
There was a problem hiding this comment.
Indentation looks a little bit off here? Might just be the GitHub diff though.
| label={`Apply fix when possible (List of validators with automated fix: ${ | ||
| VALIDATORS_WITH_FIX.map(validatorNameReadable).join(', ') | ||
| })`} |
There was a problem hiding this comment.
Bit awkward to have such a (potentially) long list as part of the checkbox label. Can you add them as an extra text paragraph below?
There was a problem hiding this comment.
SemUI Header as h4 or h5 with only the subheader and no "real" header should do: https://react.semantic-ui.com/elements/header/#content-subheader
| [ | ||
| <a href={competitionUrl(validationData.competition_id)}> | ||
| {validationData.competition_id} | ||
| </a> | ||
| {'] '} |
There was a problem hiding this comment.
Why do you need the []? Are they absolutely essential for user communication?
| if (isFetching) return <Loading />; | ||
| if (isError) return <Errored />; | ||
|
|
||
| if (!isFetching && !isError && !validationOutput) { |
There was a problem hiding this comment.
At this point, you already know that !isFetching && !isError will always be true. Otherwise, the code would have returned already. So just checking !validationOutput should be enough.
| const [startDate, setStartDate] = useState(range?.startDate); | ||
| const [endDate, setEndDate] = useState(range?.endDate); | ||
|
|
||
| const enableCompetitionListFetch = Boolean(startDate && endDate); |
There was a problem hiding this comment.
I'm strongly in favour of naming things based on what they are, not how they are used.
| const enableCompetitionListFetch = Boolean(startDate && endDate); | |
| const bothDatesAreSelected = Boolean(startDate && endDate); |
| const [startDate, setStartDate] = useState(range?.startDate); | ||
| const [endDate, setEndDate] = useState(range?.endDate); |
There was a problem hiding this comment.
Why do we have these states here in the component? If we're receiving range and setRange then the parent should be updating range when setRange is called, and const startDate = range?.startDate will work.
| isoDate={endDate} | ||
| onChange={setEndDate} | ||
| /> | ||
| {enableCompetitionListFetch && ( |
There was a problem hiding this comment.
Is this required? I think removing it and have competitionList && ... for the third case would be better, assuming it works.
| return ( | ||
| <> | ||
| {Object.entries(listByGroup).map(([group, list]) => ( | ||
| <> |
There was a problem hiding this comment.
I'm pretty sure the console will be warning you that you need a key here.
| <Header as="h5">{`${headingPrefixForType(type)} ${group}`}</Header> | ||
| <List bulleted> | ||
| {list.map((validationData) => ( | ||
| <ListItem> |
This is part of migrating all panel pages to react