diff --git a/backend/src/apps/github/api/internal/nodes/issue.py b/backend/src/apps/github/api/internal/nodes/issue.py index c603887d5f..a2e45c98fa 100644 --- a/backend/src/apps/github/api/internal/nodes/issue.py +++ b/backend/src/apps/github/api/internal/nodes/issue.py @@ -99,7 +99,7 @@ def task_deadline(self, root: Issue, info: Info) -> datetime | None: """Return the deadline for the latest assigned task linked to this issue.""" mapping = getattr(info.context, "task_deadlines_by_issue", None) if mapping is not None: - return mapping.get(root.number) + return mapping.get(root.pk) return ( Task.objects.filter( diff --git a/backend/src/apps/mentorship/api/internal/mutations/module.py b/backend/src/apps/mentorship/api/internal/mutations/module.py index 0825f754c7..1ef659a711 100644 --- a/backend/src/apps/mentorship/api/internal/mutations/module.py +++ b/backend/src/apps/mentorship/api/internal/mutations/module.py @@ -9,6 +9,7 @@ from django.db.utils import IntegrityError from django.utils import timezone from graphql import GraphQLError +from strawberry.relay import GlobalID from apps.github.models import User as GithubUser from apps.mentorship.api.internal.nodes.module import ( @@ -17,6 +18,7 @@ ReorderModulesInput, UpdateModuleInput, ) +from apps.mentorship.api.internal.utils import get_issue_pk from apps.mentorship.models import Mentor, Module, Program from apps.mentorship.models.issue_user_interest import IssueUserInterest from apps.mentorship.models.task import Task @@ -152,7 +154,7 @@ def assign_issue_to_user( *, module_key: str, program_key: str, - issue_number: int, + issue_id: GlobalID, user_login: str, ) -> ModuleNode: """Assign an issue to a user by updating Issue.assignees within the module scope.""" @@ -173,7 +175,7 @@ def assign_issue_to_user( if gh_user is None: raise ObjectDoesNotExist(ASSIGNEE_NOT_FOUND_MSG) - issue = module.issues.filter(number=issue_number).first() + issue = module.issues.filter(pk=get_issue_pk(issue_id)).first() if issue is None: raise ObjectDoesNotExist(ISSUE_NOT_FOUND_MSG) @@ -191,7 +193,7 @@ def unassign_issue_from_user( *, module_key: str, program_key: str, - issue_number: int, + issue_id: GlobalID, user_login: str, ) -> ModuleNode: """Unassign an issue from a user by updating Issue.assignees within the module scope.""" @@ -212,9 +214,9 @@ def unassign_issue_from_user( if gh_user is None: raise ObjectDoesNotExist(ASSIGNEE_NOT_FOUND_MSG) - issue = module.issues.filter(number=issue_number).first() + issue = module.issues.filter(pk=get_issue_pk(issue_id)).first() if issue is None: - msg = f"Issue {issue_number} not found in this module." + msg = "Issue not found in this module." raise ObjectDoesNotExist(msg) issue.assignees.remove(gh_user) @@ -229,7 +231,7 @@ def set_task_deadline( *, module_key: str, program_key: str, - issue_number: int, + issue_id: GlobalID, deadline_at: datetime, ) -> ModuleNode: """Set a deadline for a task. @@ -260,7 +262,7 @@ def set_task_deadline( issue = ( module.issues.select_related("repository") .prefetch_related("assignees") - .filter(number=issue_number) + .filter(pk=get_issue_pk(issue_id)) .first() ) if issue is None: @@ -305,7 +307,7 @@ def clear_task_deadline( *, module_key: str, program_key: str, - issue_number: int, + issue_id: GlobalID, ) -> ModuleNode: """Clear the deadline for a task. @@ -327,7 +329,7 @@ def clear_task_deadline( issue = ( module.issues.select_related("repository") .prefetch_related("assignees") - .filter(number=issue_number) + .filter(pk=get_issue_pk(issue_id)) .first() ) if issue is None: diff --git a/backend/src/apps/mentorship/api/internal/nodes/module.py b/backend/src/apps/mentorship/api/internal/nodes/module.py index 826d5cb81b..09ba7d0513 100644 --- a/backend/src/apps/mentorship/api/internal/nodes/module.py +++ b/backend/src/apps/mentorship/api/internal/nodes/module.py @@ -3,6 +3,7 @@ from datetime import datetime import strawberry +from strawberry.relay import GlobalID from strawberry.types import Info from apps.common.utils import normalize_limit @@ -15,6 +16,7 @@ from apps.mentorship.api.internal.nodes.enum import ExperienceLevelEnum from apps.mentorship.api.internal.nodes.mentor import MentorNode from apps.mentorship.api.internal.nodes.program import ProgramNode +from apps.mentorship.api.internal.utils import get_issue_pk from apps.mentorship.models.issue_user_interest import IssueUserInterest from apps.mentorship.models.task import Task @@ -64,9 +66,11 @@ def mentees(self) -> list[UserNode]: return list(User.objects.filter(id__in=mentee_users).order_by("login")) @strawberry.field - def issue_mentees(self, issue_number: int) -> list[UserNode]: - """Return mentees assigned to this module's issue identified by its number.""" - issue_ids = list(self.issues.filter(number=issue_number).values_list("id", flat=True)) + def issue_mentees(self, issue_id: GlobalID) -> list[UserNode]: + """Return mentees assigned to this module's issue identified by its Nest ID.""" + issue_ids = list( + self.issues.filter(pk=get_issue_pk(issue_id)).values_list("id", flat=True) + ) if not issue_ids: return [] @@ -103,26 +107,26 @@ def issues( # BULK load data deadline_rows = ( Task.objects.filter(module=self, deadline_at__isnull=False) - .order_by("issue__number", "-assigned_at") - .values("issue__number", "deadline_at") + .order_by("issue_id", "-assigned_at") + .values("issue_id", "deadline_at") ) assigned_rows = ( Task.objects.filter(module=self, assigned_at__isnull=False) - .order_by("issue__number", "-assigned_at") - .values("issue__number", "assigned_at") + .order_by("issue_id", "-assigned_at") + .values("issue_id", "assigned_at") ) deadline_map = {} assigned_map = {} for row in deadline_rows: - num = row["issue__number"] - if num not in deadline_map: - deadline_map[num] = row["deadline_at"] + issue_pk = row["issue_id"] + if issue_pk not in deadline_map: + deadline_map[issue_pk] = row["deadline_at"] for row in assigned_rows: - num = row["issue__number"] - if num not in assigned_map: - assigned_map[num] = row["assigned_at"] + issue_pk = row["issue_id"] + if issue_pk not in assigned_map: + assigned_map[issue_pk] = row["assigned_at"] info.context.task_deadlines_by_issue = deadline_map info.context.task_assigned_at_by_issue = assigned_map @@ -182,8 +186,8 @@ def available_labels(self) -> list[str]: return sorted(label_names) @strawberry.field - def issue_by_number(self, info: Info, number: int) -> IssueNode | None: - """Return a single issue by its GitHub number within this module's linked issues.""" + def issue_by_id(self, info: Info, issue_id: GlobalID) -> IssueNode | None: + """Return a single issue by its Nest ID within this module's linked issues.""" user = info.context.request.user is_admin_or_mentor = self.program and ( self.program.has_admin(user) or self.has_mentor(user) @@ -201,7 +205,7 @@ def issue_by_number(self, info: Info, number: int) -> IssueNode | None: "labels", MERGED_PULL_REQUESTS_PREFETCH, ) - .filter(number=number) + .filter(pk=get_issue_pk(issue_id)) ) if is_mentee and not is_admin_or_mentor: @@ -213,9 +217,11 @@ def issue_by_number(self, info: Info, number: int) -> IssueNode | None: return issues.first() @strawberry.field - def interested_users(self, issue_number: int) -> list[UserNode]: - """Return users interested in this module's issue identified by its number.""" - issue_ids = list(self.issues.filter(number=issue_number).values_list("id", flat=True)) + def interested_users(self, issue_id: GlobalID) -> list[UserNode]: + """Return users interested in this module's issue identified by its Nest ID.""" + issue_ids = list( + self.issues.filter(pk=get_issue_pk(issue_id)).values_list("id", flat=True) + ) if not issue_ids: return [] interests = ( @@ -226,17 +232,17 @@ def interested_users(self, issue_number: int) -> list[UserNode]: return [i.user for i in interests] @strawberry.field - def task_deadline(self, info: Info, issue_number: int) -> datetime | None: + def task_deadline(self, info: Info, issue_id: GlobalID) -> datetime | None: """Return the deadline for the latest assigned task linked to this module and issue.""" mapping = getattr(info.context, "task_deadlines_by_issue", None) if mapping is not None: - return mapping.get(issue_number) + return mapping.get(get_issue_pk(issue_id)) # fallback (single issue query) return ( Task.objects.filter( module=self, - issue__number=issue_number, + issue_id=get_issue_pk(issue_id), deadline_at__isnull=False, ) .order_by("-assigned_at") @@ -245,16 +251,16 @@ def task_deadline(self, info: Info, issue_number: int) -> datetime | None: ) @strawberry.field - def task_assigned_at(self, info: Info, issue_number: int) -> datetime | None: + def task_assigned_at(self, info: Info, issue_id: GlobalID) -> datetime | None: """Return the latest assignment time for tasks linked to this module and issue.""" mapping = getattr(info.context, "task_assigned_at_by_issue", None) if mapping is not None: - return mapping.get(issue_number) + return mapping.get(get_issue_pk(issue_id)) return ( Task.objects.filter( module=self, - issue__number=issue_number, + issue_id=get_issue_pk(issue_id), assigned_at__isnull=False, ) .order_by("-assigned_at") diff --git a/backend/src/apps/mentorship/api/internal/utils.py b/backend/src/apps/mentorship/api/internal/utils.py new file mode 100644 index 0000000000..72998d2880 --- /dev/null +++ b/backend/src/apps/mentorship/api/internal/utils.py @@ -0,0 +1,20 @@ +"""Helpers for mentorship GraphQL issue identifiers.""" + +from graphql import GraphQLError +from strawberry.relay import GlobalID + + +def get_issue_pk(issue_id: GlobalID) -> int: + """Validate a Nest issue Relay ID and return its database primary key.""" + if issue_id.type_name != "IssueNode" or not issue_id.node_id.isdecimal(): + msg = "Invalid issue ID." + raise GraphQLError(msg) + try: + issue_pk = int(issue_id.node_id) + except ValueError as exc: + msg = "Invalid issue ID." + raise GraphQLError(msg) from exc + if issue_pk > 2**63 - 1: + msg = "Invalid issue ID." + raise GraphQLError(msg) + return issue_pk diff --git a/backend/tests/unit/apps/github/api/internal/nodes/issue_test.py b/backend/tests/unit/apps/github/api/internal/nodes/issue_test.py index fd1b004d52..fde163b93e 100644 --- a/backend/tests/unit/apps/github/api/internal/nodes/issue_test.py +++ b/backend/tests/unit/apps/github/api/internal/nodes/issue_test.py @@ -173,7 +173,8 @@ def test_task_deadline_with_bulk_load_mapping(self): """Test task_deadline field when mapping exists with issue deadline.""" expected_date = datetime(2025, 10, 26, tzinfo=UTC) mock_issue = Mock() - mock_issue.number = 123 + mock_issue.number = 42 + mock_issue.pk = 123 mock_info = Mock() mock_info.context = Mock() @@ -186,7 +187,8 @@ def test_task_deadline_with_bulk_load_mapping(self): def test_task_deadline_with_bulk_load_mapping_no_deadline(self): """Test task_deadline field when mapping exists but issue not in mapping.""" mock_issue = Mock() - mock_issue.number = 123 + mock_issue.number = 42 + mock_issue.pk = 123 mock_info = Mock() mock_info.context = Mock() @@ -200,7 +202,8 @@ def test_task_deadline_without_bulk_load_mapping(self): """Test task_deadline field when no mapping exists (fallback to query).""" expected_date = datetime(2025, 10, 26, tzinfo=UTC) mock_issue = Mock() - mock_issue.number = 123 + mock_issue.number = 42 + mock_issue.pk = 123 mock_info = Mock() mock_info.context = Mock() diff --git a/backend/tests/unit/apps/mentorship/api/internal/mutations/module_mutation_test.py b/backend/tests/unit/apps/mentorship/api/internal/mutations/module_mutation_test.py index dbabe27268..aab78f304b 100644 --- a/backend/tests/unit/apps/mentorship/api/internal/mutations/module_mutation_test.py +++ b/backend/tests/unit/apps/mentorship/api/internal/mutations/module_mutation_test.py @@ -6,6 +6,7 @@ import pytest from django.core.exceptions import ObjectDoesNotExist, PermissionDenied, ValidationError from django.utils import timezone +from strawberry.relay import GlobalID from apps.mentorship.api.internal.mutations.module import ( ModuleMutation, @@ -393,7 +394,7 @@ def test_assign_issue_success(self, mock_module, mock_mentor, mock_gh_user, mock info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=GlobalID("IssueNode", "1"), user_login="testuser", ) @@ -401,6 +402,38 @@ def test_assign_issue_success(self, mock_module, mock_mentor, mock_gh_user, mock mock_issue.assignees.add.assert_called_once_with(mock_gh) mock_interest.objects.filter.return_value.delete.assert_called_once() + @pytest.mark.parametrize("selected_pk", [101, 102]) + @patch("apps.mentorship.api.internal.mutations.module.IssueUserInterest") + @patch("apps.mentorship.api.internal.mutations.module.GithubUser") + @patch("apps.mentorship.api.internal.mutations.module.Module") + def test_assign_issue_with_overlapping_numbers( + self, mock_module, mock_gh_user, mock_interest, selected_pk + ): + """Assignment affects only the selected issue when GitHub numbers overlap.""" + module = ( + mock_module.objects.select_related.return_value.filter.return_value.first.return_value + ) + issues = {pk: MagicMock(pk=pk, number=42) for pk in (101, 102)} + module.issues.filter.side_effect = lambda **filters: MagicMock( + first=lambda: issues.get(filters["pk"]) + ) + assignee = mock_gh_user.objects.filter.return_value.first.return_value + + ModuleMutation().assign_issue_to_user( + self._make_info(MagicMock()), + module_key="mod-1", + program_key="prog-1", + issue_id=GlobalID("IssueNode", str(selected_pk)), + user_login="testuser", + ) + + issues[selected_pk].assignees.add.assert_called_once_with(assignee) + other_pk = 102 if selected_pk == 101 else 101 + issues[other_pk].assignees.add.assert_not_called() + mock_interest.objects.filter.assert_called_once_with( + module=module, issue_id=issues[selected_pk].id, user=assignee + ) + @patch("apps.mentorship.api.internal.mutations.module.Module") def test_assign_issue_module_not_found(self, mock_module): """Test ObjectDoesNotExist when module not found.""" @@ -411,12 +444,13 @@ def test_assign_issue_module_not_found(self, mock_module): ) mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(ObjectDoesNotExist, match="Module not found"): mutation.assign_issue_to_user( info, module_key="bad", program_key="bad", - issue_number=1, + issue_id=issue_id, user_login="testuser", ) @@ -433,12 +467,13 @@ def test_assign_issue_not_mentor(self, mock_module, mock_mentor): mock_mod.has_mentor.return_value = False mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(PermissionDenied, match="Only mentors of this module can assign"): mutation.assign_issue_to_user( info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=issue_id, user_login="testuser", ) @@ -458,12 +493,13 @@ def test_assign_issue_user_not_found(self, mock_module, mock_mentor, mock_gh): mock_gh.objects.filter.return_value.first.return_value = None mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(ObjectDoesNotExist, match="Assignee not found"): mutation.assign_issue_to_user( info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=issue_id, user_login="testuser", ) @@ -484,12 +520,13 @@ def test_assign_issue_issue_not_found(self, mock_module, mock_mentor, mock_gh): mock_mod.issues.filter.return_value.first.return_value = None mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(ObjectDoesNotExist, match="Issue not found in this module"): mutation.assign_issue_to_user( info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=issue_id, user_login="testuser", ) @@ -502,11 +539,12 @@ def _make_info(self, user): info.context.request.user = user return info + @pytest.mark.parametrize("selected_pk", [101, 102]) @patch("apps.mentorship.api.internal.mutations.module.GithubUser") @patch("apps.mentorship.api.internal.mutations.module.Mentor") @patch("apps.mentorship.api.internal.mutations.module.Module") - def test_unassign_issue_success(self, mock_module, mock_mentor, mock_gh_user): - """Test successful issue un-assignment.""" + def test_unassign_issue_success(self, mock_module, mock_mentor, mock_gh_user, selected_pk): + """Unassign only the selected issue when GitHub numbers overlap.""" user = MagicMock() info = self._make_info(user) @@ -518,20 +556,25 @@ def test_unassign_issue_success(self, mock_module, mock_mentor, mock_gh_user): mock_mentor.objects.filter.return_value.exists.return_value = True mock_gh = MagicMock() mock_gh_user.objects.filter.return_value.first.return_value = mock_gh - mock_issue = MagicMock() - mock_mod.issues.filter.return_value.first.return_value = mock_issue + issues = {pk: MagicMock(pk=pk, number=42) for pk in (101, 102)} + mock_issue = issues[selected_pk] + mock_mod.issues.filter.side_effect = lambda **filters: MagicMock( + first=lambda: issues.get(filters["pk"]) + ) mutation = ModuleMutation() result = mutation.unassign_issue_from_user( info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=GlobalID("IssueNode", str(selected_pk)), user_login="testuser", ) assert result == mock_mod mock_issue.assignees.remove.assert_called_once_with(mock_gh) + other_pk = 102 if selected_pk == 101 else 101 + issues[other_pk].assignees.remove.assert_not_called() @patch("apps.mentorship.api.internal.mutations.module.Module") def test_unassign_module_not_found(self, mock_module): @@ -543,12 +586,13 @@ def test_unassign_module_not_found(self, mock_module): ) mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(ObjectDoesNotExist, match="Module not found"): mutation.unassign_issue_from_user( info, module_key="bad", program_key="bad", - issue_number=1, + issue_id=issue_id, user_login="testuser", ) @@ -565,12 +609,13 @@ def test_unassign_not_mentor(self, mock_module, mock_mentor): mock_mod.has_mentor.return_value = False mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(PermissionDenied): mutation.unassign_issue_from_user( info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=issue_id, user_login="testuser", ) @@ -590,12 +635,13 @@ def test_unassign_user_not_found(self, mock_module, mock_mentor, mock_gh): mock_gh.objects.filter.return_value.first.return_value = None mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(ObjectDoesNotExist, match="Assignee not found"): mutation.unassign_issue_from_user( info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=issue_id, user_login="testuser", ) @@ -616,12 +662,13 @@ def test_unassign_issue_not_found(self, mock_module, mock_mentor, mock_gh): mock_mod.issues.filter.return_value.first.return_value = None mutation = ModuleMutation() - with pytest.raises(ObjectDoesNotExist, match=r"Issue .* not found in this module"): + issue_id = GlobalID("IssueNode", "42") + with pytest.raises(ObjectDoesNotExist, match="Issue not found in this module"): mutation.unassign_issue_from_user( info, module_key="mod-1", program_key="prog-1", - issue_number=42, + issue_id=issue_id, user_login="testuser", ) @@ -634,12 +681,15 @@ def _make_info(self, user): info.context.request.user = user return info + @pytest.mark.parametrize("selected_pk", [101, 102]) @patch("apps.mentorship.api.internal.mutations.module.Task") @patch("apps.mentorship.api.internal.mutations.module.Mentor") @patch("apps.mentorship.api.internal.mutations.module.Module") @patch("apps.mentorship.api.internal.mutations.module.timezone") - def test_set_task_deadline_success(self, mock_tz, mock_module, mock_mentor, mock_task): - """Test successful deadline setting.""" + def test_set_task_deadline_success( + self, mock_tz, mock_module, mock_mentor, mock_task, selected_pk + ): + """Set a deadline only for the selected issue when GitHub numbers overlap.""" user = MagicMock() info = self._make_info(user) mock_task.Status = Task.Status @@ -653,14 +703,17 @@ def test_set_task_deadline_success(self, mock_tz, mock_module, mock_mentor, mock ) mock_mentor.objects.filter.return_value.exists.return_value = True - mock_issue = MagicMock() + issues = {pk: MagicMock(pk=pk, number=42) for pk in (101, 102)} + mock_issue = issues[selected_pk] assignee = MagicMock() assignees_qs = MagicMock() assignees_qs.exists.return_value = True assignees_qs.__iter__ = MagicMock(return_value=iter([assignee])) mock_issue.assignees.all.return_value = assignees_qs issue_qs = mock_mod.issues.select_related.return_value - issue_qs.prefetch_related.return_value.filter.return_value.first.return_value = mock_issue + issue_qs.prefetch_related.return_value.filter.side_effect = lambda **filters: MagicMock( + first=lambda: issues.get(filters["pk"]) + ) deadline = datetime(2025, 12, 1, tzinfo=UTC) mock_tz.is_naive.return_value = False @@ -675,11 +728,16 @@ def test_set_task_deadline_success(self, mock_tz, mock_module, mock_mentor, mock info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=GlobalID("IssueNode", str(selected_pk)), deadline_at=deadline, ) assert result == mock_mod + mock_task.objects.get_or_create.assert_called_once_with( + module=mock_mod, issue=mock_issue, assignee=assignee + ) + other_pk = 102 if selected_pk == 101 else 101 + issues[other_pk].assignees.all.assert_not_called() mock_task.objects.bulk_update.assert_called_once() @patch("apps.mentorship.api.internal.mutations.module.Module") @@ -697,7 +755,7 @@ def test_set_deadline_module_not_found(self, mock_module): info, module_key="bad", program_key="bad", - issue_number=1, + issue_id=GlobalID("IssueNode", "1"), deadline_at=datetime(2025, 12, 1, tzinfo=UTC), ) @@ -721,7 +779,7 @@ def test_set_deadline_not_mentor(self, mock_module, mock_mentor): info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=GlobalID("IssueNode", "1"), deadline_at=datetime(2025, 12, 1, tzinfo=UTC), ) @@ -746,7 +804,7 @@ def test_set_deadline_issue_not_found(self, mock_module, mock_mentor): info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=GlobalID("IssueNode", "1"), deadline_at=datetime(2025, 12, 1, tzinfo=UTC), ) @@ -774,7 +832,7 @@ def test_set_deadline_no_assignees(self, mock_module, mock_mentor): info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=GlobalID("IssueNode", "1"), deadline_at=datetime(2025, 12, 1, tzinfo=UTC), ) @@ -821,7 +879,7 @@ def test_set_deadline_naive_deadline(self, mock_tz, mock_module, mock_mentor, mo info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=GlobalID("IssueNode", "1"), deadline_at=naive_deadline, ) @@ -870,7 +928,7 @@ def test_set_deadline_past_within_range(self, mock_tz, mock_module, mock_mentor, info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=GlobalID("IssueNode", "1"), deadline_at=past_deadline, ) @@ -903,12 +961,13 @@ def test_set_deadline_before_module_start(self, mock_tz, mock_module, mock_mento mock_tz.now.return_value = datetime(2025, 6, 1, tzinfo=UTC) mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(ValidationError, match="within the module's start and end dates"): mutation.set_task_deadline( info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=issue_id, deadline_at=early_deadline, ) @@ -938,12 +997,13 @@ def test_set_deadline_after_module_end(self, mock_tz, mock_module, mock_mentor): mock_tz.now.return_value = datetime(2025, 6, 1, tzinfo=UTC) mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(ValidationError, match="within the module's start and end dates"): mutation.set_task_deadline( info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=issue_id, deadline_at=late_deadline, ) @@ -956,11 +1016,12 @@ def _make_info(self, user): info.context.request.user = user return info + @pytest.mark.parametrize("selected_pk", [101, 102]) @patch("apps.mentorship.api.internal.mutations.module.Task") @patch("apps.mentorship.api.internal.mutations.module.Mentor") @patch("apps.mentorship.api.internal.mutations.module.Module") - def test_clear_deadline_single_task(self, mock_module, mock_mentor, mock_task): - """Test clearing deadline for a single task.""" + def test_clear_deadline_single_task(self, mock_module, mock_mentor, mock_task, selected_pk): + """Clear a deadline only for the selected issue when GitHub numbers overlap.""" user = MagicMock() info = self._make_info(user) @@ -971,14 +1032,17 @@ def test_clear_deadline_single_task(self, mock_module, mock_mentor, mock_task): ) mock_mentor.objects.filter.return_value.exists.return_value = True - mock_issue = MagicMock() + issues = {pk: MagicMock(pk=pk, number=42) for pk in (101, 102)} + mock_issue = issues[selected_pk] assignee = MagicMock() assignees_qs = MagicMock() assignees_qs.exists.return_value = True assignees_qs.__iter__ = MagicMock(return_value=iter([assignee])) mock_issue.assignees.all.return_value = assignees_qs issue_qs = mock_mod.issues.select_related.return_value - issue_qs.prefetch_related.return_value.filter.return_value.first.return_value = mock_issue + issue_qs.prefetch_related.return_value.filter.side_effect = lambda **filters: MagicMock( + first=lambda: issues.get(filters["pk"]) + ) task_obj = MagicMock() task_obj.deadline_at = datetime(2025, 12, 1, tzinfo=UTC) @@ -986,10 +1050,18 @@ def test_clear_deadline_single_task(self, mock_module, mock_mentor, mock_task): mutation = ModuleMutation() result = mutation.clear_task_deadline( - info, module_key="mod-1", program_key="prog-1", issue_number=1 + info, + module_key="mod-1", + program_key="prog-1", + issue_id=GlobalID("IssueNode", str(selected_pk)), ) assert result == mock_mod + mock_task.objects.filter.assert_called_once_with( + module=mock_mod, issue=mock_issue, assignee=assignee + ) + other_pk = 102 if selected_pk == 101 else 101 + issues[other_pk].assignees.all.assert_not_called() assert task_obj.deadline_at is None task_obj.save.assert_called_once_with(update_fields=["deadline_at"]) @@ -1026,7 +1098,7 @@ def test_clear_deadline_multiple_tasks(self, mock_module, mock_mentor, mock_task mutation = ModuleMutation() result = mutation.clear_task_deadline( - info, module_key="mod-1", program_key="prog-1", issue_number=1 + info, module_key="mod-1", program_key="prog-1", issue_id=GlobalID("IssueNode", "1") ) assert result == mock_mod @@ -1062,7 +1134,7 @@ def test_clear_deadline_no_tasks_with_deadline(self, mock_module, mock_mentor, m mutation = ModuleMutation() result = mutation.clear_task_deadline( - info, module_key="mod-1", program_key="prog-1", issue_number=1 + info, module_key="mod-1", program_key="prog-1", issue_id=GlobalID("IssueNode", "1") ) assert result == mock_mod @@ -1096,7 +1168,7 @@ def test_clear_deadline_task_not_found(self, mock_module, mock_mentor, mock_task mutation = ModuleMutation() result = mutation.clear_task_deadline( - info, module_key="mod-1", program_key="prog-1", issue_number=1 + info, module_key="mod-1", program_key="prog-1", issue_id=GlobalID("IssueNode", "1") ) assert result == mock_mod @@ -1111,8 +1183,11 @@ def test_clear_deadline_module_not_found(self, mock_module): ) mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(ObjectDoesNotExist, match="Module not found"): - mutation.clear_task_deadline(info, module_key="bad", program_key="bad", issue_number=1) + mutation.clear_task_deadline( + info, module_key="bad", program_key="bad", issue_id=issue_id + ) @patch("apps.mentorship.api.internal.mutations.module.Mentor") @patch("apps.mentorship.api.internal.mutations.module.Module") @@ -1127,9 +1202,10 @@ def test_clear_deadline_not_mentor(self, mock_module, mock_mentor): mock_mod.has_mentor.return_value = False mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(PermissionDenied, match="Only mentors of this module can clear"): mutation.clear_task_deadline( - info, module_key="mod-1", program_key="prog-1", issue_number=1 + info, module_key="mod-1", program_key="prog-1", issue_id=issue_id ) @patch("apps.mentorship.api.internal.mutations.module.Mentor") @@ -1148,9 +1224,10 @@ def test_clear_deadline_issue_not_found(self, mock_module, mock_mentor): issue_qs.prefetch_related.return_value.filter.return_value.first.return_value = None mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(ObjectDoesNotExist, match="Issue not found in this module"): mutation.clear_task_deadline( - info, module_key="mod-1", program_key="prog-1", issue_number=1 + info, module_key="mod-1", program_key="prog-1", issue_id=issue_id ) @patch("apps.mentorship.api.internal.mutations.module.Mentor") @@ -1172,9 +1249,10 @@ def test_clear_deadline_no_assignees(self, mock_module, mock_mentor): issue_qs.prefetch_related.return_value.filter.return_value.first.return_value = mock_issue mutation = ModuleMutation() + issue_id = GlobalID("IssueNode", "1") with pytest.raises(ValidationError, match="Cannot clear deadline: issue has no assignees"): mutation.clear_task_deadline( - info, module_key="mod-1", program_key="prog-1", issue_number=1 + info, module_key="mod-1", program_key="prog-1", issue_id=issue_id ) @@ -1604,12 +1682,13 @@ def test_mentee_blocked_when_setting_disabled(self, mock_module): mutation = ModuleMutation() deadline_at = datetime(2025, 12, 1, tzinfo=UTC) + issue_id = GlobalID("IssueNode", "1") with pytest.raises(PermissionDenied, match="Only mentors of this module can set"): mutation.set_task_deadline( info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=issue_id, deadline_at=deadline_at, ) @@ -1647,12 +1726,13 @@ def test_mentee_blocked_when_not_assignee(self, mock_tz, mock_module, mock_task) mutation = ModuleMutation() deadline_at = datetime(2025, 12, 1, tzinfo=UTC) + issue_id = GlobalID("IssueNode", "1") with pytest.raises(PermissionDenied, match="You can only set deadlines"): mutation.set_task_deadline( info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=issue_id, deadline_at=deadline_at, ) @@ -1697,7 +1777,7 @@ def test_mentee_can_set_deadline_when_assignee(self, mock_tz, mock_module, mock_ info, module_key="mod-1", program_key="prog-1", - issue_number=1, + issue_id=GlobalID("IssueNode", "1"), deadline_at=datetime(2025, 12, 1, tzinfo=UTC), ) assert result is not None diff --git a/backend/tests/unit/apps/mentorship/api/internal/nodes/api_internal_module_test.py b/backend/tests/unit/apps/mentorship/api/internal/nodes/api_internal_module_test.py index 72d967eb29..13222ac7e3 100644 --- a/backend/tests/unit/apps/mentorship/api/internal/nodes/api_internal_module_test.py +++ b/backend/tests/unit/apps/mentorship/api/internal/nodes/api_internal_module_test.py @@ -3,6 +3,7 @@ import pytest import strawberry +from strawberry.relay import GlobalID from apps.github.api.internal.nodes.issue import MERGED_PULL_REQUESTS_PREFETCH from apps.mentorship.api.internal.nodes.enum import ExperienceLevelEnum @@ -56,8 +57,8 @@ def mock_mentors(self): def mock_mentees(self): return _call_module_resolver(self, "mentees") - def mock_issue_mentees(self, issue_number: int): - return _call_module_resolver(self, "issue_mentees", issue_number=issue_number) + def mock_issue_mentees(self, issue_id: GlobalID): + return _call_module_resolver(self, "issue_mentees", issue_id=issue_id) def mock_project_name(self): return _call_module_resolver(self, "project_name") @@ -76,24 +77,24 @@ def mock_issues_count(self, label: str | None = None): def mock_available_labels(self): return _call_module_resolver(self, "available_labels") - def mock_issue_by_number(self, number: int): + def mock_issue_by_id(self, issue_id: GlobalID): info = MagicMock() info.context.task_deadlines_by_issue = None info.context.task_assigned_at_by_issue = None - return _call_module_resolver(self, "issue_by_number", info, number=number) + return _call_module_resolver(self, "issue_by_id", info, issue_id=issue_id) - def mock_interested_users(self, issue_number: int): - return _call_module_resolver(self, "interested_users", issue_number=issue_number) + def mock_interested_users(self, issue_id: GlobalID): + return _call_module_resolver(self, "interested_users", issue_id=issue_id) - def mock_task_deadline(self, issue_number: int): + def mock_task_deadline(self, issue_id: GlobalID): info = MagicMock() info.context.task_deadlines_by_issue = None - return _call_module_resolver(self, "task_deadline", info, issue_number=issue_number) + return _call_module_resolver(self, "task_deadline", info, issue_id=issue_id) - def mock_task_assigned_at(self, issue_number: int): + def mock_task_assigned_at(self, issue_id: GlobalID): info = MagicMock() info.context.task_assigned_at_by_issue = None - return _call_module_resolver(self, "task_assigned_at", info, issue_number=issue_number) + return _call_module_resolver(self, "task_assigned_at", info, issue_id=issue_id) @pytest.fixture @@ -175,9 +176,9 @@ def test_module_node_issue_mentees(self, mock_module_node): ] mock_user_objects.filter.return_value.order_by.return_value = [MagicMock()] - mentees = mock_module_node.mock_issue_mentees(issue_number=123) + mentees = mock_module_node.mock_issue_mentees(issue_id=GlobalID("IssueNode", "123")) assert len(mentees) == 1 - mock_module_node.issues.filter.assert_called_once_with(number=123) + mock_module_node.issues.filter.assert_called_once_with(pk=123) mock_task_objects.filter.assert_called_once_with( module=mock_module_node, issue_id__in=["issue_id_1"], assignee__isnull=False ) @@ -187,7 +188,7 @@ def test_module_node_issue_mentees(self, mock_module_node): def test_module_node_issue_mentees_no_issue(self, mock_module_node): """Test issue_mentees when no issue is found.""" mock_module_node.issues.filter.return_value.values_list.return_value = [] - mentees = mock_module_node.mock_issue_mentees(issue_number=123) + mentees = mock_module_node.mock_issue_mentees(issue_id=GlobalID("IssueNode", "123")) assert mentees == [] def test_module_node_project_name(self, mock_module_node): @@ -238,9 +239,9 @@ def test_module_node_available_labels(self, mock_module_node): issue__mentorship_modules=mock_module_node ) - def test_module_node_issue_by_number(self, mock_module_node): - """Test the issue_by_number resolver.""" - issue = mock_module_node.mock_issue_by_number(number=456) + def test_module_node_issue_by_id(self, mock_module_node): + """Test the issue_by_id resolver.""" + issue = mock_module_node.mock_issue_by_id(issue_id=GlobalID("IssueNode", "456")) assert issue is not None mock_module_node.issues.select_related.assert_called_once_with("repository", "author") mock_module_node.issues.select_related.return_value.prefetch_related.assert_called_once_with( @@ -248,11 +249,28 @@ def test_module_node_issue_by_number(self, mock_module_node): ) mock_module_node_qs_related = mock_module_node.issues.select_related.return_value mock_module_node_qs_related.prefetch_related.return_value.filter.assert_called_once_with( - number=456 + pk=456 ) mock_node_related = mock_module_node.issues.select_related.return_value mock_node_related.prefetch_related.return_value.filter.return_value.first.assert_called_once() + @pytest.mark.parametrize("selected_pk", [101, 102]) + def test_issue_by_id_with_overlapping_numbers(self, mock_module_node, selected_pk): + """Each repository's issue remains accessible when GitHub numbers overlap.""" + issues = {pk: MagicMock(pk=pk, number=42) for pk in (101, 102)} + queryset = ( + mock_module_node.issues.select_related.return_value.prefetch_related.return_value + ) + queryset.filter.side_effect = lambda **filters: MagicMock( + first=lambda: issues.get(filters["pk"]) + ) + + selected = mock_module_node.mock_issue_by_id( + issue_id=GlobalID("IssueNode", str(selected_pk)) + ) + + assert selected is issues[selected_pk] + def test_module_node_interested_users(self, mock_module_node): """Test the interested_users resolver.""" with patch( @@ -268,11 +286,11 @@ def test_module_node_interested_users(self, mock_module_node): mock_interest2, ] - users = mock_module_node.mock_interested_users(issue_number=789) + users = mock_module_node.mock_interested_users(issue_id=GlobalID("IssueNode", "789")) assert len(users) == 2 assert users[0].login == "user1" assert users[1].login == "user2" - mock_module_node.issues.filter.assert_called_once_with(number=789) + mock_module_node.issues.filter.assert_called_once_with(pk=789) mock_issue_user_interest_objects.select_related.assert_called_once_with("user") mock_issue_user_interest_objects.select_related.return_value.filter.assert_called_once_with( module=mock_module_node, issue_id__in=["issue_id_1"] @@ -281,7 +299,7 @@ def test_module_node_interested_users(self, mock_module_node): def test_module_node_interested_users_no_issue(self, mock_module_node): """Test interested_users when no issue is found.""" mock_module_node.issues.filter.return_value.values_list.return_value = [] - users = mock_module_node.mock_interested_users(issue_number=789) + users = mock_module_node.mock_interested_users(issue_id=GlobalID("IssueNode", "789")) assert users == [] def test_module_node_task_deadline(self, mock_module_node): @@ -292,10 +310,10 @@ def test_module_node_task_deadline(self, mock_module_node): 2025, 10, 26, tzinfo=UTC ) - deadline = mock_module_node.mock_task_deadline(issue_number=101) + deadline = mock_module_node.mock_task_deadline(issue_id=GlobalID("IssueNode", "101")) assert deadline == datetime(2025, 10, 26, tzinfo=UTC) mock_task_objects.filter.assert_called_once_with( - module=mock_module_node, issue__number=101, deadline_at__isnull=False + module=mock_module_node, issue_id=101, deadline_at__isnull=False ) def test_module_node_task_deadline_none(self, mock_module_node): @@ -304,7 +322,7 @@ def test_module_node_task_deadline_none(self, mock_module_node): mock_task_order_by = mock_task_objects.filter.return_value.order_by mock_task_order_by.return_value.values_list.return_value.first.return_value = None - deadline = mock_module_node.mock_task_deadline(issue_number=101) + deadline = mock_module_node.mock_task_deadline(issue_id=GlobalID("IssueNode", "101")) assert deadline is None def test_module_node_task_deadline_with_bulk_load(self, mock_module_node): @@ -314,7 +332,9 @@ def test_module_node_task_deadline_with_bulk_load(self, mock_module_node): info = MagicMock() info.context.task_deadlines_by_issue = {101: expected_deadline} - deadline = _call_module_resolver(mock_module_node, "task_deadline", info, issue_number=101) + deadline = _call_module_resolver( + mock_module_node, "task_deadline", info, issue_id=GlobalID("IssueNode", "101") + ) assert deadline == expected_deadline def test_module_node_task_deadline_with_bulk_load_not_found(self, mock_module_node): @@ -322,7 +342,9 @@ def test_module_node_task_deadline_with_bulk_load_not_found(self, mock_module_no info = MagicMock() info.context.task_deadlines_by_issue = {102: datetime(2025, 10, 26, tzinfo=UTC)} - deadline = _call_module_resolver(mock_module_node, "task_deadline", info, issue_number=101) + deadline = _call_module_resolver( + mock_module_node, "task_deadline", info, issue_id=GlobalID("IssueNode", "101") + ) assert deadline is None def test_module_node_task_assigned_at(self, mock_module_node): @@ -333,10 +355,12 @@ def test_module_node_task_assigned_at(self, mock_module_node): 2025, 9, 15, tzinfo=UTC ) - assigned_at = mock_module_node.mock_task_assigned_at(issue_number=202) + assigned_at = mock_module_node.mock_task_assigned_at( + issue_id=GlobalID("IssueNode", "202") + ) assert assigned_at == datetime(2025, 9, 15, tzinfo=UTC) mock_task_objects.filter.assert_called_once_with( - module=mock_module_node, issue__number=202, assigned_at__isnull=False + module=mock_module_node, issue_id=202, assigned_at__isnull=False ) def test_module_node_task_assigned_at_none(self, mock_module_node): @@ -345,7 +369,9 @@ def test_module_node_task_assigned_at_none(self, mock_module_node): mock_task_order_by = mock_task_objects.filter.return_value.order_by.return_value mock_task_order_by.values_list.return_value.first.return_value = None - assigned_at = mock_module_node.mock_task_assigned_at(issue_number=202) + assigned_at = mock_module_node.mock_task_assigned_at( + issue_id=GlobalID("IssueNode", "202") + ) assert assigned_at is None def test_module_node_task_assigned_at_with_bulk_load(self, mock_module_node): @@ -356,7 +382,7 @@ def test_module_node_task_assigned_at_with_bulk_load(self, mock_module_node): info.context.task_assigned_at_by_issue = {202: expected_assigned_at} assigned_at = _call_module_resolver( - mock_module_node, "task_assigned_at", info, issue_number=202 + mock_module_node, "task_assigned_at", info, issue_id=GlobalID("IssueNode", "202") ) assert assigned_at == expected_assigned_at @@ -366,7 +392,7 @@ def test_module_node_task_assigned_at_with_bulk_load_not_found(self, mock_module info.context.task_assigned_at_by_issue = {203: datetime(2025, 9, 15, tzinfo=UTC)} assigned_at = _call_module_resolver( - mock_module_node, "task_assigned_at", info, issue_number=202 + mock_module_node, "task_assigned_at", info, issue_id=GlobalID("IssueNode", "202") ) assert assigned_at is None @@ -399,20 +425,20 @@ def test_module_node_issues_bulk_load_with_duplicates(self, mock_module_node): """Test issues resolver properly handles duplicate deadlines during bulk load.""" with patch("apps.mentorship.models.task.Task.objects") as mock_task_objects: deadline_data = [ - {"issue__number": 101, "deadline_at": datetime(2025, 10, 26, tzinfo=UTC)}, + {"issue_id": 101, "deadline_at": datetime(2025, 10, 26, tzinfo=UTC)}, { - "issue__number": 101, + "issue_id": 101, "deadline_at": datetime(2025, 10, 27, tzinfo=UTC), }, - {"issue__number": 102, "deadline_at": datetime(2025, 10, 28, tzinfo=UTC)}, + {"issue_id": 102, "deadline_at": datetime(2025, 10, 28, tzinfo=UTC)}, ] assigned_data = [ - {"issue__number": 101, "assigned_at": datetime(2025, 9, 15, tzinfo=UTC)}, + {"issue_id": 101, "assigned_at": datetime(2025, 9, 15, tzinfo=UTC)}, { - "issue__number": 101, + "issue_id": 101, "assigned_at": datetime(2025, 9, 16, tzinfo=UTC), }, - {"issue__number": 102, "assigned_at": datetime(2025, 9, 17, tzinfo=UTC)}, + {"issue_id": 102, "assigned_at": datetime(2025, 9, 17, tzinfo=UTC)}, ] mock_task_objects.filter.return_value.order_by.return_value.values.return_value = ( @@ -491,8 +517,8 @@ def test_module_node_issues_unauthorized(self, mock_module_node): mock_module_node.program.has_admin.assert_called_once_with(info.context.request.user) mock_module_node.has_mentor.assert_called_once_with(info.context.request.user) - def test_module_node_issue_by_number_unauthorized(self, mock_module_node): - """Test issue_by_number resolver returns None for unauthorized user.""" + def test_module_node_issue_by_id_unauthorized(self, mock_module_node): + """Test issue_by_id resolver returns None for unauthorized user.""" info = MagicMock() info.context.task_deadlines_by_issue = None info.context.task_assigned_at_by_issue = None @@ -501,7 +527,9 @@ def test_module_node_issue_by_number_unauthorized(self, mock_module_node): mock_module_node.program.has_admin.return_value = False mock_module_node.has_mentor.return_value = False - result = _call_module_resolver(mock_module_node, "issue_by_number", info, number=456) + result = _call_module_resolver( + mock_module_node, "issue_by_id", info, issue_id=GlobalID("IssueNode", "456") + ) assert result is None mock_module_node.program.has_admin.assert_called_once_with(info.context.request.user) mock_module_node.has_mentor.assert_called_once_with(info.context.request.user) @@ -589,8 +617,8 @@ def test_issues_mentee_no_github_user_returns_empty(self): assert result == [] - def test_issue_by_number_mentee_sees_only_assigned(self): - """Mentee can only see issue_by_number when assigned to that issue.""" + def test_issue_by_id_mentee_sees_only_assigned(self): + """Mentee can only see issue_by_id when assigned to that issue.""" m = FakeModuleNode() m.program.has_admin.return_value = False m.has_mentor.return_value = False @@ -606,14 +634,16 @@ def test_issue_by_number_mentee_sees_only_assigned(self): mock_qs = m.issues.select_related.return_value.prefetch_related.return_value mock_qs.filter.return_value.filter.return_value.first.return_value = MagicMock() - result = _call_module_resolver(m, "issue_by_number", info, number=42) + result = _call_module_resolver( + m, "issue_by_id", info, issue_id=GlobalID("IssueNode", "42") + ) - mock_qs.filter.assert_called_once_with(number=42) + mock_qs.filter.assert_called_once_with(pk=42) mock_qs.filter.return_value.filter.assert_called_once_with(assignees=github_user) assert result is not None - def test_issue_by_number_mentee_no_github_user_returns_none(self): - """Mentee with no github_user gets None from issue_by_number.""" + def test_issue_by_id_mentee_no_github_user_returns_none(self): + """Mentee with no github_user gets None from issue_by_id.""" m = FakeModuleNode() m.program.has_admin.return_value = False m.has_mentor.return_value = False @@ -625,6 +655,8 @@ def test_issue_by_number_mentee_no_github_user_returns_none(self): info = MagicMock() info.context.request.user = user - result = _call_module_resolver(m, "issue_by_number", info, number=42) + result = _call_module_resolver( + m, "issue_by_id", info, issue_id=GlobalID("IssueNode", "42") + ) assert result is None diff --git a/backend/tests/unit/apps/mentorship/api/internal/utils_test.py b/backend/tests/unit/apps/mentorship/api/internal/utils_test.py new file mode 100644 index 0000000000..fca9130ef5 --- /dev/null +++ b/backend/tests/unit/apps/mentorship/api/internal/utils_test.py @@ -0,0 +1,30 @@ +"""Tests for validating Nest issue Relay IDs.""" + +import pytest +from graphql import GraphQLError +from strawberry.relay import GlobalID + +from apps.mentorship.api.internal.utils import get_issue_pk + + +@pytest.mark.parametrize("node_id", ["101", str(2**31), str(2**63 - 1)]) +def test_get_issue_pk(node_id): + """Use the node's database ID rather than a GitHub issue number.""" + assert get_issue_pk(GlobalID("IssueNode", node_id)) == int(node_id) + + +@pytest.mark.parametrize( + "issue_id", + [ + GlobalID("UserNode", "101"), + GlobalID("IssueNode", "invalid"), + GlobalID("IssueNode", "-1"), + GlobalID("IssueNode", ""), + GlobalID("IssueNode", str(2**63)), + GlobalID("IssueNode", "9" * 5000), + ], +) +def test_get_issue_pk_invalid_id(issue_id): + """Reject IDs of other node types and invalid database IDs.""" + with pytest.raises(GraphQLError, match="Invalid issue ID"): + get_issue_pk(issue_id) diff --git a/frontend/__tests__/a11y/pages/ModuleIssueDetailsPage.a11y.test.tsx b/frontend/__tests__/a11y/pages/ModuleIssueDetailsPage.a11y.test.tsx index d0b371c3b2..7ae84d9b9a 100644 --- a/frontend/__tests__/a11y/pages/ModuleIssueDetailsPage.a11y.test.tsx +++ b/frontend/__tests__/a11y/pages/ModuleIssueDetailsPage.a11y.test.tsx @@ -25,7 +25,7 @@ jest.mock('hooks/useIssueMutations') const mockIssueData = { getModule: { - issueByNumber: { + issueById: { id: '1', title: 'Test Issue Title', body: 'This is the issue body.', diff --git a/frontend/__tests__/unit/components/IssuesTable.test.tsx b/frontend/__tests__/unit/components/IssuesTable.test.tsx index d4ff16db13..1ea309149e 100644 --- a/frontend/__tests__/unit/components/IssuesTable.test.tsx +++ b/frontend/__tests__/unit/components/IssuesTable.test.tsx @@ -399,16 +399,16 @@ describe('', () => { const issueButtons = screen.getAllByRole('button', { name: /Test Issue 1/i }) expect(issueButtons.length).toBeGreaterThan(0) fireEvent.click(issueButtons[0]) - expect(onIssueClick).toHaveBeenCalledWith(123) + expect(onIssueClick).toHaveBeenCalledWith('1') }) it('navigates to issue URL when onIssueClick is not provided', () => { - const issueUrl = (num: number) => `/issues/${num}` + const issueUrl = (id: string) => `/issues/${id}` render() const issueButtons = screen.getAllByRole('button', { name: /Test Issue 1/i }) expect(issueButtons.length).toBeGreaterThan(0) fireEvent.click(issueButtons[0]) - expect(mockPush).toHaveBeenCalledWith('/issues/123') + expect(mockPush).toHaveBeenCalledWith('/issues/1') }) it('does nothing when onIssueClick and issueUrl are not provided', () => { diff --git a/frontend/__tests__/unit/components/cards/ModuleIssues.test.tsx b/frontend/__tests__/unit/components/cards/ModuleIssues.test.tsx index d03c253947..d4084d972a 100644 --- a/frontend/__tests__/unit/components/cards/ModuleIssues.test.tsx +++ b/frontend/__tests__/unit/components/cards/ModuleIssues.test.tsx @@ -75,6 +75,28 @@ describe('ModuleIssues', () => { mockUseQuery.mockReturnValue({ data: mockModuleData, loading: false, error: undefined }) }) + it('opens distinct detail URLs for issues sharing a GitHub number', () => { + const issues = [ + { ...mockModuleData.managementModule.issues[0], id: 'SXNzdWVOb2RlOjE=', title: 'Repo A' }, + { ...mockModuleData.managementModule.issues[0], id: 'SXNzdWVOb2RlOjI=', title: 'Repo B' }, + ] + mockUseQuery.mockReturnValue({ + data: { managementModule: { ...mockModuleData.managementModule, issues, issuesCount: 2 } }, + loading: false, + }) + render() + fireEvent.click(screen.getByRole('button', { name: 'Repo A' })) + fireEvent.click(screen.getByRole('button', { name: 'Repo B' })) + expect(mockPush).toHaveBeenNthCalledWith( + 1, + '/my/mentorship/programs/prog1/modules/mod1/issues/SXNzdWVOb2RlOjE%3D' + ) + expect(mockPush).toHaveBeenNthCalledWith( + 2, + '/my/mentorship/programs/prog1/modules/mod1/issues/SXNzdWVOb2RlOjI%3D' + ) + }) + it('shows a loading message in the table while data is being fetched', () => { mockUseQuery.mockReturnValue({ data: undefined, loading: true, error: undefined }) render() @@ -125,7 +147,7 @@ describe('ModuleIssues', () => { render() const issueTitleButton = screen.getAllByRole('button', { name: /First Issue Title/i })[0] fireEvent.click(issueTitleButton) - expect(mockPush).toHaveBeenCalledWith('/my/mentorship/programs/prog1/modules/mod1/issues/101') + expect(mockPush).toHaveBeenCalledWith('/my/mentorship/programs/prog1/modules/mod1/issues/1') }) it('filters issues based on the selected label', async () => { diff --git a/frontend/__tests__/unit/hooks/useIssueMutations.test.tsx b/frontend/__tests__/unit/hooks/useIssueMutations.test.tsx new file mode 100644 index 0000000000..a36b7f4688 --- /dev/null +++ b/frontend/__tests__/unit/hooks/useIssueMutations.test.tsx @@ -0,0 +1,26 @@ +import { useMutation } from '@apollo/client/react' +import { renderHook } from '@testing-library/react' +import { useIssueMutations } from 'hooks/useIssueMutations' +import { GetManagementModuleIssueViewDocument } from 'types/__generated__/issueQueries.generated' + +jest.mock('@apollo/client/react', () => ({ useMutation: jest.fn() })) +jest.mock('@heroui/toast', () => ({ addToast: jest.fn() })) + +it('refetches the selected issue by its opaque Nest ID after each mutation', () => { + const mockUseMutation = useMutation as jest.Mock + mockUseMutation.mockReturnValue([jest.fn(), { loading: false }]) + const issueId = 'SXNzdWVOb2RlOjEyMw==' + + renderHook(() => useIssueMutations({ programKey: 'program', moduleKey: 'module', issueId })) + + expect(mockUseMutation).toHaveBeenCalledTimes(4) + for (const [, options] of mockUseMutation.mock.calls) { + expect(options.refetchQueries).toEqual([ + { + query: GetManagementModuleIssueViewDocument, + variables: { programKey: 'program', moduleKey: 'module', issueId }, + }, + ]) + expect(options.awaitRefetchQueries).toBe(true) + } +}) diff --git a/frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx b/frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx index 899dabc2a3..c72f26dd5b 100644 --- a/frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx +++ b/frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx @@ -316,6 +316,27 @@ describe('MenteeProfilePage', () => { ) }) + it('opens the selected repository issue when GitHub numbers overlap', () => { + const issue = mockMenteeData.getMenteeModuleIssues[0] + mockUseQuery.mockReturnValue({ + data: { + ...mockMenteeData, + getMenteeModuleIssues: [ + { ...issue, id: 'repo-a', title: 'Repo A', url: 'https://github.com/org/a/issues/101' }, + { ...issue, id: 'repo-b', title: 'Repo B', url: 'https://github.com/org/b/issues/101' }, + ], + }, + loading: false, + }) + render() + fireEvent.click(screen.getByRole('button', { name: 'Repo B' })) + expect(window.open).toHaveBeenCalledWith( + 'https://github.com/org/b/issues/101', + '_blank', + 'noopener,noreferrer' + ) + }) + it('does not call window.open when issue has no URL', () => { const dataWithNoUrl = { ...mockMenteeData, diff --git a/frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx b/frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx index bd0f8ffec4..aa1b2fb840 100644 --- a/frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx +++ b/frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx @@ -63,7 +63,7 @@ const mockAccessData = { const mockIssueData = { managementModule: { - issueByNumber: { + issueById: { id: '1', title: 'Test Issue Title', body: 'This is the issue body.', @@ -128,7 +128,7 @@ const menteeIssueView = ( interestedUsers: [], issueMentees: [], ...moduleOverrides, - issueByNumber: { + issueById: { id: '1', number: 42, title: 'Mentee Issue', @@ -201,7 +201,7 @@ describe('ModuleIssueDetailsPage', () => { mockUseParams.mockReturnValue({ programKey: 'prog1', moduleKey: 'mod1', - issueId: '123', + issueId: 'SXNzdWVOb2RlOjEyMw==', }) mockUseIssueMutations.mockReturnValue({ assignIssue: mockAssignIssue, @@ -259,7 +259,7 @@ describe('ModuleIssueDetailsPage', () => { setupQueryMock({ managementModule: { ...mockIssueData.managementModule, - issueByNumber: null, + issueById: null, }, }) render() @@ -293,7 +293,7 @@ describe('ModuleIssueDetailsPage', () => { variables: { programKey: 'prog1', moduleKey: 'mod1', - issueNumber: 123, + issueId: 'SXNzdWVOb2RlOjEyMw==', userLogin: 'user2', }, }) @@ -311,7 +311,7 @@ describe('ModuleIssueDetailsPage', () => { variables: { programKey: 'prog1', moduleKey: 'mod1', - issueNumber: 123, + issueId: 'SXNzdWVOb2RlOjEyMw==', userLogin: 'user1', }, }) @@ -322,8 +322,8 @@ describe('ModuleIssueDetailsPage', () => { const noPrData = { managementModule: { ...mockIssueData.managementModule, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, pullRequests: [], }, }, @@ -387,8 +387,8 @@ describe('ModuleIssueDetailsPage', () => { ...mockIssueData, managementModule: { ...mockIssueData.managementModule, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, assignees: [], }, }, @@ -474,8 +474,8 @@ describe('ModuleIssueDetailsPage', () => { ...mockIssueData, managementModule: { ...mockIssueData.managementModule, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, state, isMerged, }, @@ -492,8 +492,8 @@ describe('ModuleIssueDetailsPage', () => { managementModule: { ...mockIssueData.managementModule, taskAssignedAt: null, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, body: null, assignees: [{ id: 'a1', login: 'user1', name: null, avatarUrl: null }], pullRequests: [ @@ -540,7 +540,7 @@ describe('ModuleIssueDetailsPage', () => { variables: { programKey: 'prog1', moduleKey: 'mod1', - issueNumber: 123, + issueId: 'SXNzdWVOb2RlOjEyMw==', }, }) }) @@ -568,7 +568,7 @@ describe('ModuleIssueDetailsPage', () => { const fetchMoreMock = jest.fn().mockResolvedValue({ data: { managementModule: { - issueByNumber: { + issueById: { pullRequests: [ { id: 'pr-new', @@ -593,10 +593,10 @@ describe('ModuleIssueDetailsPage', () => { return { data: { managementModule: { - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, pullRequests: Array.from({ length: 4 }, (_, i) => ({ - ...mockIssueData.managementModule.issueByNumber.pullRequests[0], + ...mockIssueData.managementModule.issueById.pullRequests[0], id: `pr-${i}`, })), }, @@ -664,8 +664,8 @@ describe('ModuleIssueDetailsPage', () => { ...mockIssueData, managementModule: { ...mockIssueData.managementModule, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, pullRequests: Array.from({ length: 6 }, (_, i) => ({ id: `pr${i + 1}`, title: `Pull Request ${i + 1}`, @@ -716,8 +716,8 @@ describe('ModuleIssueDetailsPage', () => { ...mockIssueData, managementModule: { ...mockIssueData.managementModule, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, assignees: [ { id: 'assignee1', @@ -739,8 +739,8 @@ describe('ModuleIssueDetailsPage', () => { ...mockIssueData, managementModule: { ...mockIssueData.managementModule, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, assignees: [ { id: 'assignee1', @@ -763,8 +763,8 @@ describe('ModuleIssueDetailsPage', () => { ...mockIssueData, managementModule: { ...mockIssueData.managementModule, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, pullRequests: null, }, }, @@ -801,8 +801,8 @@ describe('ModuleIssueDetailsPage', () => { managementModule: { ...mockIssueData.managementModule, interestedUsers: null, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, assignees: null, labels: null, }, @@ -889,8 +889,8 @@ describe('ModuleIssueDetailsPage', () => { ...mockIssueData, managementModule: { ...mockIssueData.managementModule, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, assignees: [], }, }, @@ -943,9 +943,9 @@ describe('ModuleIssueDetailsPage', () => { expect(screen.queryByRole('heading', { name: /Pull Requests/i })).not.toBeInTheDocument() }) - it('shows issue not found for a mentee when issueByNumber is null', async () => { + it('shows issue not found for a mentee when issueById is null', async () => { setupQueryMock({ - managementModule: { ...menteeIssueView().managementModule, issueByNumber: null }, + managementModule: { ...menteeIssueView().managementModule, issueById: null }, }) render() await waitFor(() => { @@ -1134,7 +1134,11 @@ describe('PR pagination edge cases', () => { beforeEach(() => { jest.clearAllMocks() - mockUseParams.mockReturnValue({ programKey: 'prog1', moduleKey: 'mod1', issueId: '123' }) + mockUseParams.mockReturnValue({ + programKey: 'prog1', + moduleKey: 'mod1', + issueId: 'SXNzdWVOb2RlOjEyMw==', + }) mockUseIssueMutations.mockReturnValue({ assignIssue: jest.fn(), unassignIssue: jest.fn(), @@ -1172,7 +1176,7 @@ describe('PR pagination edge cases', () => { data: { managementModule: { ...mockIssueData.managementModule, - issueByNumber: { ...mockIssueData.managementModule.issueByNumber, pullRequests: prs }, + issueById: { ...mockIssueData.managementModule.issueById, pullRequests: prs }, }, }, loading: false, @@ -1192,10 +1196,10 @@ describe('PR pagination edge cases', () => { { managementModule: { ...mockIssueData.managementModule, - issueByNumber: { ...mockIssueData.managementModule.issueByNumber, pullRequests: prs }, + issueById: { ...mockIssueData.managementModule.issueById, pullRequests: prs }, }, }, - { fetchMoreResult: { managementModule: { issueByNumber: { pullRequests: [] } } } } + { fetchMoreResult: { managementModule: { issueById: { pullRequests: [] } } } } ) return Promise.resolve({}) } @@ -1219,8 +1223,8 @@ describe('PR pagination edge cases', () => { data: { managementModule: { ...mockIssueData.managementModule, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, pullRequests: currentPRs, }, }, @@ -1235,15 +1239,15 @@ describe('PR pagination edge cases', () => { { managementModule: { ...mockIssueData.managementModule, - issueByNumber: { - ...mockIssueData.managementModule.issueByNumber, + issueById: { + ...mockIssueData.managementModule.issueById, pullRequests: prs, }, }, }, { fetchMoreResult: { - managementModule: { issueByNumber: { pullRequests: extraPRs } }, + managementModule: { issueById: { pullRequests: extraPRs } }, }, } ) @@ -1283,7 +1287,11 @@ describe('PR pagination edge cases', () => { describe('Mentee issue status badges', () => { beforeEach(() => { jest.clearAllMocks() - mockUseParams.mockReturnValue({ programKey: 'prog1', moduleKey: 'mod1', issueId: '123' }) + mockUseParams.mockReturnValue({ + programKey: 'prog1', + moduleKey: 'mod1', + issueId: 'SXNzdWVOb2RlOjEyMw==', + }) mockUseIssueMutations.mockReturnValue({ assignIssue: jest.fn(), unassignIssue: jest.fn(), diff --git a/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx b/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx index 6df8ca34e0..095e865049 100644 --- a/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx +++ b/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx @@ -47,7 +47,7 @@ const ModuleIssueDetailsPage = () => { variables: { programKey, moduleKey, - number: Number(issueId), + issueId, limit, offset: 0, }, @@ -77,7 +77,7 @@ const ModuleIssueDetailsPage = () => { }, [error]) useEffect(() => { - const prCount = data?.managementModule?.issueByNumber?.pullRequests?.length + const prCount = data?.managementModule?.issueById?.pullRequests?.length if (prCount == null) return if (prCount <= limit) { setHasMorePRs(prCount >= limit) @@ -133,7 +133,7 @@ const ModuleIssueDetailsPage = () => { const mentorshipModule = data?.managementModule const isMentee = mentorshipModule?.userRole === 'mentee' - const issue = mentorshipModule?.issueByNumber + const issue = mentorshipModule?.issueById const taskDeadline = (mentorshipModule?.taskDeadline as string | undefined) ?? null const moduleStartedAt = mentorshipModule?.startedAt const moduleEndedAt = mentorshipModule?.endedAt @@ -272,7 +272,7 @@ const ModuleIssueDetailsPage = () => { variables: { programKey, moduleKey, - issueNumber: Number(issueId), + issueId, deadlineAt: iso, }, }) @@ -282,7 +282,7 @@ const ModuleIssueDetailsPage = () => { variables: { programKey, moduleKey, - issueNumber: Number(issueId), + issueId, }, }) } @@ -388,7 +388,7 @@ const ModuleIssueDetailsPage = () => { variables: { programKey, moduleKey, - issueNumber: Number(issueId), + issueId, userLogin: a.login, }, }) @@ -428,26 +428,25 @@ const ModuleIssueDetailsPage = () => { variables: { programKey, moduleKey, - number: Number(issueId), + issueId, offset: currentLength, limit, }, updateQuery: (prevResult, { fetchMoreResult }) => { if (!fetchMoreResult) return prevResult const newPRs = - fetchMoreResult.managementModule?.issueByNumber?.pullRequests || - [] + fetchMoreResult.managementModule?.issueById?.pullRequests || [] if (newPRs.length < limit) setHasMorePRs(false) if (newPRs.length === 0) return prevResult return { ...prevResult, managementModule: { ...prevResult.managementModule!, - issueByNumber: { - ...prevResult.managementModule!.issueByNumber!, + issueById: { + ...prevResult.managementModule!.issueById!, pullRequests: [ - ...(prevResult.managementModule?.issueByNumber - ?.pullRequests || []), + ...(prevResult.managementModule?.issueById?.pullRequests || + []), ...newPRs, ], }, @@ -527,7 +526,7 @@ const ModuleIssueDetailsPage = () => { variables: { programKey, moduleKey, - issueNumber: Number(issueId), + issueId, userLogin: u.login, }, }) diff --git a/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx b/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx index 4e31b1b1c0..cc1bcbde63 100644 --- a/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx +++ b/frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx @@ -114,8 +114,8 @@ const MenteeProfilePage = () => { { key: 'closed', label: 'Closed', count: closedIssues.length }, ] - const handleIssueClick = (issueNumber: number) => { - const issue = menteeIssues.find((i) => i.number === issueNumber) + const handleIssueClick = (issueId: string) => { + const issue = menteeIssues.find((i) => i.objectID === issueId) if (issue?.url) { window.open(issue.url, '_blank', 'noopener,noreferrer') } diff --git a/frontend/src/components/IssuesTable.tsx b/frontend/src/components/IssuesTable.tsx index 7984406e80..a6945bbe12 100644 --- a/frontend/src/components/IssuesTable.tsx +++ b/frontend/src/components/IssuesTable.tsx @@ -24,8 +24,8 @@ interface IssuesTableProps { issues: IssueRow[] showAssignee?: boolean showDeadline?: boolean - onIssueClick?: (issueNumber: number) => void - issueUrl?: (issueNumber: number) => string + onIssueClick?: (issueId: string) => void + issueUrl?: (issueId: string) => string maxVisibleLabels?: number emptyMessage?: string } @@ -43,11 +43,11 @@ const IssuesTable: React.FC = ({ }) => { const router = useRouter() - const handleIssueClick = (issueNumber: number) => { + const handleIssueClick = (issueId: string) => { if (onIssueClick) { - onIssueClick(issueNumber) + onIssueClick(issueId) } else if (issueUrl) { - router.push(issueUrl(issueNumber)) + router.push(issueUrl(issueId)) } } @@ -174,7 +174,7 @@ const IssuesTable: React.FC = ({ >