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

ETQ usager: je ne peux ajouter qu'une unique PJ pour les anciennes procédures qui l'exigent #10511

Merged
merged 7 commits into from
Jul 8, 2024

Conversation

Benoit-MINT
Copy link
Contributor

@Benoit-MINT Benoit-MINT commented Jun 11, 2024

En réponse à l'issue #10186

Ceci concerne donc que les anciennes démarches où l'admin pouvait indiquer si une ou plusieurs PJ étaient autorisées.

Le pb venait de l'association entre le for dans la balise du label et l'id de l'input associé, qui rendait donc cliquable le label, permettant ainsi à l'usager de sélectionner une nouvelle PJ.

La modification proposée introduit une exception pour le type de champ PJ et lorsque les PJ multiples ne sont pas autorisées, avec pour conséquence de ne plus forcer la valeur de l'attribut for.

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.65%. Comparing base (f954d07) to head (ebea9e3).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10511   +/-   ##
=======================================
  Coverage   80.65%   80.65%           
=======================================
  Files        1235     1235           
  Lines       26204    26208    +4     
  Branches     4715     4717    +2     
=======================================
+ Hits        21134    21138    +4     
  Misses       5070     5070           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Benoit-MINT Benoit-MINT marked this pull request as ready for review June 11, 2024 16:41
@colinux
Copy link
Member

colinux commented Jun 14, 2024

Enlever l'attribut for, ça ne va pas le faire niveau accessibilité. Et l'importance de l'accessibilité est supérieure à l'impact de ce bug (on a eu qu'une remontée ou 2), sachant que ça ne concerne que d'anciennes démarches (publiées avant décembre 2022). Donc à moins de trouver une autre façon de le corriger (et pour pas trop "cher" vu le faible enjeu), je préfère que ça reste comme c'est

@Benoit-MINT
Copy link
Contributor Author

L'autre logique proposée pour ne pas dégrader l'accessibilité est de jouer avec un attribut "disabled" sur l'input.

Pour info ici, contrairement à mon premier commentaire, ce bug se retrouve également pour les démarches actuelles (PJ multiples), en permettant d'insérer plus de 10 PJ (nb fixé arbitrairement dans le code).

Copy link
Member

@colinux colinux left a comment

Choose a reason for hiding this comment

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

déso je vois encore un point bloquant, cf commentaire. Dispo si nécessaire

@@ -21,7 +21,7 @@ def destroy
@attachment.purge_later
flash.notice = 'La pièce jointe a bien été supprimée.'

@champ_id = params[:champ_id]
@champ = Champ.find(params[:champ]) if params[:champ]
Copy link
Member

@colinux colinux Jul 1, 2024

Choose a reason for hiding this comment

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

J'ai l'impression que ça pose un pb de sécu car on render directement le champ à partir de son id. C'est à dire que si j'injecte dans l'url un champ id au hasard et qu'il existe, l'attaquant verra les attachments déjà mis car on controlle pas que le champ est "visible" par l'user.

Là à chaud je me dis que dans le destroy path, on pourrait passer un couple dossier_id/champ stable_id
comme ce qui est fait dans app/controllers/champs/champ_controller.rb#find_champ : le policy scope sur le dossier s'assure que l'user a le droit de voir le dossier, et on récupère un champ par son stable id appartenant à ce dossier. A voir aussi le cas dans un bloc répétable, il doit falloir aussi passer le row_id lorsque c'est présent.
Donc pour résumer: le champ s'identifie avec dossier_id + champ stable_id + row_id le cas échéant.

@Benoit-MINT Benoit-MINT enabled auto-merge July 8, 2024 13:57
@Benoit-MINT Benoit-MINT disabled auto-merge July 8, 2024 14:05
@Benoit-MINT Benoit-MINT force-pushed the bug-PJ-multiples-desactivees branch from 9086cb0 to ebea9e3 Compare July 8, 2024 14:10
@Benoit-MINT Benoit-MINT added this pull request to the merge queue Jul 8, 2024
Merged via the queue into main with commit 76a8e71 Jul 8, 2024
18 checks passed
@Benoit-MINT Benoit-MINT deleted the bug-PJ-multiples-desactivees branch July 8, 2024 14:37
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