Skip to content
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

feat: add django templates for tagging app UI #4547

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ali-D-Akbar
Copy link
Contributor

@Ali-D-Akbar Ali-D-Akbar commented Jan 20, 2025

PROD-4288

Adds UI the following pages.
Be sure to install requirements and visit http://localhost:18381/tagging/courses/ to test the behaviors locally.

Screenshot 2025-01-22 at 11 13 35 PM Screenshot 2025-01-22 at 11 13 50 PM Screenshot 2025-01-22 at 11 14 00 PM Screenshot 2025-01-22 at 11 14 14 PM Screenshot 2025-01-22 at 11 14 30 PM Screenshot 2025-01-22 at 11 14 43 PM

class CourseVerticalForm(forms.ModelForm):
class Meta:
model = CourseVertical
fields = ['vertical', 'sub_vertical']
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the course field not available in this form? Or is it used for editing purposes only?

<title>{% block title %}Tagging App{% endblock %}</title>
<link
href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css"
rel="stylesheet"
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you verified bootstrap added here is working correctly?

@Ali-D-Akbar Ali-D-Akbar marked this pull request as ready for review January 22, 2025 18:17
Comment on lines +5 to +35
<th>
<a href="?sort=key&direction={% if current_sort == 'key' and current_direction == 'asc' %}desc{% else %}asc{% endif %}">
Course Key
{% if current_sort == 'key' %}
<span>{% if current_direction == 'asc' %}▲{% else %}▼{% endif %}</span>
{% endif %}
</a>
</th>
<th>
<a href="?sort=title&direction={% if current_sort == 'title' and current_direction == 'asc' %}desc{% else %}asc{% endif %}">
Course Title
{% if current_sort == 'title' %}
<span>{% if current_direction == 'asc' %}▲{% else %}▼{% endif %}</span>
{% endif %}
</a>
</th>
<th>
<a href="?sort=vertical&direction={% if current_sort == 'vertical' and current_direction == 'asc' %}desc{% else %}asc{% endif %}">
Vertical
{% if current_sort == 'vertical' %}
<span>{% if current_direction == 'asc' %}▲{% else %}▼{% endif %}</span>
{% endif %}
</a>
</th>
<th>
<a href="?sort=sub_vertical&direction={% if current_sort == 'sub_vertical' and current_direction == 'asc' %}desc{% else %}asc{% endif %}">
Sub-Vertical
{% if current_sort == 'sub_vertical' %}
<span>{% if current_direction == 'asc' %}▲{% else %}▼{% endif %}</span>
{% endif %}
</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a lot of repeated code here. I think you can define a list and loop over it instead of having to write the same thing with only field name changes. Eager to see what others have to say. As an opposing arg, the table headers are "fixed", so the loop/general mechanism might not "give" much.

</td>
<td>
{% if course.vertical and course.vertical.sub_vertical %}
<a href="{% url 'tagging:sub_vertical_detail' slug=course.vertical.sub_vertical.slug %}">
Copy link
Contributor

Choose a reason for hiding this comment

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

since subvertical is also linked in CourseVertical, you can use course.subvertical.slug instead of getting it from vertical.

<option value="">-- Select Vertical --</option>
{% for vertical in verticals %}
<option value="{{ vertical.slug }}"
{% if course.vertical and course.vertical.vertical.slug == vertical.slug %}selected{% endif %}>
Copy link
Contributor

Choose a reason for hiding this comment

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

course.vertical.vertical?

response = self.client.get(self.url)
self.assertEqual(response.status_code, 200)
self.assertTemplateUsed(response, 'tagging/course_list.html')
self.assertIn('courses', response.context)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should also check for course keys or titles

self.assertNotContains(response, 'Advanced Python')

def test_sort_courses(self):
"""Tests sorting courses by title in descending order."""
Copy link
Contributor

Choose a reason for hiding this comment

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

This test can be a ddt to check for both directions.


def course_detail(request, uuid):
"""
Display details of a specific course.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is only meant for tagging, it does not display anything else.

})


def load_sub_verticals(request):
Copy link
Contributor

Choose a reason for hiding this comment

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

Left a comment about this in urls.py.

page_number = request.GET.get('page')
page_obj = paginator.get_page(page_number)

if request.headers.get('HX-Request'):
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

})


def vertical_list(request):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the views are typically verbs/actions, so we can rename them like list_Verticals, list_subverticals, etc.
Plus, it would be good to use Django's list or detail view where needed.

@@ -0,0 +1,159 @@
from django.core.paginator import Paginator
Copy link
Contributor

Choose a reason for hiding this comment

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

The views are missing authentication and permissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants