Migate Run Validators page to react #10652

danieljames-dj · 2025-01-19T03:40:42Z

This is part of migrating all panel pages to react

gregorbg · 2025-01-20T12:33:29Z

app/controllers/panel_controller.rb

+    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)


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?

I didn't use the result_validation_form.rb because it was having too many things. I agree those were good, but I feel that if I forcefully use that model, it may not be so clean. So just took the above two lines from that.

I just now saw couple of validations from the model which are also relevant, I've now implemented those validations in the client side.

I understand the risk of constantize, I now see how the result_validation_form is doing it - I have now changed to a similar logic, so no more use of constantize.

Also, now I think technically result_validation_form.rb is not needed anymore as it was used to support the rails ERB code, and that code is no longer there. So I've removed result_validation_form.rb after making necessary changes.

app/webpacker/components/Panel/pages/RunValidatorsPage/CompetitionRangeSelector.jsx

gregorbg · 2025-01-20T12:39:21Z

app/webpacker/components/Panel/pages/RunValidatorsPage/api/getCompetitionList.js

+
+export default async function getCompetitionList(startDate, endDate, maxLimit) {
+  const { data } = await fetchJsonOrError(
+    `${apiV0Urls.competitions.listIndex}?start=${startDate}&end=${endDate}&per_page=${maxLimit}`,


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.

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.

I didn't think of the admin_validation_competitions_path actually. But as part of one more cleanup, I actually removed ResultValidationForm, and hence all it's usages which includes admin_validation_competitions_path as well. But I've now created a new API to fetch the count of competitions. Hope that is fine.

app/webpacker/components/Panel/pages/RunValidatorsPage/CompetitionRangeSelector.jsx

app/webpacker/components/Panel/pages/RunValidatorsPage/RunValidatorsForm.jsx

gregorbg · 2025-01-20T13:04:29Z

app/webpacker/components/Panel/pages/RunValidatorsPage/ValidationListView.jsx

+          [
+          <a href={competitionUrl(validationData.competition_id)}>
+            {validationData.competition_id}
+          </a>
+          {'] '}


Why do you need the []? Are they absolutely essential for user communication?

The users are WRT, it will look clean if the competition is added at the beginning of the validation info.

Huh, interesting that the [ above is acceptable but ] is not. Are there HTML codes (similar to &apost;) that you could use?

app/webpacker/components/Panel/pages/RunValidatorsPage/ValidationOutput.jsx

app/webpacker/components/Panel/pages/RunValidatorsPage/CompetitionRangeSelector.jsx

app/webpacker/components/Panel/pages/RunValidatorsPage/ValidationListView.jsx

danieljames-dj

Thanks for the detailed review, @gregorbg. One of the main change that I did post last commit is that I removed result_validation_form. I'm not sure if you are against it, but just to strengthen my argument, I like to highlight that the reason I felt to remove result_validation_form is because this was a helper model for the rails UI, and the rails UI no longer exists, and everything done inside result_validation_form can now be done in 2-3 lines.

danieljames-dj · 2025-01-20T16:02:31Z

app/controllers/panel_controller.rb

+    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)


I didn't use the result_validation_form.rb because it was having too many things. I agree those were good, but I feel that if I forcefully use that model, it may not be so clean. So just took the above two lines from that.

I just now saw couple of validations from the model which are also relevant, I've now implemented those validations in the client side.

I understand the risk of constantize, I now see how the result_validation_form is doing it - I have now changed to a similar logic, so no more use of constantize.

Also, now I think technically result_validation_form.rb is not needed anymore as it was used to support the rails ERB code, and that code is no longer there. So I've removed result_validation_form.rb after making necessary changes.

app/webpacker/components/Panel/pages/RunValidatorsPage/CompetitionRangeSelector.jsx

danieljames-dj · 2025-01-20T16:42:51Z

app/webpacker/components/Panel/pages/RunValidatorsPage/RunValidatorsForm.jsx

+import runValidatorsForCompetitionsInRange from './api/runValidatorsForCompetitionsInRange';
+import ValidationOutput from './ValidationOutput';
+
+const validatorNameReadable = (validatorName) => validatorName.split('::')[1];


I have changed the logic for the validator name, so this is no longer there.

danieljames-dj · 2025-01-20T16:45:13Z

app/webpacker/components/Panel/pages/RunValidatorsPage/ValidationListView.jsx

+          [
+          <a href={competitionUrl(validationData.competition_id)}>
+            {validationData.competition_id}
+          </a>
+          {'] '}


The users are WRT, it will look clean if the competition is added at the beginning of the validation info.

danieljames-dj · 2025-01-26T13:57:29Z

app/webpacker/components/Panel/pages/RunValidatorsPage/RunValidatorsForm.jsx

+    queryKey: ['competitionCountInRange'],
+    queryFn: runValidatorsForCompetitions,


Ya it was a copy-paste, I forgot to change the key. I've anyway changed to useMutation as you suggested, so no longer relevant.

danieljames-dj · 2025-01-26T13:57:36Z

app/webpacker/components/Panel/pages/RunValidatorsPage/RunValidatorsForm.jsx

+  } = useQuery({
+    queryKey: ['competitionCountInRange'],
+    queryFn: runValidatorsForCompetitions,
+    enabled: false,


app/webpacker/components/Panel/pages/RunValidatorsPage/RunValidatorsForm.jsx

app/webpacker/components/Panel/pages/RunValidatorsPage/ValidationOutput.jsx

gregorbg · 2025-02-20T06:01:50Z

app/controllers/api/v0/competitions_controller.rb

+  def competition_count
+    start_date = params.require(:startDate)
+    end_date = params.require(:endDate)
+
+    count = Competition.where("start_date >= ? AND end_date <= ?", start_date, end_date).count
+    render json: { count: count }
+  end


This is a highly specific endpoint for internal uses, I'd rather not have that as part of the public API.

gregorbg · 2025-02-20T06:03:34Z

app/controllers/panel_controller.rb

+    Competition.over
+               .not_cancelled
+               .where(start_date: start_date..)
+               .where(start_date: ..end_date)
+               .order(:start_date)
+               .ids


Can you combine this with the logic for competition_count above?
Like, have some competitions_between(start, end) method which returns Competition.over.not_cancelled.where(...).order(...) and then one endpoint returns competitions_between(foo, bar).count and the other one returns competitions_between(foo, bar).ids

This seems like the perfect use case for an AR scope by the way :)

gregorbg · 2025-02-20T06:04:29Z

app/controllers/panel_controller.rb

+  end
+
+  def validators_for_competitions_in_range
+    range = JSON.parse(params.require(:competitionRange)).transform_keys(&:to_sym)


...Why are you not simply passing in startDate and endDate as params directly?

gregorbg · 2025-02-20T06:05:32Z

app/webpacker/components/Panel/pages/RunValidatorsPage/CompetitionRangeSelector.jsx

+    queryKey: ['competitionCountInRange', range?.startDate, range?.endDate],
+    queryFn: () => getCompetitionCount(range?.startDate, range?.endDate),
+    enabled: bothDatesAreSelected,


If the query is only enabled when both dates are selected, then you should be able to drop the ? at least from the queryFn

gregorbg · 2025-02-20T06:05:57Z

app/webpacker/components/Panel/pages/RunValidatorsPage/CompetitionRangeSelector.jsx

+
+const RUN_VALIDATORS_QUERY_CLIENT = new QueryClient();
+
+export default function CompetitionRangeSelector({ range, setRange }) {


Why are you passing in range instead of just passing the dates separately?

gregorbg · 2025-02-20T06:16:22Z

app/webpacker/components/Panel/pages/RunValidatorsPage/ValidationListView.jsx

+          [
+          <a href={competitionUrl(validationData.competition_id)}>
+            {validationData.competition_id}
+          </a>
+          {'] '}


Huh, interesting that the [ above is acceptable but ] is not. Are there HTML codes (similar to &apost;) that you could use?

gregorbg · 2025-02-20T06:16:46Z

app/webpacker/components/Panel/pages/RunValidatorsPage/ValidationOutput.jsx

+  if (!validationOutput) {
+    return (
+      <Message>Please run the validators to see the output.</Message>
+    );
+  }


What if the validators have been run but still didn't give any output?

gregorbg · 2025-02-20T06:18:29Z

app/webpacker/components/Panel/pages/RunValidatorsPage/ValidationListView.jsx

+  return (
+    <>
+      {Object.entries(listByGroup).map(([group, list]) => (
+        <Fragment key={group}>


Instead of Fragment, you could consider wrapping this in a Message. SemUI Messages have info, warning and error states with nice color codings :)

gregorbg · 2025-02-20T06:19:48Z

app/webpacker/components/Panel/pages/RunValidatorsPage/ValidationOutput.jsx

+function ValidationErrorOutput({ validationOutput, showCompetitionNameOnOutput }) {
+  const hasResults = validationOutput.has_results;
+  const hasErrors = validationOutput.errors.length > 0;
+
+  if (!hasErrors) {
+    if (hasResults) {
+      return <p>No error detected in the results.</p>;
+    }
+    return <p>No results for the competition yet.</p>;
+  }
+
+  return (
+    <>
+      <p>Please fix the errors below:</p>
+      <ValidationListView
+        validations={validationOutput.errors}
+        showCompetitionNameOnOutput={showCompetitionNameOnOutput}
+        type="error"
+      />
+    </>
+  );
+}
+
+function ValidationWarningOutput({ validationOutput, showCompetitionNameOnOutput }) {
+  const hasResults = validationOutput.has_results;
+  const hasWarning = validationOutput.warnings.length > 0;
+
+  if (!hasWarning) {
+    if (hasResults) {
+      return <p>No warning detected in the results.</p>;
+    }
+    return <p>No results for the competition yet.</p>;
+  }
+
+  return (
+    <>
+      <p>Please pay attention to the warnings below:</p>
+      <ValidationListView
+        validations={validationOutput.warnings}
+        showCompetitionNameOnOutput={showCompetitionNameOnOutput}
+        type="warning"
+      />
+    </>
+  );
+}


Quite a lot of repetition, any chance to unify this?
You could consider changing the English texts a little, like No messages of type ${type} detected in the results, to make reusing the components a bit easier

gregorbg · 2025-02-20T06:20:38Z

app/webpacker/components/Panel/pages/RunValidatorsPage/api/getCompetitionCount.js

+
+export default async function getCompetitionCount(startDate, endDate) {
+  const { data } = await fetchJsonOrError(viewUrls.competitions.countInRange(startDate, endDate));
+  return data?.count;


Hm, now you're accessing the data already again. If only the count is so important, then why not just return it directly from the backend?
(Yes, numbers by themselves are valid JSON. Not everything has to be {} or [] on top level)

github-actions bot assigned danieljames-dj Jan 19, 2025

danieljames-dj force-pushed the run-validators branch from 900e926 to f0412cb Compare January 19, 2025 03:43

gregorbg requested changes Jan 20, 2025

View reviewed changes

kr-matthews reviewed Jan 21, 2025

View reviewed changes

danieljames-dj commented Jan 26, 2025

View reviewed changes

danieljames-dj force-pushed the run-validators branch from 6553fc2 to 0468c2f Compare January 26, 2025 14:18

danieljames-dj mentioned this pull request Jan 28, 2025

Migrate Create newcomers entry page to react #10732

Merged

danieljames-dj added 5 commits February 12, 2025 10:37

Migate Run Validators page to react

a0d6500

Review changes

6833d58

Review changes

ecfe948

Rebase

0b4bd6c

rubocop fix

5f50197

danieljames-dj force-pushed the run-validators branch from 84f85c7 to 5f50197 Compare February 12, 2025 05:08

gregorbg requested changes Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migate Run Validators page to react #10652

Migate Run Validators page to react #10652

danieljames-dj commented Jan 19, 2025

gregorbg Jan 20, 2025

danieljames-dj Jan 20, 2025

gregorbg Jan 20, 2025

gregorbg Jan 20, 2025

danieljames-dj Jan 26, 2025

gregorbg Jan 20, 2025

danieljames-dj Jan 20, 2025

gregorbg Feb 20, 2025

danieljames-dj left a comment

danieljames-dj Jan 20, 2025

danieljames-dj Jan 20, 2025

danieljames-dj Jan 20, 2025

danieljames-dj Jan 26, 2025

danieljames-dj Jan 26, 2025

gregorbg Feb 20, 2025

gregorbg Feb 20, 2025

gregorbg Feb 20, 2025

gregorbg Feb 20, 2025

gregorbg Feb 20, 2025

gregorbg Feb 20, 2025

gregorbg Feb 20, 2025

gregorbg Feb 20, 2025

gregorbg Feb 20, 2025

gregorbg Feb 20, 2025

gregorbg Feb 20, 2025

		queryKey: ['competitionCountInRange'],
		queryFn: runValidatorsForCompetitions,


		const RUN_VALIDATORS_QUERY_CLIENT = new QueryClient();

		export default function CompetitionRangeSelector({ range, setRange }) {

Migate Run Validators page to react #10652

Are you sure you want to change the base?

Migate Run Validators page to react #10652

Conversation

danieljames-dj commented Jan 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieljames-dj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment