Skip to content

Fix Ready for Review button permissions - use repository write access instead of merge permissions#900

Merged
litlfred merged 4 commits intomainfrom
copilot/fix-899
Sep 2, 2025
Merged

Fix Ready for Review button permissions - use repository write access instead of merge permissions#900
litlfred merged 4 commits intomainfrom
copilot/fix-899

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Aug 31, 2025

This PR fixes a permission issue with the "Ready for Review" button in the WorkflowDashboard that was preventing users from converting draft PRs to ready-for-review status.

Problem

The "Ready for Review" button was incorrectly using merge permissions (canMergePR) to determine visibility. However, the checkPullRequestMergePermissions method returns false for draft PRs because draft PRs cannot be merged. This created a circular dependency where users couldn't mark draft PRs as ready for review even if they had the necessary repository permissions.

Solution

  • Added separate canManagePR permission check based on repository write access (checkRepositoryWritePermissions)
  • Updated button logic to use canManagePR instead of canMergePR for the "Ready for Review" button
  • Improved permission status messages to clearly distinguish between review, merge, and manage capabilities

Permission Logic

The fix separates PR management permissions from merge permissions:

  • Merge permissions: Can merge non-draft PRs (requires draft: false check)
  • Manage permissions: Can update PR draft status (requires repository write access)
  • Review permissions: Can approve/request changes (separate from merge/manage)

Users with repository write access (write, maintain, or admin permissions) can now mark draft PRs as ready for review, which aligns with GitHub's native behavior where repository collaborators can manage PR draft status independently of merge permissions.

Changes Made

  • Added canManagePR state and permission checking in PreviewBadge.js
  • Updated WorkflowDashboard.js to use canManagePR for the "Ready for Review" button condition
  • Enhanced permission status messages to provide clearer user feedback
  • Maintains all existing functionality while fixing the specific draft PR management issue

Fixes #899.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits August 31, 2025 09:56
Co-authored-by: litlfred <662242+litlfred@users.noreply.github.com>
Co-authored-by: litlfred <662242+litlfred@users.noreply.github.com>
Copilot AI changed the title [WIP] add Ready for review button Add Ready for Review button to WorkflowDashboard for draft PRs Aug 31, 2025
Copilot AI requested a review from litlfred August 31, 2025 10:01
Copilot AI temporarily deployed to branch-preview August 31, 2025 10:44 Inactive
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 31, 2025

🚀 Deployment Status: Ready for Deployment ✅

🔗 Quick Actions:


📝 Recent Change Summary

Latest commit: dc08e2e - Fix Ready for Review button permissions - use repository write access instead of merge permissions
Approved at: 2025-09-02 00:50:48 UTC


📊 Overall Progress

Branch: copilot/fix-899
Status: ✅ Approved and ready for deployment
Preview URL (after deployment): https://litlfred.github.io/sgex/copilot-fix-899/ 📋


💡 Click deploy link above, then "Run workflow". Branch name will be pre-filled.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 31, 2025

🔒 Dependency Security Check Results

✅ **No vulnerabilities found!**

All dependencies have been scanned and no security vulnerabilities were detected.

```
found 0 vulnerabilities
```

---

### ✅ Security Status: CLEAN
Your changes maintain the security posture of the project. Great job! 🎉

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 31, 2025

🔍 Page Framework Compliance Check Results

Framework Structure Compliance

sgex-workbench@1.0.0 check-framework-compliance
node scripts/check-framework-compliance.js

🔍 SGEX Page Framework Compliance Checker

Found 30 routed page components

✅ WelcomePage: 6/6 (100%) - COMPLIANT

✅ SelectProfilePage: 6/6 (100%) - COMPLIANT

✅ DAKActionSelection: 6/6 (100%) - COMPLIANT

✅ DAKSelection: 6/6 (100%) - COMPLIANT

✅ OrganizationSelection: 6/6 (100%) - COMPLIANT

✅ DAKConfiguration: 6/6 (100%) - COMPLIANT

✅ RepositorySelection: 6/6 (100%) - COMPLIANT

✅ DashboardRedirect: 6/6 (100%) - COMPLIANT

✅ TestDashboard: 6/6 (100%) - COMPLIANT

✅ BPMNViewerTestComponent: 6/6 (100%) - COMPLIANT

⚠️ DocumentationViewer: 5/6 (83%) - PARTIAL
Issues: Found 3 layout components - should only have one
Suggestions: Remove nested PageLayout components - only use one per page

⚠️ PagesManager: 5/6 (83%) - PARTIAL
Issues: Found 2 layout components - should only have one
Suggestions: Remove nested PageLayout components - only use one per page

✅ LandingPageWithFramework: 6/6 (100%) - COMPLIANT

✅ TestDocumentationPage: 6/6 (100%) - COMPLIANT

✅ AssetEditorTest: 6/6 (100%) - COMPLIANT

⚠️ BranchListingPage: 5/6 (83%) - PARTIAL
Issues: May have custom header implementation

✅ NotFound: 6/6 (100%) - COMPLIANT

⚠️ DAKFAQDemo: 3/6 (50%) - PARTIAL
Issues: Missing PageLayout wrapper, May have custom header implementation
Suggestions: Wrap component with PageLayout or AssetEditorLayout from ./framework

✅ DAKDashboard: 6/6 (100%) - COMPLIANT

✅ DAKDashboardWithFramework: 6/6 (100%) - COMPLIANT

✅ TestingViewer: 6/6 (100%) - COMPLIANT

✅ CoreDataDictionaryViewer: 6/6 (100%) - COMPLIANT

✅ ComponentEditor: 6/6 (100%) - COMPLIANT

✅ ActorEditor: 6/6 (100%) - COMPLIANT

✅ BusinessProcessSelection: 6/6 (100%) - COMPLIANT

✅ BPMNEditor: 6/6 (100%) - COMPLIANT

✅ BPMNViewer: 6/6 (100%) - COMPLIANT

✅ BPMNSource: 6/6 (100%) - COMPLIANT

✅ DecisionSupportLogicView: 6/6 (100%) - COMPLIANT

⚠️ QuestionnaireEditor: 5/6 (83%) - PARTIAL
Issues: Found 5 layout components - should only have one
Suggestions: Remove nested PageLayout components - only use one per page

📊 COMPLIANCE SUMMARY

✅ Fully Compliant: 25/30
⚠️ Partially Compliant: 5/30
❌ Non-Compliant: 0/30

📈 Overall Compliance: 83%

⚠️ IMPROVEMENTS NEEDED:
DocumentationViewer: Found 3 layout components - should only have one
PagesManager: Found 2 layout components - should only have one
BranchListingPage: May have custom header implementation
QuestionnaireEditor: Found 5 layout components - should only have one
DAKFAQDemo: Missing PageLayout wrapper, May have custom header implementation

✅ COMPLIANCE CHECK PASSED
Consider addressing partial compliance issues.

Profile Creation Compliance

Status: ✅ PASSED

sgex-workbench@1.0.0 compliance:profile
node src/tests/compliance/profileCreationCompliance.js

🔍 Starting Profile Creation Compliance Check...

📊 Profile Creation Compliance Report

📁 Files scanned: 138
🔍 Files analyzed: 138
⚠️ Violations found: 0

✅ COMPLIANCE PASSED: No profile creation violations found!


Next Steps

⚠️ Recommended: Consider addressing partial compliance issues.

For guidance, see:

@github-actions github-actions bot requested a deployment to branch-preview-copilot-fix-899 August 31, 2025 10:45 Pending
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 31, 2025

✅ Branch Preview Ready!

Branch: copilot/fix-899
Status: 🟢 Successfully Deployed
Commit: bcd3d93
Deployed: 2025-09-01 22:47:04 UTC

🌐 Preview URLs

Branch Preview
Open branch preview
Main App
Open main application

🔄 Deployment Actions

Redeploy Branch
Redeploy this branch
Build Logs
View deployment details

📋 Instructions

