-
Notifications
You must be signed in to change notification settings - Fork 4
Course Testimonials #46
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
base: master
Are you sure you want to change the base?
Changes from all commits
89b96bb
4d8516d
e458800
068cf42
8ce566d
6f51715
bc12174
6e0b0ab
e77e8c1
fcca3ca
2e5f1bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,20 @@ | |
from util import response, func_cache | ||
|
||
|
||
@response.request_get() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we remove category_id from this endpoint, then the result is fairly similar to |
||
def list_categories_with_id(request): | ||
categories = Category.objects.order_by("displayname").all() | ||
res = [ | ||
{ | ||
"category_id": cat.id, | ||
"displayname": cat.displayname, | ||
"slug": cat.slug, | ||
"euclid_codes": [euclidcode.code for euclidcode in cat.euclid_codes.all()] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is likely going to be pretty slow, since we're doing one DB query for every row returned from I would suggest to use
which under the hood will do an SQL JOIN, so everything is fetched in one query. |
||
} | ||
for cat in categories | ||
] | ||
return response.success(value=res) | ||
|
||
@response.request_get() | ||
def list_categories(request): | ||
categories = Category.objects.order_by("displayname").all() | ||
|
@@ -27,6 +41,7 @@ def list_categories_with_meta(request): | |
categories = Category.objects.select_related("meta").order_by("displayname").all() | ||
res = [ | ||
{ | ||
"category_id": cat.id, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest removing this in favour of slug. |
||
"displayname": cat.displayname, | ||
"slug": cat.slug, | ||
"examcountpublic": cat.meta.examcount_public, | ||
|
@@ -153,6 +168,7 @@ def list_exams(request, slug): | |
|
||
def get_category_data(request, cat): | ||
res = { | ||
"category_id": cat.id, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removal |
||
"displayname": cat.displayname, | ||
"slug": cat.slug, | ||
"admins": [], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ class NotificationType(enum.Enum): | |
NEW_COMMENT_TO_COMMENT = 2 | ||
NEW_ANSWER_TO_ANSWER = 3 | ||
NEW_COMMENT_TO_DOCUMENT = 4 | ||
UPDATE_TO_TESTIMONIAL_APPROVAL_STATUS = 5 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's worth creating a custom DB migration to enable this setting for everyone by default. In The migration should create a It might be worth having a reverse migration too, but I'm not sure what the correct semantics should be. Maybe delete every row that has |
||
|
||
|
||
class Notification(models.Model): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,18 @@ def send_notification( | |
associated_data: Answer, | ||
) -> None: ... | ||
|
||
@overload | ||
def send_notification( | ||
sender: User, | ||
receiver: User, | ||
type_: Literal[ | ||
NotificationType.UPDATE_TO_TESTIMONIAL_APPROVAL_STATUS | ||
], | ||
title: str, | ||
message: str, | ||
associated_data: str, #Update to Testimonial | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a TODO that you meant to finish? |
||
) -> None: ... | ||
|
||
|
||
@overload | ||
def send_notification( | ||
|
@@ -51,10 +63,11 @@ def send_notification( | |
type_: NotificationType, | ||
title: str, | ||
message: str, | ||
associated_data: Union[Answer, Document], | ||
associated_data: Union[Answer, Document, str], | ||
): | ||
if sender == receiver: | ||
return | ||
|
||
if is_notification_enabled(receiver, type_): | ||
send_inapp_notification( | ||
sender, receiver, type_, title, message, associated_data | ||
|
@@ -97,7 +110,7 @@ def send_email_notification( | |
type_: NotificationType, | ||
title: str, | ||
message: str, | ||
data: Union[Document, Answer], | ||
data: Union[Document, Answer, str], | ||
): | ||
"""If the user has email notifications enabled, send an email notification. | ||
|
||
|
@@ -114,16 +127,34 @@ def send_email_notification( | |
) | ||
): | ||
return | ||
|
||
send_mail( | ||
f"BetterInformatics: {title} / {data.display_name if isinstance(data, Document) else data.answer_section.exam.displayname}", | ||
|
||
email_body = "" | ||
if isinstance(data, Document): | ||
email_body = f"BetterInformatics: {title} / {data.display_name}", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trailing comma needs to be removed. Email subject and body shouldn't both be in a variable called |
||
( | ||
f"Hello {receiver.profile.display_username}!\n" | ||
f"{message}\n\n" | ||
f"View it in context here: {get_absolute_notification_url(data)}" | ||
), | ||
) | ||
elif isinstance(data, str): | ||
email_body = f"BetterInformatics: {title}", | ||
( | ||
f"Hello {receiver.profile.display_username}!\n" | ||
f"{message}\n\n" | ||
) | ||
else: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These conditionals seem brittle in the current order, since the I suggest rearranging the conditions so we do |
||
email_body = f"BetterInformatics: {title} / {data.answer_section.exam.displayname}", | ||
( | ||
f"Hello {receiver.profile.display_username}!\n" | ||
f"{message}\n\n" | ||
f"View it in context here: {get_absolute_notification_url(data)}" | ||
) | ||
|
||
send_mail( | ||
email_body, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently it sends mails like this. See the "Subject" line where the tuple syntax is displayed verbatim.
The body text is also wrong, I think. It's using the subject line. You should be able to debug this in development mode by checking the Docker Compose logs for the backend; by default, when in development, emails are sent to the console. |
||
f'"{sender.username} (via BetterInformatics)" <{settings.VERIF_CODE_FROM_EMAIL_ADDRESS}>', | ||
[receiver.email], | ||
from_email=[sender.email], | ||
recipient_list=[receiver.email], | ||
fail_silently=False, | ||
) | ||
|
||
|
@@ -200,3 +231,13 @@ def new_comment_to_document(document: Document, new_comment: DocumentComment): | |
"A new comment was added to your document.\n\n{}".format(new_comment.text), | ||
document, | ||
) | ||
|
||
def update_to_testimonial_status(sender, receiver, title, message): | ||
send_notification( | ||
sender, | ||
receiver, | ||
NotificationType.UPDATE_TO_TESTIMONIAL_APPROVAL_STATUS, | ||
title, | ||
message, | ||
"" | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
from django.contrib import admin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. File shouldn't be needed, we don't use Django-Admin so I think deleting is better to avoid confusion. |
||
|
||
# Register your models here. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from django.apps import AppConfig | ||
|
||
|
||
class TestimonialsConfig(AppConfig): | ||
default_auto_field = "django.db.models.BigAutoField" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this line needed? I realise |
||
name = "testimonials" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
from django.db import models | ||
from django.db.models import Q, UniqueConstraint | ||
|
||
|
||
class ApprovalStatus(models.IntegerChoices): | ||
APPROVED = 0, "Approved" | ||
PENDING = 1, "Pending" | ||
REJECTED = 2, "Rejected" | ||
|
||
|
||
class Testimonial(models.Model): | ||
id = models.AutoField(primary_key=True) | ||
author = models.ForeignKey("auth.User", on_delete=models.CASCADE, default="") | ||
category = models.ForeignKey( # Link Testimonial to a Category | ||
"categories.Category", | ||
on_delete=models.CASCADE# Delete testimonials if category is deleted | ||
) | ||
testimonial = models.TextField() | ||
year_taken = models.IntegerField() | ||
approval_status = models.IntegerField( | ||
choices=ApprovalStatus.choices, | ||
default=ApprovalStatus.PENDING, | ||
) | ||
|
||
class Meta: | ||
#Only one row with (author, course) where approval_status is APPROVED or PENDING can exist. | ||
#Multiple rejected rows can exist for (author, course) combination. | ||
constraints = [ | ||
UniqueConstraint( | ||
fields=["author", "category"], | ||
condition=Q(approval_status__in=[ApprovalStatus.APPROVED, ApprovalStatus.PENDING]), | ||
name="unique_approved_or_pending_per_author_course", | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
from django.test import TestCase | ||
|
||
# Create your tests here. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
from django.urls import path | ||
from testimonials import views | ||
urlpatterns = [ | ||
path("listtestimonials/", views.testimonial_metadata, name="testimonial_list"), | ||
path("gettestimonial/", views.get_testimonial_metadata_by_code, name="get_testimonial"), | ||
path('addtestimonial/', views.add_testimonial, name='add_testimonial'), | ||
path('removetestimonial/', views.remove_testimonial, name='remove_testimonial'), | ||
path('updatetestimonialapproval/', views.update_testimonial_approval_status, name="update_testimonial_approval_status") | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,140 @@ | ||
from util import response | ||
from ediauth import auth_check | ||
from testimonials.models import Testimonial, ApprovalStatus | ||
from categories.models import Category | ||
from django.contrib.auth.models import User | ||
from django.shortcuts import get_object_or_404 | ||
from datetime import timedelta | ||
from django.http import JsonResponse | ||
from django.views.decorators.csrf import csrf_exempt | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a sus import security-wise; I suggest removing unused imports. |
||
from notifications.notification_util import update_to_testimonial_status | ||
import ediauth.auth_check as auth_check | ||
|
||
@response.request_get() | ||
@auth_check.require_login | ||
def testimonial_metadata(request): | ||
testimonials = Testimonial.objects.all() | ||
res = [ | ||
{ | ||
"author_id": testimonial.author.username, | ||
"author_diplay_name": testimonial.author.profile.display_username, | ||
"category_id": testimonial.category.id, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace with |
||
"euclid_codes": [euclidcode.code for euclidcode in testimonial.category.euclid_codes.all()], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some form of prefetching would again be useful here, we're doing one DB query per testimonial (and some are repeats even, since there are multiple testimonials per category). This is very slow. |
||
"course_name": testimonial.category.displayname, | ||
"testimonial": testimonial.testimonial, | ||
"testimonial_id": testimonial.id, | ||
"year_taken": testimonial.year_taken, | ||
"approval_status": testimonial.approval_status, | ||
} | ||
for testimonial in testimonials | ||
] | ||
return response.success(value=res) | ||
|
||
@response.request_get("category_id") | ||
@auth_check.require_login | ||
def get_testimonial_metadata_by_code(request): | ||
category_id = request.POST.get('category_id') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This endpoint is unused. I can see it being useful though (maybe the category-page tab should be changed to use this endpoint instead), but there's a lot of duplicate with Like:
|
||
try: | ||
category_obj = Category.objects.get(id=category_id) | ||
except Category.DoesNotExist: | ||
return response.not_possible(f"The category with id {category_id} does not exist in the database.") | ||
testimonials = Testimonial.objects.filter(category=category_obj) | ||
res = [ | ||
{ | ||
"author_id": testimonial.author.username, | ||
"author_diplay_name": testimonial.author.profile.display_username, | ||
"category_id": testimonial.category.id, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Slug here too |
||
"euclid_codes": testimonial.category.euclid_codes, | ||
"course_name": testimonial.category.displayname, | ||
"testimonial": testimonial.testimonial, | ||
"testimonial_id": testimonial.id, | ||
"year_taken": testimonial.year_taken, | ||
"approval_status": testimonial.approval_status, | ||
} | ||
for testimonial in testimonials | ||
] | ||
return response.success(value=res) | ||
|
||
@response.request_post("category_id", "year_taken", optional=True) | ||
@auth_check.require_login | ||
def add_testimonial(request): | ||
author = request.user | ||
category_id = request.POST.get('category_id') #course code instead of course name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Slug |
||
year_taken = request.POST.get('year_taken') | ||
testimonial = request.POST.get('testimonial') | ||
|
||
if not author: | ||
return response.not_possible("Missing argument: author") | ||
if not year_taken: | ||
return response.not_possible("Missing argument: year_taken") | ||
if not testimonial: | ||
return response.not_possible("Missing argument: testimonial") | ||
|
||
testimonials = Testimonial.objects.all() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fetching all testimonials by everyone for all courses, but we really only care about testimonials by one author for one course. Filtering in the database would be dozens of times faster than fetching everything and filtering in Python. I would suggest something like existing_testimonial= Testimonial.objects.filter(author=author, category__id=category_id).first()
if existing_testimonial:
if existing_testimonial.approval_status ...
return
testimonial = Testimonial.objects.create() |
||
category_obj = Category.objects.get(id=category_id) | ||
|
||
for t in testimonials: | ||
if t.author == author and t.category == category_obj and (t.approval_status == ApprovalStatus.APPROVED): | ||
return response.not_possible("You have written a testimonial for this course that has been approved.") | ||
elif t.author == author and t.category == category_obj and (t.approval_status == ApprovalStatus.PENDING): | ||
return response.not_possible("You have written a testimonial for this course that is currently pending approval.") | ||
|
||
testimonial = Testimonial.objects.create( | ||
author=author, | ||
category=category_obj, | ||
year_taken=year_taken, | ||
approval_status= ApprovalStatus.PENDING, | ||
testimonial=testimonial, | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, this doesn't check against the Uniqueness constraint that you defined in
If you don't need that constraint (because the Python checks above would do the same thing, give or take some race conditions), then I think it's better to remove it altogether. |
||
|
||
return response.success(value={"testimonial_id" : testimonial.id, "approved" : False}) | ||
|
||
@response.request_post("username", 'testimonial_id', optional=True) | ||
@auth_check.require_login | ||
def remove_testimonial(request): | ||
username = request.POST.get('username') | ||
testimonial_id = request.POST.get('testimonial_id') | ||
|
||
testimonial = Testimonial.objects.filter(id=testimonial_id) #Since id is primary key, always returns 1 or none. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better to encode the fact that the result is 1 or None in the type. Currently Rather, I suggest doing |
||
|
||
if not testimonial: | ||
return response.not_possible("Testimonial not found for author: " + username + " with id " + testimonial_id) | ||
|
||
if not (testimonial[0].author == request.user or auth_check.has_admin_rights(request)): | ||
return response.not_possible("No permission to delete this.") | ||
|
||
testimonial.delete() | ||
return response.success(value="Deleted Testimonial " + str(testimonial)) | ||
|
||
@response.request_post("title", "message", optional=True) | ||
@auth_check.require_login | ||
def update_testimonial_approval_status(request): | ||
sender = request.user | ||
has_admin_rights = auth_check.has_admin_rights(request) | ||
testimonial_author = request.POST.get('author') | ||
receiver = get_object_or_404(User, username=testimonial_author) | ||
testimonial_id = request.POST.get('testimonial_id') | ||
title = request.POST.get('title') | ||
message = request.POST.get('message') | ||
approval_status = request.POST.get('approval_status') | ||
course_name = request.POST.get('course_name') | ||
|
||
testimonial = Testimonial.objects.filter(id=testimonial_id) | ||
|
||
final_message = "" | ||
if has_admin_rights: | ||
testimonial.update(approval_status=approval_status) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you want to add checks (frontend & backend) so you can't approve or reject testimonials which already have a decision? Currently admins can approve the same testimonial many times in succession, each one sending a new notification. Maybe the buttons should be disabled and the backend should prevent it. |
||
if approval_status == str(ApprovalStatus.APPROVED.value): | ||
final_message = f'Your Testimonial to {course_name}: \n"{testimonial[0].testimonial}" has been Accepted, it is now available to see in the Testimonials tab.' | ||
if (sender != receiver): | ||
update_to_testimonial_status(sender, receiver, title, final_message) #notification | ||
return response.success(value="Testimonial Accepted and the notification has been sent to " + str(receiver) + ".") | ||
elif approval_status == str(ApprovalStatus.REJECTED.value): | ||
final_message = f'Your Testimonial to {course_name}: \n"{testimonial[0].testimonial}" has not been accepted due to: {message}' | ||
if (sender != receiver): | ||
update_to_testimonial_status(sender, receiver, title, final_message) #notification | ||
return response.success(value="Testimonial Not Accepted " + "and the notification has been sent to " + str(receiver) + ".") | ||
else: | ||
return response.not_possible("Cannot Update the Testimonial to approval_status: " + str(approval_status)) | ||
else: | ||
return response.not_possible("No permission to approve/disapprove this testimonial.") |
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
slug
acts as the user-friendly unique ID for categories, and I'd prefer not to send raw database IDs to the frontend since it's not good practice. This PR looks like it can be entirely written with slugs, so could we do that?