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] fix recuring date in edit meeting #946

Merged

Conversation

ptitloup
Copy link
Contributor

@ptitloup ptitloup commented Sep 12, 2023

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].

@ptitloup ptitloup marked this pull request as ready for review September 14, 2023 13:45
@ptitloup ptitloup changed the title [WIP] fix recuring date in edit meeting [DONE] fix recuring date in edit meeting Sep 14, 2023
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.

quelques pydocs et c'est bon pour le code ^^

pod/meeting/forms.py Show resolved Hide resolved
pod/meeting/forms.py Show resolved Hide resolved
Copy link
Contributor

@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.

Pas de problèmes majeurs rencontrés.
Néanmoins, un petit soucis.

Lorsque l'on créé une réunion (admettons à la date du 20 septembre) et que l'on applique une récurrence. La date affichée sous la vignette est incorrecte. En effet, celle indiquée correspond à la prochaine date de récurrence.

image

Ci-dessus, on peut voir, que la date affichée est le 27 septembre, soit une semaine après (puisque récurrence hebdomadaire).

@SebastienCozeDev SebastienCozeDev self-requested a review September 18, 2023 13:35
Copy link
Contributor

@SebastienCozeDev SebastienCozeDev left a comment

Choose a reason for hiding this comment

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

J'ai testé localement et j'ai remarqué plusieurs soucis.

Choix de la fréquence

Sur le choix de la fréquence, j'ai pu choisir --------- et je n'ai pas eu d'erreur. La création de la réunion a été validé.

Ciblage des erreurs

Lorsqu'il y a des erreurs au niveau des champs de la modale de récurrence, on ne voit pas directement là où il y a une erreur. L'utilisateur doit chercher de lui-même. Ce serait peut-être un bonne idée d'ajouter une bordure rouge sur le bouton "Récurrence".

IntegrityError

Lorsque je met 0 dans le champs id_frequency, je n'ai pas d'erreur directement dans le formulaire, mais une IntegrityError.

image

@ptitloup
Copy link
Contributor Author

Pas de problèmes majeurs rencontrés. Néanmoins, un petit soucis.

Lorsque l'on créé une réunion (admettons à la date du 20 septembre) et que l'on applique une récurrence. La date affichée sous la vignette est incorrecte. En effet, celle indiquée correspond à la prochaine date de récurrence.

image

Ci-dessus, on peut voir, que la date affichée est le 27 septembre, soit une semaine après (puisque récurrence hebdomadaire).

Je viens de refaire les fonctions de calcul, peux-tu retester stp ? merci !

@ptitloup
Copy link
Contributor Author

J'ai testé localement et j'ai remarqué plusieurs soucis.

Choix de la fréquence

Sur le choix de la fréquence, j'ai pu choisir --------- et je n'ai pas eu d'erreur. La création de la réunion a été validé.

Ciblage des erreurs

Lorsqu'il y a des erreurs au niveau des champs de la modale de récurrence, on ne voit pas directement là où il y a une erreur. L'utilisateur doit chercher de lui-même. Ce serait peut-être un bonne idée d'ajouter une bordure rouge sur le bouton "Récurrence".

IntegrityError

Lorsque je met 0 dans le champs id_frequency, je n'ai pas d'erreur directement dans le formulaire, mais une IntegrityError.

image

Je viens de faire un patch pour corriger les pb remontés, peux-tu tester à nouveau stp ? Merci

pod/meeting/forms.py Show resolved Hide resolved
if self.start == timezone.now().date():
# start_datetime = dt.combine(self.start, self.start_time)
# start_datetime = timezone.make_aware(start_datetime)
next_one = self.next_occurrence(timezone.now().date())
Copy link
Collaborator

Choose a reason for hiding this comment

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

petite pydoc ?

Copy link
Contributor

@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.

Ton fix est ok pour moi !

Après, une amélioration serait intéressante, le optgroup pour le champs de sélection.

Je proposerais quelque chose du style 👍
image

Soit on laisse les "----" soit on met quelque chose de plus clair comme "Ne pas répéter".

Pour faire ça, il faut juste modifier INTERVAL_CHOICES dans le modèle.

    INTERVAL_FREQUENCIES = (
        (DAILY, _("Daily")),
        (WEEKLY, _("Weekly")),
        (MONTHLY, _("Monthly")),
        (YEARLY, _("Yearly")),
    )

    INTERVAL_CHOICES = (
        ('', 'Ne pas répéter'),
        ('-- Fréquences --', INTERVAL_FREQUENCIES),
    )

Dans le cas où on souhaite laisser "---", il faut juste supprimer cette ligne ('', 'Ne pas répéter'),

Copy link
Contributor

@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.

Deux petits commentaires, puis c'est bon pour moi :)

@@ -4887,6 +4894,15 @@ msgstr "Mois"
msgid "Yearly"
msgstr "Année"

#: pod/meeting/models.py
msgid "Choose repeat frequency"
msgstr "choisissez la fréquence de répétition"
Copy link
Contributor

Choose a reason for hiding this comment

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

Pense à mettre la majuscule :)

(DAILY, _("Daily")),
(WEEKLY, _("Weekly")),
(MONTHLY, _("Monthly")),
(YEARLY, _("Yearly")),
)

INTERVAL_CHOICES = (
('', '%s' % _("Choose repeat frequency")),
('%s :' % _("Frequency"), INTERVAL_FREQUENCIES),
Copy link
Contributor

Choose a reason for hiding this comment

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

Pour le coup, j'aurais suivi le même style qui a été appliqué pour les vidéos avec les langues.
image

Donc avec les -- Texte --. Pour être uniforme :)

Copy link
Contributor

@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.

Merci :)

@ptitloup ptitloup merged commit c3c0554 into EsupPortail:develop Sep 20, 2023
4 checks passed
vsabatie pushed a commit to vsabatie/Pod that referenced this pull request Nov 22, 2023
* change position of calendar when it's opened in modal

* dev get date start value

* fix weekly recurring

* fix bug with recurring weekly meeting on monday

* add translation

* add pydoc

* fix recurring options and computing

* add aria-hidden in icon tags

* add pydoc on meeting model function

* fix recurrence and test

* fix translation, add optgroup for frequency, add unit test for weekly frequency on monday

* improve display of frequencies choices
vsabatie pushed a commit to vsabatie/Pod that referenced this pull request Nov 23, 2023
* change position of calendar when it's opened in modal

* dev get date start value

* fix weekly recurring

* fix bug with recurring weekly meeting on monday

* add translation

* add pydoc

* fix recurring options and computing

* add aria-hidden in icon tags

* add pydoc on meeting model function

* fix recurrence and test

* fix translation, add optgroup for frequency, add unit test for weekly frequency on monday

* improve display of frequencies choices
vsabatie pushed a commit to vsabatie/Pod that referenced this pull request Nov 23, 2023
* change position of calendar when it's opened in modal

* dev get date start value

* fix weekly recurring

* fix bug with recurring weekly meeting on monday

* add translation

* add pydoc

* fix recurring options and computing

* add aria-hidden in icon tags

* add pydoc on meeting model function

* fix recurrence and test

* fix translation, add optgroup for frequency, add unit test for weekly frequency on monday

* improve display of frequencies choices
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants