Skip to content

New internal YouTube video's player #624

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.

Sign up for GitHub

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

Open
abdelrahman390 wants to merge 8 commits into
base: main
Choose a base branch
from
Open

New internal YouTube video's player #624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
07b21ce
full code, Fixes: #620
abdelrahman390 May 15, 2025
4ee287d
final after edits
abdelrahman390 May 15, 2025
5e81a3d
final
abdelrahman390 May 15, 2025
36666f1
done all edits
abdelrahman390 May 15, 2025
b6fba7e
done
abdelrahman390 May 15, 2025
8cb37f5
Merge branch 'main' into youTube-vedios
abdelrahman390 Aug 4, 2025
38a549e
Merge branch 'main' into youTube-vedios
abdelrahman390 Aug 25, 2025
db7cfdc
Merge branch 'main' into youTube-vedios
A1L13N Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions web/management/commands/create_test_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
Course,
CourseMaterial,
CourseProgress,
EducationalVideo,
Enrollment,
ForumCategory,
ForumReply,
Expand Down Expand Up @@ -68,6 +69,7 @@ def clear_data(self):
Enrollment,
Session,
Course,
EducationalVideo,
Subject,
Profile,
User,
Expand Down
24 changes: 24 additions & 0 deletions web/migrations/0062_educationalvideo_video_id.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Generated by Django 5.1.6 on 2025-05-15 11:24

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("web", "0061_alter_educationalvideo_description_videorequest"),
]

operations = [
migrations.AddField(
model_name="educationalvideo",
name="video_id",
field=models.CharField(
default="",
editable=False,
help_text="Auto-generated unique video identifier.",
max_length=12,
unique=True,
),
),
]
48 changes: 27 additions & 21 deletions web/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,24 @@ class EducationalVideo(models.Model):
help_text="User who uploaded the video. If null, the submission is considered anonymous.",
)
uploaded_at = models.DateTimeField(auto_now_add=True)
video_id = models.CharField(
max_length=12, unique=True, editable=False, default="", help_text="Auto-generated unique video identifier."
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

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

Using an empty string as the default for a unique field can cause integrity constraint violations. Consider using default=None and null=True, or remove the default entirely since the field is auto-generated in the save method.

Suggested change
max_length=12, unique=True, editable=False, default="", help_text="Auto-generated unique video identifier."
max_length=12, unique=True, editable=False, default=None, null=True, help_text="Auto-generated unique video identifier."

Copilot uses AI. Check for mistakes.
)
Comment thread
abdelrahman390 marked this conversation as resolved.

Comment on lines +680 to +683
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 25, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Avoid unique default collisions for video_id; add trailing comma

Using default="" on a unique field risks migration-time collisions and invalid intermediate states. Since the field is editable=False and populated in save(), drop the default and add the trailing comma per lint.

-    video_id = models.CharField(
-        max_length=12, unique=True, editable=False, default="", help_text="Auto-generated unique video identifier."
-    )
+    video_id = models.CharField(
+        max_length=12,
+        unique=True,
+        editable=False,
+        help_text="Auto-generated unique video identifier.",
+    )

Verification (ensure migration 0062 backfills unique IDs before adding the unique constraint):

🏁 Script executed:

#!/bin/bash
# Sanity-check for a data migration that assigns video_id before applying UNIQUE
rg -nP 'migrations/.+0062_.*educationalvideo_video_id' -C3
rg -nP '(RunPython|SeparateDatabaseAndState)' -C2 web/migrations

Length of output: 3493

🏁 Script executed:

#!/bin/bash
# Search for any occurrences of 'video_id' in migration files to locate when the field was added or altered
rg -n "video_id" -C3 web/migrations

Length of output: 615

🏁 Script executed:

#!/bin/bash
# Inspect migration 0062 for AddField, backfill, and unique constraint steps on video_id
rg -nP 'AddField' -C5 web/migrations/0062_educationalvideo_video_id.py
rg -nP 'RunPython' -C5 web/migrations/0062_educationalvideo_video_id.py
rg -nP 'SeparateDatabaseAndState' -C5 web/migrations/0062_educationalvideo_video_id.py
rg -nP 'AlterField' -C5 web/migrations/0062_educationalvideo_video_id.py

Length of output: 650

🏁 Script executed:

#!/bin/bash
# Show full AddField block for video_id in migration 0062
sed -n '1,200p' web/migrations/0062_educationalvideo_video_id.py | sed -n '/AddField/,/)/p'

