Name the granted and requested scopes in the authorization activity, PG-5168 - #44
Merged
Conversation
The authorization decision was recorded with both the granted scope and the scopes the client requested, but the activity log described the client only, so the scopes could not be seen without reading the stored payload. The description now names them. The descriptions go through translation keys, as the activity classes in ActivityLog and the other plugins do, with one complete sentence per key so the wording can be reordered when translated. Decisions recorded before those scopes were stored keep describing the client on its own, as there is nothing else to show for them. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The create, update, delete, rotate secret and pause or resume activities still built their descriptions in English with sprintf, so they could never be translated. They now use translation keys like the authorization activity and like the activity classes in ActivityLog and the other plugins. The test covers every description in the plugin now, so it is named after that rather than after the one activity it started with. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The activity classes extend one from ActivityLog, so the tests covering their descriptions cannot load the classes at all unless that plugin is installed, which it is not in CI by default. The generator comment did not list the dependent plugins either, so regenerating the workflow would have dropped the existing one as well. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The dependent plugin was declared but the workflow never passed a token, so the checkout of the private repository was unauthenticated and skipped, leaving the activity tests without the class they extend. The other matomo-org plugins that depend on an innocraft plugin pass this token. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The workflow already passed this token, so adding it again left two identical keys in the same step and the workflow file was rejected. The checkout of ActivityLog failed for a different reason, which the duplicate did nothing to address. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lachiebol
previously approved these changes
Jul 31, 2026
lachiebol
left a comment
lachiebol
left a comment
Contributor
There was a problem hiding this comment.
LGTM
Few codex comments but not blocking
Blocking
- The new translation keys are not alphabetically ordered in lang/en.json:5. For example, UpdateClientActivity precedes DeleteClientActivity, and ResumeClientActivity precedes PauseClientActivity. This violates matomo-i18n-development-rules; reorder
the new keys before merging.
Medium
- Two compatibility branches in Activity/AuthorizeClient.php:51 lack coverage: an allowed activity with a granted scope but no requestedScopes, and a historical denied activity without requestedScopes. tests/Integration/ActivityDescriptionsTest.php:43
covers the other three paths. Adding these cases would protect rendering of stored historical entries.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lachiebol
approved these changes
Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-up to #43. The authorization decision was already recorded with both the granted scope and the scopes the client requested, but the activity log described only the client, so neither could be seen without reading the stored payload. The description now names them, which is what makes the audit trail useful at a glance.
While in there, the activity descriptions are now translatable. Every description in this plugin was built in English with
sprintf, unlike the activity classes in ActivityLog and the other plugins, so they could never be translated. Each variant is one complete sentence per translation key rather than assembled fragments, so the wording can be reordered when translated.Decisions recorded before the scopes were stored keep describing the client on its own, since there is nothing else to show for them. That case is covered by a test so it cannot regress into printing empty parentheses.
No version bump: 5.3.0 has not shipped yet, so this goes in the same release section.
Issue No
PG-5168
Steps to Replicate the Issue
Checklist
🤖 Generated with Claude Code