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

Add extra validations for links, trackers; and news #385

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Oumph
Copy link
Contributor

@Oumph Oumph commented Mar 10, 2024

  • bookmarks: unique title, unique URL, force lang definition
  • news (direct to moderation): force section definition, force lang definition on links
  • news (in redaction): force lang definition on links
  • trackers: for category definition

One related entry at least: https://linuxfr.org/suivi/liens-verifier-que-l-url-n-a-pas-deja-ete-soumise

@Oumph Oumph requested a review from Trim March 10, 2024 12:11
@Oumph Oumph self-assigned this Mar 10, 2024
- bookmarks: unique title, unique URL, force lang definition
- news (direct to moderation): force section definition, force lang definition on links,
  different messages for max length of news title and link title
- news (in redaction): force lang definition on links,
  different messages for max length of news title and link title
- trackers: for category definition
Copy link
Member

@Trim Trim left a comment

Choose a reason for hiding this comment

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

I didn't have time to try the code on my development environment, but it looks good 👍

@@ -44,6 +45,11 @@ def url=(raw)
write_attribute :url, raw
end

def lang_validation
if lang == "xx"
Copy link
Member

Choose a reason for hiding this comment

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

Il faudrait aussi tester si la valeur est une string vide (c'est possible en ouvrant la console du navigateur et en supprimant l'attribut required du <input>).

En fait, la validation devrait vérifier si la valeur appartient aux langues possibles et refuser si ce n'est pas le cas où si c'est la langue xx.

Copy link
Member

Choose a reason for hiding this comment

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

Pour la langue, malheureusement, c'est plus compliqué à définir l'association que pour la section, parce qu'on ne peut pas utiliser validates_associated_to: en base de donnée, la langue est juste une string de 2 caractères.

Je n'ai pas compris pourquoi les langues ne sont définies que dans la base Redis et non pas dans la base mysql, c'est dommage :/

Copy link
Member

Choose a reason for hiding this comment

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

Ah, c'est bon, j'ai trouvé sur StackOverflow une solution théorique avec validates_inclusion_of: https://stackoverflow.com/a/4730085

L'idée serait de tester directement dans le modèle de Bookmark, l'inclusion dans la liste générée par Lang sans la valeur "xx".

Copy link
Member

Choose a reason for hiding this comment

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

Ah, c'est bon, j'ai trouvé sur StackOverflow une solution théorique avec validates_inclusion_of: https://stackoverflow.com/a/4730085

C'est fait et j'ai ajouté le test dans models/link et dans models/bookmark pour les liens des dépêches et la section lien.

@@ -16,7 +16,7 @@
= form.text_field :title, autocomplete: 'off', required: 'required', spellcheck: 'true', maxlength: 100
%p
= form.label :section_id, "Section de la dépêche"
= form.collection_select :section_id, Section.published, :id, :title
= form.collection_select :section_id, Section.published, :id, :title, { include_blank: true }, { required: "required" }
Copy link
Member

Choose a reason for hiding this comment

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

On a aussi besoin d'ajouter la même validation pour la section dans ce controlleur.

Si je procède à la même manipulation (enlever l'attribut required), alors, j'ai cette erreur qui apparaît:

image

Ça serait plus sympa d'avoir un bon message d'erreur du controlleur pour éviter de perdre le contenu de la dépêche soumise sans espace de rédaction :)

Copy link
Member

Choose a reason for hiding this comment

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

Théoriquement, pour valider une association à une table (donc, pour la section de news ici), on peut utiliser les propriétés: validates_associated (validation de l'appartenance à la liste) et validates_presence_of (validation de la présence de la valeur) ensemble.

Copy link
Member

Choose a reason for hiding this comment

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

J'ai ajouté "validates_associated" et j'ai changé le système de "prévisualisation" pour ne prévisualiser que si le contenu est valide.

La capture d'écran d'erreur que j'ai posté plus haut était due au fait que l'on essaie de faire la preview d'une news sans section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On peut envoyer en Anonyme en modération une dépêche avec un lien sans langue définie.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On peut injecter des langues inconnues sur un lien de dépêche en Anonyme (et ensuite le site essaie d'afficher une image inexistante).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En rédaction, on peut générer des 500 en envoyant des identifiants de section invalide pour la dépêche en cours.

@@ -5,7 +5,7 @@
= form.text_field :title, autocomplete: 'off', required: 'required', spellcheck: 'true', maxlength: 100
%p
= form.label :category_id, "Catégorie"
= form.collection_select :category_id, Category.all, :id, :title
= form.collection_select :category_id, Category.all, :id, :title, { include_blank: true }, { required: "required" }
Copy link
Member

Choose a reason for hiding this comment

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

Je n'ai pas eu le temps de testé, mais j'imagine qu'il y aura le même problème que pour la section de news.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Encore un 500 si on soumet avec un id de catégorie inconnu.

ActiveRecord::InvalidForeignKey (Mysql2::Error: Cannot add or update a child row: a foreign key constraint fails (linux fr.trackers, CONSTRAINT fk_trackers_on_category_id FOREIGN KEY (category_id) REFERENCES categori es (id)): INSERT INTO trackers (...)

As title and link are both mandatory, we set the lang mandatory too.

The "xx" language code is removed from the valid list and blank is
refused.

The "xx" value was used as default value before we enable "blank" in
form. It is not really useful now except for links and bookmarks which
are already using this value.
This way, the errors is easier to see at they appear first on screen.
This shows a user friendly message instead of an exception if user
remove the client side validation of section.

The exception were thrown by the preview, so we avoid to show preview
and submit button for invalid news.
The bookmark attribute for URL is "link" and not "url".
The inclusion validator already does the job correctly.
@Trim
Copy link
Member

Trim commented Apr 11, 2024

Voilà, j'ai essayé de faire en sorte que toutes les prévisualisations ne soient affichées que si le formulaire est valide. Comme ça, les erreurs sont affichées au sommet de l'écran et sont bien visibles.

Le bouton "Enregistrer" ne s'affiche aussi que si le formulaire est valide au moment de la prévisualisation.

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