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

[DONE] Update the restriction fields in the video form #1158

Merged

Conversation

SebastienCozeDev
Copy link
Collaborator

@SebastienCozeDev SebastienCozeDev commented Jun 11, 2024

Before sending your pull request, make sure the following are done:

  • You have read our contribution guidelines.
  • Your PR targets the develop branch.
  • The title of your PR starts with [WIP] or [DONE].

@SebastienCozeDev SebastienCozeDev added the enhancement New feature or request label Jun 11, 2024
@SebastienCozeDev SebastienCozeDev self-assigned this Jun 11, 2024
@SebastienCozeDev SebastienCozeDev changed the base branch from master to develop June 11, 2024 12:42
@SebastienCozeDev SebastienCozeDev changed the title [WIP] Update the restriction fields in the video form [DONE] Update the restriction fields in the video form Jul 15, 2024
@SebastienCozeDev SebastienCozeDev marked this pull request as ready for review July 15, 2024 14:35
Copy link
Collaborator

@AymericJak AymericJak left a comment

Choose a reason for hiding this comment

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

Quelques détails mineurs

pod/video/static/js/video_edit.js Outdated Show resolved Hide resolved
pod/video/static/js/video_edit.js Outdated Show resolved Hide resolved
pod/video/static/js/video_edit.js Outdated Show resolved Hide resolved
pod/video/static/js/video_edit.js Show resolved Hide resolved
pod/video/tests/test_views.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@AymericJak AymericJak left a comment

Choose a reason for hiding this comment

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

ok pour moi.

Je n'ai pas testé localement

@ptitloup
Copy link
Contributor

Hello, je viens de tester. Il manque les traductions en francais et la case à cocher quand on séléctionne accès restreint. En effet, quand on sélectionne accès restreint, il faut qu'on puisse choisir si on veut restreindre par authentification (et à ce moment là, on peut choisir éventuellement un groupe s'il y en a dans Pod) et/ou par mot de passe. Merci !

Copy link
Collaborator

@AymericJak AymericJak left a comment

Choose a reason for hiding this comment

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

Toujours RAS au niveau du code. Je vais tester

@AymericJak
Copy link
Collaborator

J'ai une remarque quand je teste :
image

Le field pour les groupes ne s'affiche pas correctement.

…rm' into SebastienCozeDev/update_video_form

# Conflicts:
#	pod/locale/fr/LC_MESSAGES/django.mo
Copy link
Contributor

@ptitloup ptitloup left a comment

Choose a reason for hiding this comment

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

ok pour moi

pod/video/forms.py Outdated Show resolved Hide resolved
pod/video/forms.py Outdated Show resolved Hide resolved
pod/video/forms.py Outdated Show resolved Hide resolved
ptitloup and others added 6 commits July 16, 2024 16:50
…rm' into SebastienCozeDev/update_video_form

# Conflicts:
#	pod/locale/fr/LC_MESSAGES/django.po
#	pod/locale/fr/LC_MESSAGES/djangojs.po
#	pod/locale/nl/LC_MESSAGES/django.po
#	pod/locale/nl/LC_MESSAGES/djangojs.po
Copy link
Collaborator

@Badatos Badatos left a comment

Choose a reason for hiding this comment

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

Beau travail, merci :)
Il faudra juste peut-etre refaire un dernier compilemessages, car certains textes n'étaient pas traduits dans mon dernier essai.

@fanfounet
Copy link
Collaborator

Hello,
Je viens de tester fonctionnellement cette partie. Tout fonctionne correctement. J'ai toutefois une remarque. Quand on choisit "Accès restreint" si on remplit la case mot de passe on enregistre ça fonctionne mais si on édite à nouveau le champ mot de passe est vide. Ça peut être troublant pour l'utilisateur de se demander si il a bien ou pas ajouté un mdp. Peut etre également coté UX la case à cocher accès restreint par authentification au dessous de la partie mdp est peut être compliqué à comprendre. Plus simple, pour bien segmenter, de faire la liste déroulante avec "public", "brouillon/privé", "accès restreint par mot de passe", "accès restreint par authentification" c'est une idée ;-)

@SebastienCozeDev
Copy link
Collaborator Author

Hello, Je viens de tester fonctionnellement cette partie. Tout fonctionne correctement. J'ai toutefois une remarque. Quand on choisit "Accès restreint" si on remplit la case mot de passe on enregistre ça fonctionne mais si on édite à nouveau le champ mot de passe est vide. Ça peut être troublant pour l'utilisateur de se demander si il a bien ou pas ajouté un mdp. Peut etre également coté UX la case à cocher accès restreint par authentification au dessous de la partie mdp est peut être compliqué à comprendre. Plus simple, pour bien segmenter, de faire la liste déroulante avec "public", "brouillon/privé", "accès restreint par mot de passe", "accès restreint par authentification" c'est une idée ;-)

Bonjour, merci d'avoir testé. Je vais regarder pour le mot de passe. Pour ce qui est de séparer l'accès restreint par mot de passe et par l'authentification, je serais plus d'avis à mettre une balise <hr> entre les deux. Ça permettrait à l'utilisateur d'utiliser les deux possibilités en même temps s'il le souhaite.

@SebastienCozeDev SebastienCozeDev changed the title [DONE] Update the restriction fields in the video form [WIP] Update the restriction fields in the video form Jul 17, 2024
@fanfounet
Copy link
Collaborator

Comme tu veux mais l'important c'est de faire comprendre que c'est fromage ou dessert et pas fromage et dessert pour l'accès restreint !!! ;-)

@Badatos
Copy link
Collaborator

Badatos commented Jul 17, 2024

Comme tu veux mais l'important c'est de faire comprendre que c'est fromage ou dessert et pas fromage et dessert pour l'accès restreint !!! ;-)

et pourquoi ce ne serait pas "fromage ET/OU dessert" ? ;)

Oui pourquoi pas tu veux dire que l'on pourrait en premier s'authentifier par SSO ou autre puis un mot de passe pour la vidéo ?

@SebastienCozeDev SebastienCozeDev changed the title [WIP] Update the restriction fields in the video form [DONE] Update the restriction fields in the video form Jul 17, 2024
Copy link
Contributor

@ptitloup ptitloup left a comment

Choose a reason for hiding this comment

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

ok pour moi, merci

@ptitloup ptitloup merged commit 7126524 into EsupPortail:develop Jul 17, 2024
3 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
5 participants