openfun / jitsi-magnify

An authentication and room management system for Jitsi built with Django/React
MIT License
23 stars 6 forks source link

Update models.py #121

Closed ptitloup closed 1 year ago

ptitloup commented 2 years ago

Set weekdays to None if recurrence is not equal to Weekly

sampaccoud commented 2 years ago

Set weekdays to None if recurrence is not equal to Weekly

Nice to see you here @ptitloup :star_struck: Can you help me understand which case goes wrong when we don't do what?

ptitloup commented 2 years ago

With pleasure ! 🥇 thank you very much for your recurring example, it helps me very much and make me save a lot of time ! :-) I found this for example if you create recurring meeting each week on monday and tuesday but if you want to transform it on monthly meeting... It could be not important but I found this issue

sampaccoud commented 2 years ago

I was thinking of what test I should write to evidence the problem but I'm a bit far from the code after the summer :sweat_smile: My understanding at the moment is that, if you switched to monthly recurrence, the weekday field would keep its value but it would not affect your monthly recurrence. Right? So it's more correct to reset the field but there is no misbehavior associated or am I missing something and there is a test to write to demonstrate a bug?

ptitloup commented 2 years ago

My understanding at the moment is that, if you switched to monthly recurrence, the weekday field would keep its value but it would not affect your monthly recurrence. Right?

Yep !

So it's more correct to reset the field but there is no misbehavior associated or am I missing something and there is a test to write to demonstrate a bug?

Correct

ptitloup commented 1 year ago

Hello, I made some unit tests to check reset weekdays. You can do something like that :

def test_models_meetings_get_occurrences_weekly_reset_weekdays(self): """reset weekdays if recurrence not equal to weekly""" meeting = MeetingFactory( start=date(2022, 7, 6), recurrence="weekly", frequency=1, recurring_until=date(2022, 8, 2), nb_occurrences=None, meeting.weekdays = "26" ) self.assertEqual(meeting.weekdays, "26") meeting.recurrence = "daily" meeting.save() meeting.refresh_from_db() self.assertEqual(meeting.weekdays, None) meeting.recurrence = "weekly" meeting.weekdays = "126" meeting.save() meeting.refresh_from_db() self.assertEqual(meeting.weekdays, "126")

sampaccoud commented 1 year ago

@ptitloup thanks. Do you want to add it to the PR and rebase it so we merge it?

ptitloup commented 1 year ago

@ptitloup thanks. Do you want to add it to the PR and rebase it so we merge it?

Ok ! I'll do that asap

ptitloup commented 1 year ago

Done !

sampaccoud commented 1 year ago

:rocket: Let's merge :wink: Thanks for your contribution

ptitloup commented 1 year ago

my pleasure !