Clarify error message for private Phabricator revisions#6138
Conversation
| patch = SanitizedPhabricatorPatch(revision_id=revision_id) | ||
| if not patch.is_public(): | ||
| raise ToolError( | ||
| f"Revision D{revision_id} is private and cannot be accessed." |
There was a problem hiding this comment.
This won’t work since we will always get PhabricatorRevisionNotFoundException on secure revisions.
There was a problem hiding this comment.
Ok, but this is what I said above.
There was a problem hiding this comment.
Since the Web UI itself can differentiate between 404 and secure revisions, we could fall back to scraping the web URL when a revision is not found, if you think the distinction would be useful.
What you have for blocking secure revisions could be helpful anyway as a defensive mechanism; so if the account accidentally gains access to a secure revision, it won't be publicly available.
Nit: we do the check differently in Review Helper, we could align it for consistency:
Previously, attempting to access a private revision would surface a confusing internal error. Now it raises a ToolError with an explicit message explaining the revision is private or not found. Fixes mozilla#5992
padenot
force-pushed
the
fix-issue-5992-private-revision-error
branch
from
c17863c to
c7ea047
Compare
suhaibmujahid
left a comment
suhaibmujahid
left a comment
There was a problem hiding this comment.
Could you please fix the linting error?
Align with the pattern used in review_processor.py.
2 participants
Previously, attempting to access a private revision would surface a confusing internal error. Now it raises a
ToolErrorwith an explicit message explaining the revision is private or not found.I'm not entirely certain that it's that useful to distinguish, because our token isn't capable of seeing all revisions, but maybe that will change in the future, and feels right anyway.
Fixes #5992