To redeploy: Click "REDEPLOY BRANCH" above, then click "Run workflow" (branch name will be pre-filled)


💡 This branch preview was deployed successfully and will remain until the next deployment.

Note: This deployment only updates the branch subdirectory and does not affect the landing page.

Copilot AI temporarily deployed to branch-preview August 31, 2025 10:46 Inactive
Copy link
Copy Markdown
Owner

@litlfred litlfred left a comment

Choose a reason for hiding this comment

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

@litlfred
Copy link
Copy Markdown
Owner

@copilot I clicked "Approve" button to apporve the workflow. it tuned solid green. I waited for the approval (I thought saw a dynanmic update of the approval workflows, but it finished too quickly so I don't think it worked. I expected to see a dynamic update for each of the approval workdlow running as you would see in GitHub). once the Workflow did finish I expected to see a "Ready for Review" button but I did not.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Aug 31, 2025

@copilot I clicked "Approve" button to apporve the workflow. it tuned solid green. I waited for the approval (I thought saw a dynanmic update of the approval workflows, but it finished too quickly s...

The workflow did complete successfully! I can see from the GitHub API that PR #900 is currently in draft mode ("draft":true) and the Deploy Feature Branch workflow finished with status "success".

The "Ready for Review" button only appears when you're authenticated and have merge permissions on the repository. The button logic requires:

  1. User is authenticated (isAuthenticated)
  2. PR is in draft mode (prInfo[0].draft) ✅
  3. User has merge permissions (canMergePR) ❓

If you're not seeing the button, please check:

  1. Are you signed in with your GitHub credentials in the preview page?
  2. Does your Personal Access Token have "repo" permissions?
  3. Try refreshing the page to ensure the PR status is up to date.

The preview URL where the button should appear is: https://litlfred.github.io/sgex/copilot-fix-899/

Copilot AI requested a review from litlfred August 31, 2025 11:43
Copy link
Copy Markdown
Owner

@litlfred litlfred left a comment

Choose a reason for hiding this comment

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

@litlfred
Copy link
Copy Markdown
Owner

litlfred commented Sep 1, 2025

@copilot at
https://litlfred.github.io/sgex/copilot-fix-899/
it says" You can review this PR but cannot merge it " even though I am authenticated with PAT that should have the correct permissions

… instead of merge permissions

Co-authored-by: litlfred <662242+litlfred@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 1, 2025

@copilot at
https://litlfred.github.io/sgex/copilot-fix-899/
it says" You can review this PR but cannot merge it " even though I am authenticated with PAT that should have the correct permissions

I found and fixed the issue! The problem was in the permission checking logic. The "Ready for Review" button was using merge permissions (canMergePR) to determine visibility, but the checkPullRequestMergePermissions method returns false for draft PRs because you can't merge a draft PR.

However, marking a PR as ready for review only requires repository write access, not merge permissions. I've fixed this by:

  1. Added canManagePR permission check based on repository write access (checkRepositoryWritePermissions)
  2. Updated the button condition to use canManagePR instead of canMergePR for the "Ready for Review" button
  3. Improved permission status messages to clearly show when you can manage/review/merge PRs

The fix is in commit dc08e2e. Now users with repository write permissions (write, maintain, or admin) can mark draft PRs as ready for review, regardless of whether they can merge non-draft PRs.

You should now see the "Ready for Review" button when viewing the draft PR at https://litlfred.github.io/sgex/copilot-fix-899/ with your authenticated PAT.

Copilot AI changed the title Add Ready for Review button to WorkflowDashboard for draft PRs Fix Ready for Review button permissions - use repository write access instead of merge permissions Sep 1, 2025
Copilot AI requested a review from litlfred September 1, 2025 14:33
@github-actions github-actions bot requested a deployment to branch-preview-copilot-fix-899 September 1, 2025 22:44 Pending
@litlfred litlfred marked this pull request as ready for review September 2, 2025 00:50
@litlfred litlfred merged commit ca32327 into main Sep 2, 2025
14 checks passed
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.

add Ready for review button

2 participants