Set shiftremindermail to optional #559

crosspolar · 2024-09-27T08:53:25Z

This PR changes ShiftReminderMail to be optional and so resolves #302

The MultipleChoiceField keeps tuples, e.g.

( 
    ("1", "Naveen"), 
    ("2", "Pranav"), 
    ("3", "Isha"), 
    ("4", "Saloni"), 
)

but the ArrayField only stores the keys, e.g. ["1", "2"].

Nice: Because we've updated to Django 5, I was able to set choices as callable (see here)
I changed also the parameter default at get_mails_mandatory to a literal bećause I found None not really helpful. Now both represents all types of mails, default and non-default ones.

Open:

Decide together with Vorstand which mail should be optional

Theophile-Madet

I left a few comments.

Assuming that newly create TapirUsers get the optional-but-on-by-default mails correctly set (maybe write a test for that?), we could deploy and right after deploying open a shell on the server to enable the mail for all users manually. That would be easier than a data migration I think.

I haven't done data migration yet so if you feel like it, you could make one, then we'd have an example.

There is another problem: if we add a new mail later, that is optional and enabled by default, it wont be added to the field in TapirUser. We could manually add it each time, but it looks likely that we would forget.
What if, instead of saving which mails are enabled in an ArrayField, we create a separate model with user, mail_id and enabled columns. Then, in user_wants_to_or_has_to_receive_mail, we first look if the corresponding user/mail pair exists and if it does, use that value. If it doesn't, use the default value from the mail class.
That would also eliminate the need for data migrations or manual work.

tapir/accounts/models.py

tapir/core/tapir_email_base.py

Theophile-Madet · 2024-09-27T13:33:16Z

tapir/core/tapir_email_base.py

@@ -17,9 +17,17 @@

 all_emails: Dict[str, Type[TapirEmailBase]] = {}

+enabled_by_default_options = Literal[True, False, "both"]


What's the problem with using None instead of "both"?

I think readability. For example, if I want both default and non-default values, None would read like I don't get None of it

Even as an optional parameter? It's quite standard at least in Python.
It would look like get_mails() for all mails, get_mails(mandatory=True) with just one parameter, get_mails(mandatory=False, enabled_by_default=True) with both, isn't that fine?

tapir/core/tapir_email_base.py

Co-authored-by: Théophile MADET <[email protected]>

crosspolar · 2024-09-27T21:02:19Z

I agree with your idea of having a new model

crosspolar · 2024-10-20T07:43:05Z

Moved the slack discussion here:

class OptionalMails(models.Model):
    user = models.ForeignKey(
        "accounts.TapirUser",
        null=False,
        related_name="mail_setting",
        on_delete=models.CASCADE,
    )
    mail_id = models.CharField(
        max_length=256,
        blank=False,
        choices=get_optional_mails,
    )
    is_requested = models.BooleanField(
        default=True, verbose_name="Is this mail requested by the user?"
    )

    class Meta:
        constraints = [
            models.UniqueConstraint(
                fields=["user", "mail_id"], name="user-mail-constraint"
            )
        ]

Any idea how to create the Form to ask with CheckboxSelectMultiple which mail people want to receive? My attempt so far would be

class MailPreferencesForm(forms.ModelForm):

    class Meta:
        model = OptionalMails
        fields = ["is_requested"]
        widgets = {
            "is_requested": CheckboxSelectMultiple(
                choices=get_mail_types(optional=True),
            )
        }

    def __init__(self, *args, **kwargs):
        self.tapir_user: TapirUser = kwargs.pop("user")
        super().__init__(*args, **kwargs)
        self.fields["is_requested"].queryset = OptionalMails.objects.filter(
            user=self.tapir_user
        )

Theo: You could do it more manually: on init add to self.fields one CheckBox for each existing optional mail, with the mail_id as field name. Then in the view's form_valid() you check which checkboxes are checked.
17:28 Uhr
using a Form instead of a ModelForm

crosspolar · 2024-10-27T17:40:01Z

