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.

Sign up for GitHub

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

Jump to bottom

Fix: CAU validation fixes #106

Merged
merged 15 commits into from
Oct 29, 2024
Merged

Fix: CAU validation fixes #106

merged 15 commits into from
Oct 29, 2024

Conversation

vokimon
Copy link
Member

@vokimon vokimon commented Oct 24, 2024 •
edited
Loading

Description

Fix some regressions on CAU validation and ApiValidatedFields (IBAN, CadastralRef) introduced in my last PR, and reverting some old features (pretty printing, initial message, progress indicators) that had been disabled because similar cases, now reviewd and fixed.

Changes

  • Fix: Avoided validation infinite loop on ApiValidatedFields because i remote validated the pretty version of the value
  • Fix: Initial CAU validated message "where to find the CAU code" was substituted by "CAU required" messsage. Now the later is shown only once the field is visited.
  • Fix: "Checking..." message on checking restored
  • Fix: Reverted splitted (pretty) display for ApiValidatedFields (CAU, IBAN, Cadastral) for an easier the review by the user.

Checklist

Justify any unchecked point:

  • Changed code is covered by tests.
  • Relevant changes are explained in the "Unreleased" section of the CHANGES.md file.
  • That section includes "Upgrade notes" with any config, dependency or deploy tweek needed on development and server setups.
  • Changes on the setup process (development, testing, production) have been updated in the proper documentation

Observations

  • It looks like that pretty format caused infinite loops in the past when combined with Formik validation. It was disabled but pretty format was still living inside the widget and i struggled with that.
  • The solution to enable it again and avoiding loops was to define two realms, inside the widget pretty format is the one. But compact format is the one received from and sent to the outside world.
  • Pretty view for CUPS is still disabled because is not a ApiValidatedField yet. A future fix could be applying the fixes also to CUPS. Current change affects to IBAN, CAU and Cadastral Reference.
  • Known old glitches (present in old versions and not fixed):
    • If you edit the field while an external validation is in progress, when the external validation ends it reverts the edit. We should apply query abortion here on every edit similar to the ones proposed here.
    • Editing in the middle of the value moves the cursor to the end when the pretty string changes too much, making editing in the middle a pain.
    • Current recheck when the individual/collective field changes, still does an extra render validation cycle. Because it is done in an useEffect it takes an extra render to have it updated. I couldn't find a better/simpler solution thought. Being an inter field interaction, maybe the check should be moved up, but this isn't an direct change, for the (not so) quick fix that it was needed here.

Please, review

  • CAU validation, but also IBAN and Cadastral Reference
    • Messages are consistent as you edit the fields
  • I guess that the changes i reverted might have other motivations than that. It works for me, but, please review if old bugs have not reverted.

How to check the new features

You could deploy locally or use testing since i deployed it in there.

Deploy notes

@vercel Vercel
Copy link

vercel bot commented Oct 24, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
webforms-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 29, 2024 10:35am

@vokimon vokimon marked this pull request as ready for review October 25, 2024 09:20
@vokimon
⚡️ Bye Google Analytics
b900939 
Don't track users with GA if we are using Plausible.
Besides 1% 10K of gzip payload reduced.
@vokimon
📝 changes
5a6e2d4
@vokimon
Merge branch 'IMP_bye_google_analytics'
cdc7d58
@vokimon
🐛 warning: no component body1
0edc148 
indeed body1 variant was too big used proper variant
@vokimon
♻️ extracted CAU inputFilter as prettyCAU in utils
f56e5aa
@vokimon
🐛 API validated: when validating issuing such 'error'
a5c0940
@vokimon
🐛 ApiValidated: descending pretty version
13cd751 
Even though we bubble up compact version,
if we push down pretty version, the input
will understand it as a change of the value,
and will repeat the check. In some cases
even creating an infinite loop.
@vokimon
🐛 removed warning, error is boolean not string
812846d
@vokimon
🐛 the validation progress and check didn't appear
8dd2034
@vokimon
🐛 discriminate 'required' and 'initial' message
6a0f258 
Initial message (explaining where to find the CAU)
was not shown initially, just the required error
'CAU must be filled'.
Now the required error is shown only after
the field has been visited.
@vokimon
🐛 remote checking compact and not pretty CAU
bdb63f9 
This was the root of the remaining validation infinite loop.
The validator took the pretty value and so formik sent it
back to the input which was detected as different and
thus validated again.

Previous fixes just avoided this on other cases.
@vokimon
♻️ explicitly naming for compact or pretty values
e5aaa5c 
This might avoid falling again confusions and errors.
They are not all the same value. What we receive from
and send to the outside world is a compact value,
what we edit in the widget is a pretty value
(with spaces and decoration).

(pretty and compact is python stdnum library nomenclature)
@vokimon
📝 ApiValidatedField: documented known bugs
517ea35
@vokimon
📝 changes
2e52f64
@vokimon
🐛 fix variable misspel
39d08dc
@vokimon vokimon merged commit a912b56 into master Oct 29, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dukenuken09 dukenuken09 Awaiting requested review from dukenuken09

@marta197 marta197 Awaiting requested review from marta197

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@vokimon