Length of output: 432

🏁 Script executed:

#!/bin/bash
# Display lines around the video_id field in models.py to confirm default usage
sed -n '670,710p' web/models.py

Length of output: 1362

Prevent video_id default collisions and update migration safely

Dropping the default="" on a unique field without a prior backfill will cause collisions (and likely migration failures) when existing rows are initialized to the same empty string. Instead, remove the default in the model, add the trailing comma per lint, and revise the migration so that it:

  • Adds the new video_id column with a temporary DB-only default (no unique constraint).
  • Backfills every existing record with a unique identifier via migrations.RunPython.
  • Enforces uniqueness in a separate step (via AlterField or SeparateDatabaseAndState).

Locations to update:

  • web/models.py – drop the default and add trailing comma
  • web/migrations/0062_educationalvideo_video_id.py – split AddField/constraint and insert backfill

Model diff:

-    video_id = models.CharField(
-        max_length=12, unique=True, editable=False, default="", help_text="Auto-generated unique video identifier."
-    )
+    video_id = models.CharField(
+        max_length=12,
+        unique=True,
+        editable=False,
+        help_text="Auto-generated unique video identifier.",
+    )

Migration sketch for 0062 (replace the single AddField with steps):

 operations = [
-   migrations.AddField(
-       model_name="educationalvideo",
-       name="video_id",
-       field=models.CharField(default="", editable=False, help_text="Auto-generated unique video identifier.", max_length=12, unique=True),
-   ),
+   # 1) Create the column with only a DB default to avoid nulls
+   migrations.AddField(
+       model_name="educationalvideo",
+       name="video_id",
+       field=models.CharField(
+           max_length=12,
+           editable=False,
+           default="",    # temporary DB default
+       ),
+   ),
+   # 2) Backfill all existing rows with unique values
+   migrations.RunPython(backfill_video_ids, reverse_code=migrations.RunPython.noop),
+   # 3) Enforce uniqueness in state
+   migrations.AlterField(
+       model_name="educationalvideo",
+       name="video_id",
+       field=models.CharField(
+           max_length=12,
+           unique=True,
+           editable=False,
+           help_text="Auto-generated unique video identifier.",
+       ),
+   ),
 ]

This ensures no two rows collide on the same default and that the unique constraint is only applied once every record has a valid, unique video_id.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
video_id = models.CharField(
max_length=12, unique=True, editable=False, default="", help_text="Auto-generated unique video identifier."
)
video_id = models.CharField(
max_length=12,
unique=True,
editable=False,
help_text="Auto-generated unique video identifier.",
)
🤖 Prompt for AI Agents 
In web/models.py around lines 680-683 and in
web/migrations/0062_educationalvideo_video_id.py: remove the model-level
default="" from the video_id CharField and add the missing trailing comma in the
field declaration; then update migration 0062 to avoid introducing a unique
column with a Python-level default by (1) adding the new video_id column with a
temporary DB-only default or null and WITHOUT the unique constraint, (2)
including a migrations.RunPython backfill that generates and writes a unique
identifier for each existing row, and finally (3) in a separate migration step
or via AlterField/SeparateDatabaseAndState, add the unique=True constraint (or
alter the field to set unique=True) once all rows have unique values. Ensure the
DB default is only used for temporary column creation and not persisted in the
model, and keep the backfill id-generation deterministic/UUID-based to avoid
collisions.

@property
def youtube_id(self) -> str | None:
parsed = urlparse(self.video_url)
host = parsed.netloc.lower()

# youtu.be/<id>
if host in ("youtu.be", "www.youtu.be"):
return parsed.path.lstrip("/")

# youtube.com/watch?v=<id>
if host in ("youtube.com", "www.youtube.com"):
return parse_qs(parsed.query).get("v", [None])[0]

return None
Comment on lines +684 to +697
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 25, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

🛠️ Refactor suggestion

Harden YouTube ID extraction; support embed/shorts and mobile hosts; validate 11-char pattern

Current logic misses /embed/, /shorts/, m.youtube.com, and youtube-nocookie.com, and doesn’t validate the 11-char ID. Tighten extraction and validate using the canonical 11-character regex.

     @property
