feat(auth): sort OAuth scopes by service, then access level#413
feat(auth): sort OAuth scopes by service, then access level#413anshul-garg27 wants to merge 3 commits intogoogleworkspace:mainfrom
Conversation
Previously scopes were sorted by risk classification first (restricted, sensitive, non-sensitive), making it hard to find all scopes for a specific service. Now scopes are grouped by service name (Drive, Gmail, etc.), then by access level (read-only first), then by classification (safe before restricted), then alphabetically. This makes the scope picker much easier to navigate when selecting scopes for specific services. Closes googleworkspace#319
🦋 Changeset detectedLatest commit: bf99daf The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the OAuth scope sorting mechanism within the TUI picker to provide a more intuitive and user-friendly experience. Previously, scopes were scattered based on risk level, making selection cumbersome. The new sorting prioritizes grouping by service and access level, streamlining the process for users to identify and choose necessary permissions. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the user experience by sorting OAuth scopes more logically in the TUI. The implementation of the new sorting logic is correct. However, I've identified two areas for improvement in the new unit test:
- There is code duplication between the test and the production code, which should be refactored to improve maintainability.
- The test case is incomplete and doesn't fully validate all aspects of the new sorting criteria.
Please see my detailed comments for suggestions on how to address these points.
src/setup.rs
Outdated
| scopes.sort_by(|a, b| { | ||
| a.api_name | ||
| .cmp(&b.api_name) | ||
| .then_with(|| b.is_readonly.cmp(&a.is_readonly)) | ||
| .then_with(|| a.classification.cmp(&b.classification)) | ||
| .then_with(|| a.short.cmp(&b.short)) | ||
| }); |
There was a problem hiding this comment.
This sorting logic is a duplicate of the implementation in fetch_scopes_for_apis. Duplicating production logic in tests makes the code harder to maintain. If the sorting rules change in the future, they would need to be updated in two places, and it's easy to forget the test, leading to it no longer verifying the correct behavior.
To avoid this, I recommend extracting the comparison logic into a new private helper function at the module level.
For example:
// In src/setup.rs (module level)
fn compare_scopes(a: &DiscoveredScope, b: &DiscoveredScope) -> std::cmp::Ordering {
a.api_name
.cmp(&b.api_name)
.then_with(|| b.is_readonly.cmp(&a.is_readonly))
.then_with(|| a.classification.cmp(&b.classification))
.then_with(|| a.short.cmp(&b.short))
}Then, both fetch_scopes_for_apis and this test can use all_scopes.sort_by(compare_scopes); and scopes.sort_by(compare_scopes); respectively. This ensures you're testing the exact same logic that runs in production.
| fn scope_sort_groups_by_service_then_access_then_classification() { | ||
| let mut scopes = vec![ | ||
| DiscoveredScope { | ||
| url: "https://www.googleapis.com/auth/gmail".to_string(), | ||
| short: "gmail".to_string(), | ||
| description: "Full Gmail access".to_string(), | ||
| api_name: "Gmail".to_string(), | ||
| is_readonly: false, | ||
| classification: ScopeClassification::Restricted, | ||
| }, | ||
| DiscoveredScope { | ||
| url: "https://www.googleapis.com/auth/drive.readonly".to_string(), | ||
| short: "drive.readonly".to_string(), | ||
| description: "Read-only Drive".to_string(), | ||
| api_name: "Drive".to_string(), | ||
| is_readonly: true, | ||
| classification: ScopeClassification::NonSensitive, | ||
| }, | ||
| DiscoveredScope { | ||
| url: "https://www.googleapis.com/auth/drive".to_string(), | ||
| short: "drive".to_string(), | ||
| description: "Full Drive access".to_string(), | ||
| api_name: "Drive".to_string(), | ||
| is_readonly: false, | ||
| classification: ScopeClassification::Restricted, | ||
| }, | ||
| DiscoveredScope { | ||
| url: "https://www.googleapis.com/auth/gmail.readonly".to_string(), | ||
| short: "gmail.readonly".to_string(), | ||
| description: "Read-only Gmail".to_string(), | ||
| api_name: "Gmail".to_string(), | ||
| is_readonly: true, | ||
| classification: ScopeClassification::NonSensitive, | ||
| }, | ||
| ]; | ||
|
|
||
| scopes.sort_by(|a, b| { | ||
| a.api_name | ||
| .cmp(&b.api_name) | ||
| .then_with(|| b.is_readonly.cmp(&a.is_readonly)) | ||
| .then_with(|| a.classification.cmp(&b.classification)) | ||
| .then_with(|| a.short.cmp(&b.short)) | ||
| }); | ||
|
|
||
| // Drive scopes first (alphabetically before Gmail) | ||
| assert_eq!(scopes[0].short, "drive.readonly"); | ||
| assert_eq!(scopes[1].short, "drive"); | ||
| // Gmail scopes second | ||
| assert_eq!(scopes[2].short, "gmail.readonly"); | ||
| assert_eq!(scopes[3].short, "gmail"); | ||
| } |
There was a problem hiding this comment.
The test plan mentions verifying the sort order by service, access level, and classification, but this test case only effectively validates the first two criteria (api_name and is_readonly). The current test data doesn't include items that would have the same api_name and is_readonly status but different classification or short names, so the third and fourth levels of the sort logic are not being exercised.
To make the test more robust and fully cover the new sorting logic, please consider adding more scopes to the test vector. For example, you could add two scopes with the same api_name and is_readonly value but different classifications to verify that classification sorting works as expected.
Address Gemini review feedback: - Extract inline sort closure into a reusable compare_scopes function to avoid duplicating sort logic between production code and tests - Add a Sensitive-classified scope to the test to verify all four sort criteria (service, access level, classification, name)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the OAuth scope sorting logic to group scopes by service and then access level, which improves the user experience in the TUI picker. The implementation correctly extracts the sorting logic into a dedicated compare_scopes function and adds a unit test to verify the new behavior. My feedback includes a suggestion to make the new test assertions more robust and maintainable.
|
/gemini review |
Replace brittle index-based assertions with a collected Vec comparison so the test remains maintainable as scopes change over time.
anshul-garg27
force-pushed
the
feat/sort-scopes-by-service
branch
from
6f6bdfc to
bf99daf
Compare
There was a problem hiding this comment.
Code Review
This pull request changes the sorting order of OAuth scopes in the TUI picker. The new logic sorts scopes by service name, then access level, then sensitivity classification. This is achieved by introducing a new compare_scopes function and applying it in fetch_scopes_for_apis. A new unit test is included to verify the correctness of the new sorting logic. The implementation appears to correctly reflect the intended changes.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new sorting logic for OAuth scopes in the TUI picker, grouping them by service, then access level, and finally classification. The implementation is clean, with a dedicated compare_scopes function for the new logic and a comprehensive unit test to verify its correctness. The changes effectively address the issue of scattered scopes and improve the user experience when selecting them. The code is well-structured and the changes are correctly implemented.
3 participants
Summary
Sort OAuth scopes in the TUI picker by service name first (grouping all Drive scopes together, all Gmail together, etc.), then by access level (read-only before write), then by sensitivity classification.
Problem
The current sort order groups scopes by risk level (restricted → sensitive → non-sensitive), which scatters scopes from the same service across different sections of the list. When a user wants to select specific Drive or Gmail scopes, they have to scan through the entire list.
Solution
Changed the sort key in
fetch_scopes_for_apis()from:to:
Before: Scopes scattered by risk level
After: All Drive scopes together, all Gmail scopes together, read-only variants listed before write variants
Test plan
Closes #319