I think in the Future we need also the possibility for staff to decide which mails are optional and enabled-by-default

Theophile-Madet

Logic looks good 👍

I think the style and naming can be improved in a few places, also there's a few tests we can add. I left comments.

tapir/accounts/forms.py

Theophile-Madet · 2024-10-28T12:43:02Z

tapir/accounts/forms.py

+    mandatory_mails = forms.MultipleChoiceField(
+        required=False,
+        choices=get_mail_types(enabled_by_default="both", optional=False),
+        label=_("Important Mails"),
+        widget=CheckboxSelectMultiple(),
+        initial=[
+            m[0] for m in get_mail_types(enabled_by_default="both", optional=False)
+        ],
+    )
+
+    def __init__(self, *args, **kwargs):
+        tapir_user: TapirUser = kwargs.pop("tapir_user")
+        super().__init__(*args, **kwargs)
+        self.fields["mandatory_mails"].disabled = True


I think the disabled=True can be put directly in the field definition

Suggested change

mandatory_mails = forms.MultipleChoiceField(

required=False,

choices=get_mail_types(enabled_by_default="both", optional=False),

label=_("Important Mails"),

widget=CheckboxSelectMultiple(),

initial=[

m[0] for m in get_mail_types(enabled_by_default="both", optional=False)

],

)

def __init__(self, *args, **kwargs):

tapir_user: TapirUser = kwargs.pop("tapir_user")

super().__init__(*args, **kwargs)

self.fields["mandatory_mails"].disabled = True

mandatory_mails = forms.MultipleChoiceField(

required=False,

choices=get_mail_types(enabled_by_default="both", optional=False),

label=_("Important Mails"),

widget=CheckboxSelectMultiple(),

initial=[

m[0] for m in get_mail_types(enabled_by_default="both", optional=False)

],

disabled=True,

)

def __init__(self, *args, **kwargs):

tapir_user: TapirUser = kwargs.pop("tapir_user")

super().__init__(*args, **kwargs)

Theophile-Madet · 2024-10-28T12:44:14Z

tapir/accounts/models.py

+def get_optional_mails_enabledbydefault():
+    return [m[0] for m in get_mail_types(optional=True, enabled_by_default=True)]


This looks unused

Suggested change

def get_optional_mails_enabledbydefault():

return [m[0] for m in get_mail_types(optional=True, enabled_by_default=True)]

Theophile-Madet · 2024-10-28T12:50:30Z

tapir/accounts/models.py

@@ -239,6 +234,31 @@ def check_password(self, raw_password):
            return False
        return True

+    def optional_mails_by_user(self) -> List[str]:


I'd name this something like get_enabled_and_optional_mail_ids():

It's not returning the mail class but the mail ID

It doesn't return all optional mails but just the enabled ones

by_user is not necessary since it's a method of user

Theophile-Madet · 2024-10-28T12:54:57Z

tapir/accounts/models.py

+class MailChoice(models.Model):
+    name = models.CharField(max_length=256, blank=False, choices=get_optional_mails)
+    choice = models.BooleanField()
+
+    class Meta:
+        constraints = [
+            models.UniqueConstraint(
+                fields=["name", "choice"], name="mail-name-chosen-constraint"
+            )
+        ]
+
+    def __str__(self):
+        return self.name
+
+
+class OptionalMails(models.Model):
+    user = models.ForeignKey(
+        "accounts.TapirUser",
+        null=False,
+        related_name="mail_setting",
+        on_delete=models.CASCADE,
+    )
+    mail = models.ForeignKey(
+        MailChoice,
+        on_delete=models.CASCADE,
+    )


Could this not be a single model with mail_id, user, enabled as field? I think that would be clearer.
Also, with a single model you could have mail_id, user as unique contraint pair.
In the current solution, you could have a user linked to two MailChoice that have opposite values for the same mail id.

tapir/core/tests/test_email_preferences.py

tapir/shifts/emails/stand_in_found_email.py

Theophile-Madet · 2024-10-28T13:22:52Z