-    def youtube_id(self) -> str | None:
-        parsed = urlparse(self.video_url)
-        host = parsed.netloc.lower()
-
-        # youtu.be/<id>
-        if host in ("youtu.be", "www.youtu.be"):
-            return parsed.path.lstrip("/")
-
-        # youtube.com/watch?v=<id>
-        if host in ("youtube.com", "www.youtube.com"):
-            return parse_qs(parsed.query).get("v", [None])[0]
-
-        return None
+    def youtube_id(self) -> str | None:
+        parsed = urlparse(self.video_url)
+        host = parsed.netloc.lower().lstrip("www.")
+        path = parsed.path
+
+        candidate: str | None = None
+
+        # youtu.be/<id>
+        if host == "youtu.be":
+            candidate = path.strip("/")
+        # youtube.* domains
+        elif host in ("youtube.com", "m.youtube.com", "music.youtube.com", "youtube-nocookie.com"):
+            # watch?v=<id>
+            candidate = parse_qs(parsed.query).get("v", [None])[0]
+            if not candidate:
+                # /embed/<id> or /shorts/<id>
+                import re as _re
+                m = _re.match(r"^/(?:embed|shorts)/([A-Za-z0-9_-]{11})", path)
+                if m:
+                    candidate = m.group(1)
+
+        # Final validation: exact 11 allowed chars
+        if candidate:
+            import re as _re
+            if _re.fullmatch(r"[A-Za-z0-9_-]{11}", candidate):
+                return candidate
+        return None

Optional: add targeted tests for youtube_id covering watch, youtu.be, embed, shorts, m.youtube.com, and invalid cases. I can generate them if helpful.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@property
def youtube_id(self) -> str | None:
parsed = urlparse(self.video_url)
host = parsed.netloc.lower()
# youtu.be/<id>
if host in ("youtu.be", "www.youtu.be"):
return parsed.path.lstrip("/")
# youtube.com/watch?v=<id>
if host in ("youtube.com", "www.youtube.com"):
return parse_qs(parsed.query).get("v", [None])[0]
return None
@property
def youtube_id(self) -> str | None:
parsed = urlparse(self.video_url)
host = parsed.netloc.lower().lstrip("www.")
path = parsed.path
candidate: str | None = None
# youtu.be/<id>
if host == "youtu.be":
candidate = path.strip("/")
# youtube.* domains
elif host in ("youtube.com", "m.youtube.com", "music.youtube.com", "youtube-nocookie.com"):
# watch?v=<id>
candidate = parse_qs(parsed.query).get("v", [None])[0]
if not candidate:
# /embed/<id> or /shorts/<id>
import re as _re
m = _re.match(r"^/(?:embed|shorts)/([A-Za-z0-9_-]{11})", path)
if m:
candidate = m.group(1)
# Final validation: exact 11 allowed chars
if candidate:
import re as _re
if _re.fullmatch(r"[A-Za-z0-9_-]{11}", candidate):
return candidate
return None


class Meta:
verbose_name = "Educational Video"
Expand All @@ -697,27 +715,15 @@ def thumbnail_url(self):
return f"https://img.youtube.com/vi/{vid}/hqdefault.jpg"
return None

@property
def youtube_id(self):
"""
Extract the YouTube video ID, whether it's a long or short URL.
Returns None if not a YouTube link.
"""
parsed = urlparse(self.video_url)
host = parsed.netloc.lower()

# youtu.be/<id>
if host in ("youtu.be", "www.youtu.be"):
return parsed.path.lstrip("/")

# youtube.com/watch?v=<id>
if host in ("youtube.com", "www.youtube.com"):
try:
return parse_qs(parsed.query).get("v", [None])[0]
except Exception:
return None

return None
def save(self, *args, **kwargs):
Comment thread
abdelrahman390 marked this conversation as resolved.
if not self.video_id:
# Keep generating until a unique ID is found
while True:
new_id = uuid.uuid4().hex[:12]
if not EducationalVideo.objects.filter(video_id=new_id).exists():
self.video_id = new_id
break
Comment thread
abdelrahman390 marked this conversation as resolved.
Comment on lines +720 to +725
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

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

The infinite loop for generating unique IDs could become inefficient as the number of videos grows. Consider adding a retry limit or using a different approach like a database sequence.

Suggested change
# Keep generating until a unique ID is found
while True:
new_id = uuid.uuid4().hex[:12]
if not EducationalVideo.objects.filter(video_id=new_id).exists():
self.video_id = new_id
break
# Try up to 10 times to generate a unique ID
max_attempts = 10
for attempt in range(max_attempts):
new_id = uuid.uuid4().hex[:12]
if not EducationalVideo.objects.filter(video_id=new_id).exists():
self.video_id = new_id
break
else:
raise RuntimeError("Could not generate a unique video_id after 10 attempts.")

