Skip to content

Bug 2033015 - Review requests to rotation groups are missing from the Phabricator ETL#17

Open
dklawren wants to merge 2 commits intomainfrom
2033015
Open

Bug 2033015 - Review requests to rotation groups are missing from the Phabricator ETL#17
dklawren wants to merge 2 commits intomainfrom
2033015

Conversation

@dklawren
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Phabricator ETL extraction logic to correctly capture reviewer changes and reviewer-group (rotation group) review requests, addressing missing data in downstream analytics.

Changes:

  • Adds "differential.revision.reviewers" to the set of tracked transaction types.
  • Introduces get_project_name() and uses it when reviewer PHIDs refer to projects/groups.
  • Adds convert_json_to_string() to parse reviewer-change transactions whose old/new values are JSON maps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread phabricator_etl/stats.py
Comment thread phabricator_etl/stats.py
Comment thread phabricator_etl/stats.py Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread phabricator_etl/stats.py
Comment on lines +286 to +314
_user_name_cache: dict[str | bytes, Optional[str]] = {}
_project_name_cache: dict[str | bytes, Optional[str]] = {}


def get_user_name(author_phid: str | bytes, sessions: Sessions) -> Optional[str]:
if author_phid not in _user_name_cache:
try:
user = sessions.users.query(UserDb.User).filter_by(phid=author_phid).one()
_user_name_cache[author_phid] = user.userName
except NoResultFound:
_user_name_cache[author_phid] = None
return _user_name_cache[author_phid]


def get_project_name(project_phid: str | bytes, sessions: Sessions) -> Optional[str]:
if project_phid not in _project_name_cache:
try:
project = (
sessions.projects.query(ProjectDb.Project)
.filter_by(phid=project_phid)
.one()
)
_project_name_cache[project_phid] = project.name
except NoResultFound:
_project_name_cache[project_phid] = None
return _project_name_cache[project_phid]

def get_user_email(author_phid: str, sessions: Sessions) -> Optional[str]:

def get_user_email(author_phid: str | bytes, sessions: Sessions) -> Optional[str]:
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

str | bytes type unions require Python 3.10+, but this repo targets Python 3.9 (see .python-version and pyproject.toml requires-python). Please replace these annotations with typing.Union[str, bytes] (and update the cache type hints similarly), or bump the project’s minimum Python version to 3.10+ if that’s intended.

Copilot uses AI. Check for mistakes.
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.

2 participants