Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attachments - Policy and Service updates for Group and Project attachments #752

Merged
merged 12 commits into from
Sep 12, 2024

Conversation

ChrisHuynh333
Copy link
Collaborator

@ChrisHuynh333 ChrisHuynh333 commented Sep 12, 2024

What does this PR do and why?

This policy updates the policies and attachments create service for adding attachments to groups and projects

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other pull requests.

How to set up and validate locally

  1. Migrate your database
  2. Verify tests pass locally

PR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

This comment has been minimized.

@ChrisHuynh333 ChrisHuynh333 changed the title Attachments - Group and Project Attachment Controllers and Policies Attachments - Policy and Service updates for Group and Project attachments Sep 12, 2024

This comment has been minimized.

This comment has been minimized.

@ChrisHuynh333 ChrisHuynh333 marked this pull request as ready for review September 12, 2024 16:22

This comment has been minimized.

Copy link
Member

@ericenns ericenns left a comment

Choose a reason for hiding this comment

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

Looking good, just need a few more tests as specified in the comments. But in addition tests for the attachments destroy service are needed.

app/policies/group_policy.rb Show resolved Hide resolved
app/policies/project_policy.rb Show resolved Hide resolved
@ChrisHuynh333 ChrisHuynh333 self-assigned this Sep 12, 2024

This comment has been minimized.

@ChrisHuynh333 ChrisHuynh333 force-pushed the namespace-attachments-backend branch from cb9ba2a to 291e265 Compare September 12, 2024 18:29

This comment has been minimized.

ericenns
ericenns previously approved these changes Sep 12, 2024
Copy link
Member

@ericenns ericenns left a comment

Choose a reason for hiding this comment

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

Looks good to me!

This comment has been minimized.

Copy link

Code Metrics Report

Coverage Test Execution Time
93.0% 7m18s

Code coverage of files in pull request scope (96.6%)

Files Coverage
app/models/member.rb 96.8%
app/models/namespace.rb 95.6%
app/policies/group_policy.rb 95.1%
app/policies/project_policy.rb 96.0%
app/services/attachments/create_service.rb 100.0%
app/services/attachments/destroy_service.rb 96.8%
app/views/layouts/projects.html.erb 100.0%

Reported by octocov

Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ericenns ericenns merged commit 4640a3d into main Sep 12, 2024
3 checks passed
@ericenns ericenns deleted the namespace-attachments-backend branch September 12, 2024 20:03
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.

3 participants