Copilot uses AI. Check for mistakes.
super().save(*args, **kwargs)
Comment on lines +718 to +726
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 25, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

🛠️ Refactor suggestion

Prevent infinite loop and handle race on unique video_id; add type annotations; satisfy DJ012

  • while True without a cap risks hanging.
  • exists() check is racy; catch IntegrityError and retry.
  • Add type annotations and import Any; consider moving save() above custom properties to satisfy DJ012.
-    def save(self, *args, **kwargs):
-        if not self.video_id:
-            # Keep generating until a unique ID is found
-            while True:
-                new_id = uuid.uuid4().hex[:12]
-                if not EducationalVideo.objects.filter(video_id=new_id).exists():
-                    self.video_id = new_id
-                    break
-        super().save(*args, **kwargs)
+    def save(self, *args: "Any", **kwargs: "Any") -> None:
+        from django.db import IntegrityError
+
+        # Generate ID if missing; retry a few times on race
+        if not self.video_id:
+            attempts = 0
+            while attempts < 5:
+                self.video_id = uuid.uuid4().hex[:12]  # 12 lowercase hex
+                try:
+                    return super().save(*args, **kwargs)
+                except IntegrityError:
+                    # Collision on UNIQUE(video_id); retry with a new ID
+                    self.video_id = ""
+                    attempts += 1
+            # If we get here, something is wrong with ID generation or DB state
+            raise IntegrityError("Failed to generate a unique video_id after multiple attempts.")
+        else:
+            return super().save(*args, **kwargs)

Add missing imports at file top (or reuse existing typing imports):

from typing import Any

If you want to clear DJ012, move save() above custom properties (youtube_id, thumbnail_url). I can push a follow-up patch.

🧰 Tools
🪛 Ruff (0.12.2)

718-726: Order of model's inner classes, methods, and fields does not follow the Django Style Guide: save method should come before custom method

(DJ012)

718-718: Missing return type annotation for public function save

Add return type annotation: None

(ANN201)

718-718: Missing type annotation for *args

(ANN002)

718-718: Missing type annotation for **kwargs

(ANN003)

🤖 Prompt for AI Agents 
In web/models.py around lines 718 to 726, the save() implementation risks an
infinite loop, is racy against concurrent inserts, and lacks type annotations;
change it to limit the generation attempts (e.g., max_attempts constant), import
typing.Any and add a return type/arg types for save(self, *args: Any, **kwargs:
Any) -> None, and wrap the super().save(...) in a retry loop that catches
django.db.IntegrityError to retry ID generation when a unique constraint
violation occurs; also ensure the new imports (from typing import Any and from
django.db import IntegrityError) are added at the top, and optionally move
save() above the custom properties to satisfy DJ012.



class Achievement(models.Model):
Expand Down
16 changes: 13 additions & 3 deletions web/templates/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,16 +721,26 @@ <h3 class="text-sm font-semibold mb-2 text-left">Quick Add YouTube Video</h3>
name="title"
placeholder="Video title "
class="w-full px-4 py-2 border rounded-lg focus:ring-2 focus:ring-blue-500 bg-white dark:bg-gray-800 text-gray-900 dark:text-gray-100" />
<!-- Hidden description (filled by JS) -->
<input type="hidden" id="video-description" name="description" value="" />
<!-- description -->
<label for="video-description"
class="block text-left text-sm text-gray-700 dark:text-gray-300 mb-1">
Video description
</label>
<textarea id="video-description"
name="description"
rows="3"
placeholder="Enter a description for the video"
class="w-full px-4 py-2 border rounded-lg focus:ring-2 focus:ring-blue-500 bg-white dark:bg-gray-800 text-gray-900 dark:text-gray-100"></textarea>
Comment on lines +724 to +733
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 25, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Align label/textarea styling with project form guidelines

  • Add font-medium to label per guidelines.
  • Use explicit Tailwind borders: border-gray-300 and dark:border-gray-600.
-                  <label for="video-description"
-                         class="block text-left text-sm text-gray-700 dark:text-gray-300 mb-1">
+                  <label for="video-description"
+                         class="block text-left text-sm font-medium text-gray-700 dark:text-gray-300 mb-1">
                     Video description
                   </label>
                   <textarea id="video-description"
                             name="description"
                             rows="3"
                             placeholder="Enter a description for the video"
