-
Notifications
You must be signed in to change notification settings - Fork 22
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
New Emailing Settings #362
base: master
Are you sure you want to change the base?
New Emailing Settings #362
Conversation
'attestationEmails' and 'uploadConfirmEmails' checkboxes added to the UserChangeForm.
to get the values of the 'attestationEmails' and 'uploadConfirmEmails' checkboxes of the UserChangeForm
'attestationEmails' and 'uploadConfirmEmails' have been added to the class User
to check if the user has selected the checkbox on the 'Change Account' page to confirm their willingness to receive an email fo each file upload ('False' by default)
to check if the user has selected the checkbox on the 'Change Account' page to confirm their willingness to receive attestation emails ('True' by default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, some suggestions inline.
src/accounts/views.py
Outdated
#to get the value of the 'attestationEmails' checkbox. | ||
attestationEmails = request.POST.get('attestationEmails') | ||
#to change the values of the value of the checkbox to 'True' or 'False' | ||
is_checked = bool(attestationEmails) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is never used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment should relate to line 73-88. I would guess
user = form.save(commit=False)
attestationEmails = form.cleaned_data['attestationEmails']
uploadConfirmEmails = form.cleaned_data['uploadConfirmEmails']
user.attestationEmails = attestationEmails
user.uploadConfirmEmails = uploadConfirmEmails
user.save()
does the job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Abdallah-Abouelatta I mean that only the lines quoted above should remain; the rest assigns a variable that is never used and can be deleted.
'attestation_emails' and 'upload_confirm_emails' checkboxes added to the UserChangeForm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit : 1dc36c7
Looks good to me :-)
to get the values of the 'attestation_emails' and 'upload_confirm_emails' checkboxes of the UserChangeForm
'attestation_emails' and 'upload_confirm_emails' have been added to the class User
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit 79c508c
looks good to me :-)
to check if the user has selected the checkbox on the 'Change Account' page to confirm their willingness to receive an email for each file upload ('False' by default)
to check if the user has selected the checkbox on the 'Change Account' page to confirm their willingness to receive attestation_emails ('True' by default)
The commit=False in the form.save(commit=False) is used to prevent the form from saving the object to the database immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better now, can you also fix the typo in the pull request title.
'upload_confirm_emails' and 'attestation_emails' help text updated
A line that was previously too long has been broken into two separate lines for better readability.
A line that was previously too long has been broken into two separate lines for better readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I think this is good to merge.
'upload_confirm_emails' and 'attestation_emails' help text updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps change
Praktomat/src/solutions/views.py
Lines 167 to 168 in 49ab69f
if solution.author.email: | |
message.send() # any PY2-PY3 problem in here ? |
to become like that:
if solution.author.email and User.attestation_emails:
message.send()
'and User.upload_confirm_emails' added to the if-statement of sending the upload confirmation email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
Implementing customizable email settings that allow users to select whether they wish to receive confirmation emails or not.
By default, the 'upload_confirm_emails' checkbox, which allows users to opt-in to receive email notifications for each file upload they make, is set to 'False'. However, the 'attestation_emails' checkbox, which allows users to opt-in to receive email notifications for each task attestation, is set to 'True' by default.