tapir/core/tapir_email_base.py


-def mails_not_mandatory(default: bool | None = True) -> List[Tuple[str, str]]:
    return [


Is there an advantage to returning the id, name pair here? We could just return the list of classes directly and let the callers of get_mail_type then decide what they what to do with it.

I thought tuples were necessary for choices, but have to check

Theophile-Madet · 2024-10-28T13:25:47Z

tapir/core/tests/test_email_preferences.py

Maybe a few more tests:

A normal member can't edit the mail preferences of another member

Member office can edit the mail preferences of another member

Normal members can edit their own preferences

Sending post data to the preference view with disabled mandatory mails doesn't actually disable the mails (if someone edited the HTML to enable the fields for example)

crosspolar added 2 commits September 26, 2024 22:37

fix: MultipleChoiceField of additional_mails works again with ArrayField

4ebd05c

feat: set ShiftReminderMail to optional but enabled by default

5c514c6

crosspolar requested a review from Theophile-Madet September 27, 2024 08:53

Theophile-Madet linked an issue Sep 27, 2024 that may be closed by this pull request

Let users manage their email preferences #308

Open

Theophile-Madet requested changes Sep 27, 2024

View reviewed changes

Update tapir/accounts/models.py

8aad570

Co-authored-by: Théophile MADET <[email protected]>

crosspolar added 6 commits October 26, 2024 20:32

(feat) create model for optional mails

1268eaa

(fix) migration with old method

2adfb01

(fix) test errors because of not-installed-apps yet

bfea114

(fix) return correct list of mails

7e79aa4

(fix) tests failed successfully

c004926

(fix) update translations

5cc6bed

crosspolar marked this pull request as ready for review October 27, 2024 17:38

crosspolar requested a review from kerstenkenan October 27, 2024 17:40

Theophile-Madet requested changes Oct 28, 2024

View reviewed changes

(renovate) remove unnecessary template

868d8ef

crosspolar force-pushed the set_shiftremindermail_to_optional branch from 61f0682 to 868d8ef Compare December 26, 2024 17:51

crosspolar added 4 commits December 26, 2024 19:18

(feat) tests for get_mail_types

d40ade8

(refactor) condense choices-calculation for optional-mails

93064b2

(renovate) test name compliance

3a3840d

(feature) update translation

4360746

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set shiftremindermail to optional #559

Set shiftremindermail to optional #559

crosspolar commented Sep 27, 2024 •

edited

Loading

Theophile-Madet left a comment •

edited

Loading

Theophile-Madet Sep 27, 2024

crosspolar Sep 27, 2024

Theophile-Madet Sep 28, 2024

crosspolar commented Sep 27, 2024

crosspolar commented Oct 20, 2024

crosspolar commented Oct 27, 2024

Theophile-Madet left a comment

Theophile-Madet Oct 28, 2024

Theophile-Madet Oct 28, 2024

Theophile-Madet Oct 28, 2024

Theophile-Madet Oct 28, 2024

Theophile-Madet Oct 28, 2024

crosspolar Dec 26, 2024

Theophile-Madet Oct 28, 2024

		@@ -17,9 +17,17 @@

		all_emails: Dict[str, Type[TapirEmailBase]] = {}

		enabled_by_default_options = Literal[True, False, "both"]

		def get_optional_mails_enabledbydefault():
		return [m[0] for m in get_mail_types(optional=True, enabled_by_default=True)]


		def mails_not_mandatory(default: bool \| None = True) -> List[Tuple[str, str]]:
		return [

Set shiftremindermail to optional #559

Are you sure you want to change the base?

Set shiftremindermail to optional #559

Conversation

crosspolar commented Sep 27, 2024 • edited Loading

Theophile-Madet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crosspolar commented Sep 27, 2024

crosspolar commented Oct 20, 2024

crosspolar commented Oct 27, 2024

Theophile-Madet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crosspolar commented Sep 27, 2024 •

edited

Loading

Theophile-Madet left a comment •

edited

Loading