-                            class="w-full px-4 py-2 border rounded-lg focus:ring-2 focus:ring-blue-500 bg-white dark:bg-gray-800 text-gray-900 dark:text-gray-100"></textarea>
+                            class="w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500 bg-white dark:bg-gray-800 text-gray-900 dark:text-gray-100"></textarea>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<!-- description -->
<label for="video-description"
class="block text-left text-sm text-gray-700 dark:text-gray-300 mb-1">
Video description
</label>
<textarea id="video-description"
name="description"
rows="3"
placeholder="Enter a description for the video"
class="w-full px-4 py-2 border rounded-lg focus:ring-2 focus:ring-blue-500 bg-white dark:bg-gray-800 text-gray-900 dark:text-gray-100"></textarea>
<!-- description -->
<label for="video-description"
class="block text-left text-sm font-medium text-gray-700 dark:text-gray-300 mb-1">
Video description
</label>
<textarea id="video-description"
name="description"
rows="3"
placeholder="Enter a description for the video"
class="w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500 bg-white dark:bg-gray-800 text-gray-900 dark:text-gray-100"></textarea>
🤖 Prompt for AI Agents 
In web/templates/index.html around lines 698 to 707, the label and textarea do
not follow project form styling: add the Tailwind class "font-medium" to the
label element, and replace the generic "border" class on the textarea with
explicit borders "border border-gray-300 dark:border-gray-600" (keeping existing
padding, rounded, focus and color classes intact) so the styling matches project
guidelines.

<!-- Category + submit -->
<div class="flex gap-2">
<select id="video-category"
name="category"
required
class="flex-1 px-4 py-2 border rounded-lg bg-white dark:bg-gray-800 text-gray-900 dark:text-gray-100 focus:ring-2 focus:ring-blue-500">
<option value="">Select a category</option>
{% for subject in subjects %}<option value="{{ subject.id }}">{{ subject.name }}</option>{% endfor %}
{% for subject in subjects %}
<option value="{{ subject.id }}">{{ subject.name }}</option>
{% endfor %}
Comment thread
abdelrahman390 marked this conversation as resolved.
</select>
<button type="submit"
class="bg-orange-500 hover:bg-orange-600 text-white px-4 py-2 rounded-lg flex items-center">
Expand Down
22 changes: 6 additions & 16 deletions web/templates/videos/list.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,21 +97,11 @@ <h1 class="text-2xl font-bold">
<img src="{{ video.thumbnail_url }}"
alt="{{ video.title }} thumbnail"
class="w-full h-full object-cover" />
<a href="{{ video.video_url }}"
target="_blank"
<a href="{% url 'play_youtube_video' video.video_id %}"
aria-label="Play video: {{ video.title }}"
class="absolute inset-0 flex items-center justify-center bg-black bg-opacity-25 hover:bg-opacity-0 transition-opacity">
<i class="fas fa-play text-white text-4xl"></i>
</a>
</div>
{% elif video.thumbnail_url %}
<div class="relative aspect-w-16 aspect-h-9 overflow-hidden rounded-lg shadow-sm">
<img src="{{ video.thumbnail_url }}"
alt="{{ video.title }} thumbnail"
class="w-full h-full object-cover" />
<a href="{{ video.video_url }}"
target="_blank"
class="absolute inset-0 flex items-center justify-center bg-black bg-opacity-25 hover:bg-opacity-0 transition-opacity">
<i class="fas fa-play text-white text-4xl"></i>
<i class="fas fa-play text-white text-4xl" aria-hidden="true"></i>
<span class="sr-only">Play video: {{ video.title }}</span>
</a>
</div>
{% elif 'vimeo.com' in video.video_url %}
Expand All @@ -123,8 +113,8 @@ <h1 class="text-2xl font-bold">
<!-- Video Info -->
<div class="p-4">
<h3 class="text-lg font-semibold mb-2 line-clamp-2">
<a href="{{ video.video_url }}"
target="_blank"
<a href="{% url 'play_youtube_video' video.video_id %}"
aria-label="Play video: {{ video.title }}"
class="hover:text-orange-500 transition-colors">{{ video.title }}</a>
</h3>
<div class="flex items-center text-sm text-gray-500 dark:text-gray-400 mb-2">
Expand Down
44 changes: 44 additions & 0 deletions web/templates/videos/play_youtube_video.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{% extends "base.html" %}

{% load static %}
{% load dict_filters %}

{% block title %}Educational Videos{% endblock %}
{% block content %}
<main class="flex-1 w-full max-w-7xl mx-auto px-4 md:px-6 py-8">
<div class="flex flex-col gap-6">
<div class="bg-white rounded-lg shadow-md p-6">
<h1 class="text-2xl font-bold mb-2">Title: {{ video.title }}</h1>
<!-- Video Description -->
{% if video.description %}
<div class="mt-4">
<h2 class="text-lg font-semibold mb-2">Description:</h2>
<p class="text-gray-700 whitespace-pre-line">{{ video.description }}</p>
</div>
Comment thread
abdelrahman390 marked this conversation as resolved.
{% endif %}
<!-- Additional Info -->
<div class="mt-4 mb-4 text-md text-gray-500">
<p>Category: {{ video.category.name }}</p>
<p>Uploaded: {{ video.uploaded_at|date:"F j, Y" }}</p>
</div>
Comment thread
abdelrahman390 marked this conversation as resolved.
<!-- YouTube Video Embed -->
<div class="relative w-full rounded-lg overflow-hidden aspect-w-16 aspect-h-9 mb-6">
{% if video.video_url %}
<iframe class=" w-full"
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

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

Extra space in class attribute should be removed.

Suggested change
<iframe class=" w-full"
<iframe class="w-full"

Copilot uses AI. Check for mistakes.
height="500"
src="https://www.youtube.com/embed/{{ video.youtube_id }}"
title="{{ video.title }}"
frameborder="0"
allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture"
allowfullscreen>
</iframe>
Comment thread
abdelrahman390 marked this conversation as resolved.
{% else %}
<div class="flex items-center justify-center bg-gray-200 w-full h-full">
<p class="text-gray-500">Video not available</p>
</div>
{% endif %}
</div>
</div>
</div>
</main>
{% endblock %}
1 change: 1 addition & 0 deletions web/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@
path("challenges/<int:challenge_id>/submit/", views.challenge_submit, name="challenge_submit"),
path("current-weekly-challenge/", views.current_weekly_challenge, name="current_weekly_challenge"),
# Educational Videos URLs
path("videos/play/<str:video_id>/", views.play_youtube_video, name="play_youtube_video"),
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 25, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Constrain route param to internal 12-hex video_id

The route uses our internal video_id (uuid.hex[:12]), not the 11-char YouTube ID. Tighten the pattern to lowercase hex for input hygiene and to avoid accidental matches.

-from django.urls import include, path
+from django.urls import include, path, re_path
@@
-    path("videos/play/<str:video_id>/", views.play_youtube_video, name="play_youtube_video"),
+    re_path(
+        r"^videos/play/(?P<video_id>[0-9a-f]{12})/$",
+        views.play_youtube_video,
+        name="play_youtube_video",
+    ),

Optional hardening (view): normalize incoming IDs to lowercase before querying to prevent case-mismatch misses.

# web/views.py
def play_youtube_video(request: HttpRequest, video_id: str) -> HttpResponse:
    video = EducationalVideo.objects.filter(video_id=video_id.lower()).first()
    ...

🏁 Script executed:

#!/bin/bash
# Ensure all template calls pass `video.video_id` (not youtube_id) to the route
rg -nP -C2 "{% url ['\"]play_youtube_video['\"]\s+" --type=html web/templates

Length of output: 1191

Constrain video_id route to internal 12-hex ID

Verification confirms that all template calls already pass video.video_id (not the YouTube ID) to the play_youtube_video route:

  • web/urls.py (line 304) – current path("videos/play/<str:video_id>/", …)
  • web/templates/videos/list.html – both occurrences on lines 100 and 116 use video.video_id

Proceed with the essential refactor to tighten the URL pattern:

• In web/urls.py replace the existing path(...) with a re_path(...) using a lowercase-hex 12-character constraint:

-from django.urls import include, path
+from django.urls import include, path, re_path
@@
-    path("videos/play/<str:video_id>/", views.play_youtube_video, name="play_youtube_video"),
+    re_path(
+        r"^videos/play/(?P<video_id>[0-9a-f]{12})/$",
+        views.play_youtube_video,
+        name="play_youtube_video",
+    ),

• Optional hardening in web/views.py: normalize incoming IDs to lowercase before querying to avoid case-mismatch misses.

def play_youtube_video(request: HttpRequest, video_id: str) -> HttpResponse:
    video = EducationalVideo.objects.filter(video_id=video_id.lower()).first()
    ...
