From 11d950a13aeec4b5ce8f5f9c6fcbea98838105c1 Mon Sep 17 00:00:00 2001 From: null Date: Fri, 22 May 2026 22:20:28 -0500 Subject: [PATCH] refactor(issue-sync): streamline issue closing logic and enhance error handling --- backend/app/api/forgejo_issues.py | 49 +-- backend/app/services/forgejo_issue_sync.py | 370 ++++++++++----------- 2 files changed, 210 insertions(+), 209 deletions(-) diff --git a/backend/app/api/forgejo_issues.py b/backend/app/api/forgejo_issues.py index 38dc031..4a2ed9b 100644 --- a/backend/app/api/forgejo_issues.py +++ b/backend/app/api/forgejo_issues.py @@ -694,7 +694,10 @@ async def close_issue( if auth.user is None: raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED) - # Get boards linked to this issue's repository for this organization. + # Check board links — used for authorization and activity logging. + # If the repository is not linked to any board the user can still close + # the issue (org membership is sufficient); we just skip the board-scoped + # activity event. links = ( await session.exec( select(BoardRepositoryLink).where( @@ -703,22 +706,19 @@ async def close_issue( ) ) ).all() - if not links: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="Issue repository is not linked to any board", - ) - allowed_board_ids = set(await list_accessible_board_ids(session, member=ctx.member, write=True)) - authorized_board_id = next( - (link.board_id for link in links if link.board_id in allowed_board_ids), - None, - ) - if authorized_board_id is None: - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="Board access denied", + authorized_board_id: object = None + if links: + allowed_board_ids = set(await list_accessible_board_ids(session, member=ctx.member, write=True)) + authorized_board_id = next( + (link.board_id for link in links if link.board_id in allowed_board_ids), + None, ) + if authorized_board_id is None: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Board access denied", + ) # Close the issue using the service. try: @@ -735,15 +735,16 @@ async def close_issue( raise HTTPException(status_code=status.HTTP_502_BAD_GATEWAY, detail=str(e)) repository_full_name = str(result.get("repository_full_name") or "unknown/unknown") - record_activity( - session, - event_type="forgejo.issue.closed", - message=( - "Forgejo issue closed by user " - f"{auth.user.id}: {repository_full_name}#{result['forgejo_issue_number']}" - ), - board_id=authorized_board_id, - ) + if authorized_board_id is not None: + record_activity( + session, + event_type="forgejo.issue.closed", + message=( + "Forgejo issue closed by user " + f"{auth.user.id}: {repository_full_name}#{result['forgejo_issue_number']}" + ), + board_id=authorized_board_id, + ) await session.commit() return CloseIssueResponse( diff --git a/backend/app/services/forgejo_issue_sync.py b/backend/app/services/forgejo_issue_sync.py index d2b58fc..b677858 100644 --- a/backend/app/services/forgejo_issue_sync.py +++ b/backend/app/services/forgejo_issue_sync.py @@ -85,209 +85,209 @@ class IssueSyncService: limit=limit, ) - # Forgejo returns issues as a JSON array, not wrapped in "items" - issues = ( - response - if isinstance(response, list) - else response.get("items", response.get("data", [])) - ) - if not isinstance(issues, list) or len(issues) == 0: - break + # Forgejo returns issues as a JSON array, not wrapped in "items" + issues = ( + response + if isinstance(response, list) + else response.get("items", response.get("data", [])) + ) + if not isinstance(issues, list) or len(issues) == 0: + break - for issue_data in issues: - # Skip pull requests - if issue_data.get("pull_request") is not None: - continue + for issue_data in issues: + # Skip pull requests + if issue_data.get("pull_request") is not None: + continue - raw_number = issue_data.get("number", 0) - try: - forgejo_number = int(raw_number) - except (TypeError, ValueError): - forgejo_number = 0 - state = issue_data.get("state", "open") - issue_payload = dict(issue_data) - comments_payload: list[dict[str, object]] = [] - timeline_payload: list[dict[str, object]] = [] - reactions_payload: list[dict[str, object]] = [] + raw_number = issue_data.get("number", 0) + try: + forgejo_number = int(raw_number) + except (TypeError, ValueError): + forgejo_number = 0 + state = issue_data.get("state", "open") + issue_payload = dict(issue_data) + comments_payload: list[dict[str, object]] = [] + timeline_payload: list[dict[str, object]] = [] + reactions_payload: list[dict[str, object]] = [] - # Enrich each issue with full detail and exhaustive nested data. - try: - full_issue = await client.get_issue( - owner=repository.owner, - repo=repository.repo, - issue_number=forgejo_number, - ) - maybe_full = _as_dict(full_issue) - if maybe_full is not None: - issue_payload = maybe_full - except Exception as exc: - logger.warning( - "issue_detail_sync_failed", - extra={ - "repository_id": str(repository_id), - "issue_number": forgejo_number, - "error": str(exc), - }, - ) - - try: - comments_payload = _as_dict_list( - await client.list_issue_comments( + # Enrich each issue with full detail and exhaustive nested data. + try: + full_issue = await client.get_issue( owner=repository.owner, repo=repository.repo, issue_number=forgejo_number, ) - ) - except Exception as exc: - logger.warning( - "issue_comments_sync_failed", - extra={ - "repository_id": str(repository_id), - "issue_number": forgejo_number, - "error": str(exc), - }, - ) - - try: - timeline_payload = _as_dict_list( - await client.list_issue_timeline( - owner=repository.owner, - repo=repository.repo, - issue_number=forgejo_number, + maybe_full = _as_dict(full_issue) + if maybe_full is not None: + issue_payload = maybe_full + except Exception as exc: + logger.warning( + "issue_detail_sync_failed", + extra={ + "repository_id": str(repository_id), + "issue_number": forgejo_number, + "error": str(exc), + }, ) - ) - except Exception as exc: - logger.warning( - "issue_timeline_sync_failed", - extra={ - "repository_id": str(repository_id), - "issue_number": forgejo_number, - "error": str(exc), - }, - ) - try: - reactions_payload = _as_dict_list( - await client.list_issue_reactions( - owner=repository.owner, - repo=repository.repo, - issue_number=forgejo_number, + try: + comments_payload = _as_dict_list( + await client.list_issue_comments( + owner=repository.owner, + repo=repository.repo, + issue_number=forgejo_number, + ) + ) + except Exception as exc: + logger.warning( + "issue_comments_sync_failed", + extra={ + "repository_id": str(repository_id), + "issue_number": forgejo_number, + "error": str(exc), + }, ) - ) - except Exception as exc: - logger.warning( - "issue_reactions_sync_failed", - extra={ - "repository_id": str(repository_id), - "issue_number": forgejo_number, - "error": str(exc), - }, - ) - # Parse labels - labels_data = [] - for label in issue_payload.get("labels") or []: - labels_data.append( - { - "id": label.get("id"), - "name": label.get("name", ""), - "color": label.get("color", ""), - "description": label.get("description", ""), + try: + timeline_payload = _as_dict_list( + await client.list_issue_timeline( + owner=repository.owner, + repo=repository.repo, + issue_number=forgejo_number, + ) + ) + except Exception as exc: + logger.warning( + "issue_timeline_sync_failed", + extra={ + "repository_id": str(repository_id), + "issue_number": forgejo_number, + "error": str(exc), + }, + ) + + try: + reactions_payload = _as_dict_list( + await client.list_issue_reactions( + owner=repository.owner, + repo=repository.repo, + issue_number=forgejo_number, + ) + ) + except Exception as exc: + logger.warning( + "issue_reactions_sync_failed", + extra={ + "repository_id": str(repository_id), + "issue_number": forgejo_number, + "error": str(exc), + }, + ) + + # Parse labels + labels_data = [] + for label in issue_payload.get("labels") or []: + labels_data.append( + { + "id": label.get("id"), + "name": label.get("name", ""), + "color": label.get("color", ""), + "description": label.get("description", ""), + } + ) + + # Parse assignees + assignees_data = [] + for assignee in issue_payload.get("assignees") or []: + assignees_data.append( + { + "login": assignee.get("login", ""), + "id": assignee.get("id", 0), + "avatar_url": assignee.get("avatar_url", ""), + } + ) + + # Parse milestone + milestone_data = None + raw_milestone = issue_payload.get("milestone") + if raw_milestone and isinstance(raw_milestone, dict): + milestone_data = { + "id": raw_milestone.get("id"), + "title": raw_milestone.get("title", ""), + "state": raw_milestone.get("state", "open"), + "description": raw_milestone.get("description") or None, + "due_on": raw_milestone.get("due_on") or None, + "closed_at": raw_milestone.get("closed_at") or None, } - ) - # Parse assignees - assignees_data = [] - for assignee in issue_payload.get("assignees") or []: - assignees_data.append( - { - "login": assignee.get("login", ""), - "id": assignee.get("id", 0), - "avatar_url": assignee.get("avatar_url", ""), - } - ) + # Full body and preview + raw_body = issue_payload.get("body") or "" + body_full = raw_body if raw_body else None + body_preview = raw_body[:1000] if raw_body else None - # Parse milestone - milestone_data = None - raw_milestone = issue_payload.get("milestone") - if raw_milestone and isinstance(raw_milestone, dict): - milestone_data = { - "id": raw_milestone.get("id"), - "title": raw_milestone.get("title", ""), - "state": raw_milestone.get("state", "open"), - "description": raw_milestone.get("description") or None, - "due_on": raw_milestone.get("due_on") or None, - "closed_at": raw_milestone.get("closed_at") or None, - } + # Parse dates — required fields fall back to utcnow(), optional closed_at stays None + created_at = self._parse_iso_date(issue_payload.get("created_at")) or utcnow() + updated_at = self._parse_iso_date(issue_payload.get("updated_at")) or utcnow() + closed_at = self._parse_iso_date(issue_payload.get("closed_at")) - # Full body and preview - raw_body = issue_payload.get("body") or "" - body_full = raw_body if raw_body else None - body_preview = raw_body[:1000] if raw_body else None + # Check if issue exists + existing = await self._find_issue(repository_id, forgejo_number) - # Parse dates — required fields fall back to utcnow(), optional closed_at stays None - created_at = self._parse_iso_date(issue_payload.get("created_at")) or utcnow() - updated_at = self._parse_iso_date(issue_payload.get("updated_at")) or utcnow() - closed_at = self._parse_iso_date(issue_payload.get("closed_at")) + if existing is None: + issue = ForgejoIssue( + organization_id=self.organization_id, + repository_id=repository_id, + forgejo_issue_number=forgejo_number, + title=issue_data.get("title", ""), + body=body_full, + body_preview=body_preview, + state=state, + is_pull_request=False, + labels=labels_data, + assignees=assignees_data, + milestone=milestone_data, + forgejo_payload=issue_payload, + forgejo_comments_payload=comments_payload, + forgejo_timeline_payload=timeline_payload, + forgejo_reactions_payload=reactions_payload, + author=_author_login(issue_payload), + html_url=_html_url(issue_payload), + forgejo_created_at=created_at, + forgejo_updated_at=updated_at, + forgejo_closed_at=closed_at, + ) + self.session.add(issue) + await self.session.flush() + created += 1 + else: + existing.title = issue_data.get("title", "") + existing.body = body_full + existing.body_preview = body_preview + existing.state = state + existing.labels = labels_data + existing.assignees = assignees_data + existing.milestone = milestone_data + existing.forgejo_payload = issue_payload + existing.forgejo_comments_payload = comments_payload + existing.forgejo_timeline_payload = timeline_payload + existing.forgejo_reactions_payload = reactions_payload + existing.author = _author_login(issue_payload) + existing.html_url = _html_url(issue_payload) + existing.forgejo_created_at = created_at + existing.forgejo_updated_at = updated_at + existing.forgejo_closed_at = closed_at + existing.last_synced_at = utcnow() + await crud.save(self.session, existing) + updated_count += 1 - # Check if issue exists - existing = await self._find_issue(repository_id, forgejo_number) + if state == "open": + open_count += 1 + elif state == "closed": + closed_count += 1 - if existing is None: - issue = ForgejoIssue( - organization_id=self.organization_id, - repository_id=repository_id, - forgejo_issue_number=forgejo_number, - title=issue_data.get("title", ""), - body=body_full, - body_preview=body_preview, - state=state, - is_pull_request=False, - labels=labels_data, - assignees=assignees_data, - milestone=milestone_data, - forgejo_payload=issue_payload, - forgejo_comments_payload=comments_payload, - forgejo_timeline_payload=timeline_payload, - forgejo_reactions_payload=reactions_payload, - author=_author_login(issue_payload), - html_url=_html_url(issue_payload), - forgejo_created_at=created_at, - forgejo_updated_at=updated_at, - forgejo_closed_at=closed_at, - ) - self.session.add(issue) - await self.session.flush() - created += 1 - else: - existing.title = issue_data.get("title", "") - existing.body = body_full - existing.body_preview = body_preview - existing.state = state - existing.labels = labels_data - existing.assignees = assignees_data - existing.milestone = milestone_data - existing.forgejo_payload = issue_payload - existing.forgejo_comments_payload = comments_payload - existing.forgejo_timeline_payload = timeline_payload - existing.forgejo_reactions_payload = reactions_payload - existing.author = _author_login(issue_payload) - existing.html_url = _html_url(issue_payload) - existing.forgejo_created_at = created_at - existing.forgejo_updated_at = updated_at - existing.forgejo_closed_at = closed_at - existing.last_synced_at = utcnow() - await crud.save(self.session, existing) - updated_count += 1 - - if state == "open": - open_count += 1 - elif state == "closed": - closed_count += 1 - - # If we got fewer than limit, we're done - if len(issues) < limit: - break + # If we got fewer than limit, we're done + if len(issues) < limit: + break current_page += 1 # Sync repository label catalog