Skip to content

Commit

Permalink
feat: removing visible_date attribute (#2511)
Browse files Browse the repository at this point in the history
removing the reliance on the visible_date attribute

* re-factoring some tests, not just to make them work with the new code,  but also because some of these tests were never ideal. They weren't  all testing the right things, some of them never tested the code path that used certificate available date, and definitely a bunch have  non-useful names and no comments. Several were testing impossible (or, definitionally broken) situations like program certificates with no corresponding course certificates.
* added django stubs for type checking
* added some type checking
* modified a test not only to work with the new code but also to do a   better job of validating what was looking for
* added mypy
* told coverage not to look for type checking conditionals
* all tests now use certificate available date (which has another pending ticket to be removed as a toggle)
* upgraded the Python version used to build the documentation because accessible-pygments  requires a version greater than 3.8.
* updating one more github CI python version
  • Loading branch information
deborahgu authored Jul 1, 2024
1 parent 588ab9a commit d9808ca
Show file tree
Hide file tree
Showing 22 changed files with 273 additions and 499 deletions.
4 changes: 4 additions & 0 deletions .coveragerc
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,7 @@ omit =
*admin.py
*static*
*templates*
[report]
exclude_lines =
pragma: no cover
if TYPE_CHECKING:
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,13 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ["py38"]
python-version: ["py311"]
django-version: ["django42"]
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: "3.8"
python-version: "3.11"
architecture: x64
- name: Setup Nodejs Env
run: echo "NODE_VER=`cat .nvmrc`" >> $GITHUB_ENV
Expand Down Expand Up @@ -106,7 +106,7 @@ jobs:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: "3.8"
python-version: "3.11"
architecture: x64
- name: Install Dependencies
run: make requirements
Expand Down
39 changes: 1 addition & 38 deletions credentials/apps/api/v2/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def test_authentication(self):

def test_create(self):
program = ProgramFactory(site=self.site)
program_certificate = ProgramCertificateFactory(site=self.site, program_uuid=program.uuid)
program_certificate = ProgramCertificateFactory(site=self.site, program_uuid=program.uuid, program=program)
expected_username = self.user.username
expected_attribute_name = "fake-name"
expected_attribute_value = "fake-value"
Expand Down Expand Up @@ -356,43 +356,6 @@ def test_list_type_filtering(self):
self.assertEqual(response.status_code, 200)
self.assertEqual(response.data["results"], self.serialize_user_credential([program_cred], many=True))

# This test should be removed in MICROBA-1198 in favor of the next test.
def test_list_visible_filtering(self):
"""Verify the endpoint can filter by visible date."""
program_certificate = ProgramCertificateFactory(site=self.site)
course_run = CourseRunFactory()
course_certificate = CourseCertificateFactory(course_id=course_run.key, course_run=course_run, site=self.site)

course_cred = UserCredentialFactory(credential=course_certificate)
program_cred = UserCredentialFactory(credential=program_certificate)

UserCredentialAttributeFactory(
user_credential=program_cred,
name="visible_date",
value="9999-01-01T01:01:01Z",
)

self.authenticate_user(self.user)
self.add_user_permission(self.user, "view_usercredential")

both = [course_cred, program_cred]

response = self.client.get(self.list_path)
self.assertEqual(response.status_code, 200)
self.assertEqual(response.data["results"], self.serialize_user_credential(both, many=True))

response = self.client.get(self.list_path + "?only_visible=True")
self.assertEqual(response.status_code, 200)
self.assertEqual(response.data["results"], self.serialize_user_credential([course_cred], many=True))

response = self.client.get(self.list_path + "?only_visible=False")
self.assertEqual(response.status_code, 200)
self.assertEqual(response.data["results"], self.serialize_user_credential(both, many=True))

# The following test is the same as the previous test, but with the
# USE_CERTIFICATE_AVAILABLE_DATE waffle switch enabled. Clean up previous
# tests in MICROBA-1198.
@override_switch("credentials.use_certificate_available_date", active=True)
def test_list_visible_filtering_with_certificate_available_date(self):
"""Verify the endpoint can filter by visible date."""
course = CourseFactory.create(site=self.site)
Expand Down
5 changes: 1 addition & 4 deletions credentials/apps/credentials/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,10 @@


if TYPE_CHECKING:
from django.contrib.auth import get_user_model
from django.contrib.sites.models import Site

from credentials.apps.catalog.models import CourseRun

User = get_user_model()


logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -235,7 +232,7 @@ def get_credential_dates(user_credentials, many):
return get_credential_visible_date(user_credentials, use_date_override=True)


def process_course_credential_update(user: "User", course_run_key: str, mode: str, credential_status: str) -> None:
def process_course_credential_update(user, course_run_key: str, mode: str, credential_status: str) -> None:
"""
A utility function responsible for creating or updating a course credential associated with a learner. Primarily
used when consuming events from the Event Bus.
Expand Down
32 changes: 21 additions & 11 deletions credentials/apps/credentials/tests/test_api.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from unittest.mock import patch

import ddt
from ddt import unpack
from django.contrib.contenttypes.models import ContentType
from django.test import TestCase
from testfixtures import LogCapture
Expand Down Expand Up @@ -156,7 +155,9 @@ def setUp(self):
)
for course_run in self.course_runs
]
self.program_cert = ProgramCertificateFactory.create(program_uuid=self.program.uuid, site=self.site)
self.program_cert = ProgramCertificateFactory.create(
program_uuid=self.program.uuid, site=self.site, program=self.program
)
self.course_credential_content_type = ContentType.objects.get(
app_label="credentials", model="coursecertificate"
)
Expand All @@ -177,7 +178,18 @@ def setUp(self):
credential=self.program_cert,
)

def test_get_user_credentials_by_content_type_zero(self):
def test_get_user_credentials_by_content_type_when_no_valid_types(self):
"""get_user_credentials_by_content_type returns empty when there's no creds of the type"""
course_cert_content_types = ContentType.objects.filter(app_label="credentials", model__in=["goldstar"])
for course_user_credential in self.course_user_credentials:
course_user_credential.delete()
result = get_user_credentials_by_content_type(
self.user.username, course_cert_content_types, UserCredentialStatus.AWARDED.value
)
assert len(result) == 0

def test_get_user_credentials_by_content_type_when_no_creds(self):
"""get_user_credentials_by_content_type returns empty when there's no applicable creds"""
course_cert_content_types = ContentType.objects.filter(
app_label="credentials", model__in=["coursecertificate", "programcertificate"]
)
Expand All @@ -190,10 +202,8 @@ def test_get_user_credentials_by_content_type_zero(self):
assert len(result) == 0

def test_get_user_credentials_by_content_type_course_only(self):
course_cert_content_types = ContentType.objects.filter(
app_label="credentials", model__in=["coursecertificate", "programcertificate"]
)
self.program_user_credential.delete()
"""get_user_credentials_by_content_type returns course certificates when asked"""
course_cert_content_types = ContentType.objects.filter(app_label="credentials", model__in=["coursecertificate"])
result = get_user_credentials_by_content_type(
self.user.username, course_cert_content_types, UserCredentialStatus.AWARDED.value
)
Expand All @@ -202,18 +212,18 @@ def test_get_user_credentials_by_content_type_course_only(self):
assert result[1] == self.course_user_credentials[1]

def test_get_user_credentials_by_content_type_program_only(self):
"""get_user_credentials_by_content_type returns program certificates when asked"""
course_cert_content_types = ContentType.objects.filter(
app_label="credentials", model__in=["coursecertificate", "programcertificate"]
app_label="credentials", model__in=["programcertificate"]
)
for course_user_credential in self.course_user_credentials:
course_user_credential.delete()
result = get_user_credentials_by_content_type(
self.user.username, course_cert_content_types, UserCredentialStatus.AWARDED.value
)
assert len(result) == 1
assert result[0] == self.program_user_credential

def test_get_user_credentials_by_content_type_course_and_program(self):
"""get_user_credentials_by_content_type returns courses and programs when asked"""
course_cert_content_types = ContentType.objects.filter(
app_label="credentials", model__in=["coursecertificate", "programcertificate"]
)
Expand Down Expand Up @@ -292,7 +302,7 @@ def setUp(self):
[ProgramCertificate, UserCredentialStatus.AWARDED],
[ProgramCertificate, UserCredentialStatus.REVOKED],
)
@unpack
@ddt.unpack
def test_create_credential(self, credential_type, cert_status):
"""
Happy path. This test verifies the functionality of the `_update_or_create_credentials` utility function.
Expand Down
17 changes: 1 addition & 16 deletions credentials/apps/credentials/tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import datetime
import textwrap
from unittest import mock

Expand All @@ -15,11 +14,7 @@
from credentials.apps.core.tests.mixins import SiteMixin
from credentials.apps.credentials.models import ProgramCompletionEmailConfiguration
from credentials.apps.credentials.tests.factories import ProgramCertificateFactory
from credentials.apps.credentials.utils import (
datetime_from_visible_date,
send_program_certificate_created_message,
validate_duplicate_attributes,
)
from credentials.apps.credentials.utils import send_program_certificate_created_message, validate_duplicate_attributes


User = get_user_model()
Expand Down Expand Up @@ -47,16 +42,6 @@ def test_with_duplicate_attributes(self):

self.assertFalse(validate_duplicate_attributes(attributes))

def test_datetime_from_visible_date(self):
"""Verify that we convert LMS dates correctly."""
self.assertIsNone(datetime_from_visible_date(""))
self.assertIsNone(datetime_from_visible_date("2018-07-31"))
self.assertIsNone(datetime_from_visible_date("2018-07-31T09:32:46+00:00")) # should be Z for timezone
self.assertEqual(
datetime_from_visible_date("2018-07-31T09:32:46Z"),
datetime.datetime(2018, 7, 31, 9, 32, 46, tzinfo=datetime.timezone.utc),
)


@override_settings(EMAIL_BACKEND="django.core.mail.backends.locmem.EmailBackend")
class ProgramCertificateIssuedEmailTests(SiteMixin, TestCase):
Expand Down
Loading

0 comments on commit d9808ca

Please sign in to comment.