🤖 Prompt for AI Agents 
In web/urls.py around line 304 the route uses
path("videos/play/<str:video_id>/", ...) which accepts any string; replace it
with a re_path that restricts video_id to a 12-character lowercase hex ID
(regex: r"^videos/play/(?P<video_id>[0-9a-f]{12})/$") so only internal IDs
match, and update imports to include re_path; optionally, in web/views.py
normalize the incoming video_id to lowercase before querying (e.g., video_id =
video_id.lower()) to avoid case-mismatch when looking up EducationalVideo.

path("videos/", views.educational_videos_list, name="educational_videos_list"),
path("videos/upload/", views.upload_educational_video, name="upload_educational_video"),
path("fetch-video-title/", views.fetch_video_title, name="fetch_video_title"),
Expand Down
16 changes: 16 additions & 0 deletions web/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5994,6 +5994,22 @@ def upload_educational_video(request):
return render(request, "videos/upload.html", {"form": form})


def play_youtube_video(request: HttpRequest, video_id: str) -> HttpResponse:
# Display a YouTube video in the internal player page.

Comment on lines +5997 to +5999
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 25, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Constrain to GET and add a concise docstring.

This is a read-only page; be explicit about the allowed method and document the intent.

+@require_GET
 def play_youtube_video(request: HttpRequest, video_id: str) -> HttpResponse:
-    # Display a YouTube video in the internal player page.
+    """Display a YouTube video in the internal player page."""
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def play_youtube_video(request: HttpRequest, video_id: str) -> HttpResponse:
# Display a YouTube video in the internal player page.
@require_GET
def play_youtube_video(request: HttpRequest, video_id: str) -> HttpResponse:
"""Display a YouTube video in the internal player page."""
🤖 Prompt for AI Agents 
In web/views.py around lines 5366 to 5368, the play_youtube_video view lacks an
explicit HTTP method constraint and a concise docstring; add the @require_GET
decorator (from django.views.decorators.http) above the function to restrict it
to GET requests and add a one-line docstring under the def that states its
purpose (e.g., "Render internal player page for a given YouTube video ID.").
Ensure you import require_GET at the top of the file if not already present.

video = EducationalVideo.objects.filter(video_id=video_id).first()
Comment thread
abdelrahman390 marked this conversation as resolved.

if not video:
messages.error(request, "The requested video could not be found.")
return redirect("educational_videos_list")

Comment on lines +6000 to +6005
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 25, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Prefer get_object_or_404 and select_related for correctness and efficiency.

Use the model’s uniqueness on video_id and fail fast with a 404 while also reducing potential N+1s in the template.

Apply this diff:

-    video = EducationalVideo.objects.filter(video_id=video_id).first()
-
-    if not video:
-        messages.error(request, "The requested video could not be found.")
-        return redirect("educational_videos_list")
+    video = get_object_or_404(
+        EducationalVideo.objects.select_related("uploader", "category"),
+        video_id=video_id,
+    )

If you strongly prefer a redirect with a user-facing message instead of 404, keep the current flow but still add select_related("uploader", "category") to cut extra queries.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
video = EducationalVideo.objects.filter(video_id=video_id).first()
if not video:
messages.error(request, "The requested video could not be found.")
return redirect("educational_videos_list")
video = get_object_or_404(
EducationalVideo.objects.select_related("uploader", "category"),
video_id=video_id,
)
🤖 Prompt for AI Agents 
In web/views.py around lines 5369-5374, replace the current filter(...).first()
+ redirect pattern with Django's get_object_or_404 to enforce the model
uniqueness and return a 404 on missing video, and call
select_related("uploader", "category") when retrieving the object to avoid N+1
queries in the template; if you must preserve the user-facing redirect flow
instead of raising 404, keep the redirect but still use
.select_related("uploader", "category") on the query to cut extra DB queries.

Comment on lines +6002 to +6005
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 25, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Internationalize the user-facing error message (if you keep the redirect).

Other views use gettext; keep consistency for translations.

-        messages.error(request, "The requested video could not be found.")
+        messages.error(request, _("The requested video could not be found."))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if not video:
messages.error(request, "The requested video could not be found.")
return redirect("educational_videos_list")
if not video:
messages.error(request, _("The requested video could not be found."))
return redirect("educational_videos_list")
🤖 Prompt for AI Agents 
In web/views.py around lines 5371 to 5374, the user-facing error message is not
internationalized; wrap the string in Django's gettext (e.g. use _("The
requested video could not be found.")) so it is translatable and keep the
redirect. Also ensure django.utils.translation.gettext (or gettext_lazy) is
imported at the top of the file (add the import if missing) before using _().

context = {
"video": video,
}

return render(request, "videos/play_youtube_video.html", context)

Comment on lines +5997 to +6011
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 25, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Optional: access control hook for future premium/private videos.

If visibility restrictions are planned, add a simple guard (e.g., require login for premium or draft videos). Keep it minimal and feature-flagged to avoid scope creep now.

I can sketch a small policy function (e.g., can_view_video(request.user, video)) and wire it here if you want.

🤖 Prompt for AI Agents 
In web/views.py around lines 5366 to 5380, add a minimal, feature-flagged
access-control check before rendering the video: call a helper like
can_view_video(user, video) (implement it to return False for premium/draft
videos unless user.is_authenticated or user.has_premium), gate the check behind
a settings flag (e.g., ENABLE_VIDEO_VISIBILITY_CHECKS) so behavior is opt-in,
and if the check fails either redirect to login with a message or return
HttpResponseForbidden; keep the helper small and colocated (or in the same
module) so it can be expanded later.

💡 Verification agent

🧩 Analysis chain

Verify the template handles non‑YouTube URLs gracefully.

Model property EducationalVideo.youtube_id can be None for non‑YouTube links. Ensure videos/play_youtube_video.html renders a fallback (e.g., external link or a “not embeddable” message) instead of breaking.

Run this to confirm the template guards on video.youtube_id and provides a fallback:

🏁 Script executed:

#!/bin/bash
# Locate the play template and inspect usage of youtube_id and fallback branches
set -euo pipefail
fd -a 'play_youtube_video.html'
rg -n -C3 'youtube_id|iframe|embed' $(fd -a 'play_youtube_video.html')

Length of output: 1032

Guard on YouTube ID and provide proper fallback in the play template

The current template only checks video.video_url, which means that for non-YouTube videos (where video.youtube_id is None) it will still attempt to render an empty YouTube embed and break the page. You should update play_youtube_video.html to first verify video.youtube_id, then fall back to an external link if available, and finally show “not available” when neither is present.

• File: web/templates/videos/play_youtube_video.html (around lines 26–35)
Replace this block:

{% if video.video_url %}
  <iframe class="w-full"
          height="500"
          src="https://www.youtube.com/embed/{{ video.youtube_id }}"
          title="{{ video.title }}"
          frameborder="0"
          allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture"
          allowfullscreen>
  </iframe>
{% else %}
  <div class="flex items-center justify-center bg-gray-200 w-full h-full">
    <p class="text-gray-500">Video not available</p>
  </div>
{% endif %}

With this guarded, multi-branch logic:

{% if video.youtube_id %}
  <iframe class="w-full"
          height="500"
          src="https://www.youtube.com/embed/{{ video.youtube_id }}"
          title="{{ video.title }}"
          frameborder="0"
          allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture"
          allowfullscreen>
  </iframe>
{% elif video.video_url %}
  <div class="flex items-center justify-center bg-gray-100 w-full h-full">
    <a href="{{ video.video_url }}" target="_blank" rel="noopener" class="text-blue-600 underline">
      Watch this video externally
    </a>
  </div>
{% else %}
  <div class="flex items-center justify-center bg-gray-200 w-full h-full">
    <p class="text-gray-500">Video not available</p>
  </div>
{% endif %}

This change ensures:

  • YouTube videos embed only when youtube_id is present
  • Non-YouTube videos link out instead of breaking
  • A clear “not available” message appears only if there’s truly no URL to show
🤖 Prompt for AI Agents 
In web/templates/videos/play_youtube_video.html around lines 26–35, the template
currently checks only video.video_url so pages with no youtube_id render an
empty YouTube iframe; update the template to first check if video.youtube_id and
render the YouTube iframe only in that case, then elif video.video_url render an
external link (anchor with target="_blank" and rel="noopener") to watch
externally, and finally else render the “Video not available” placeholder so
non-YouTube videos don’t break the page and truly missing videos show a clear
fallback.


def certificate_detail(request, certificate_id):
certificate = get_object_or_404(Certificate, certificate_id=certificate_id)
if request.user != certificate.user and not request.user.is_staff:
Expand Down
Loading