Skip to content

Conversation

@wvangeit
Copy link
Contributor

What do these changes do?

This is in preparation for the sharing of functions in the function browser. I expose two methods in the function service layer to change and remove group permissions for functions.
(It also a few small changes based on comments in a previous PR that i forgot to commit).

Related issue/s

ITISFoundation/osparc-issues#1905

How to test

There is a new test file for the service layer (without rpc).
Register a function, give another group access, test if they have access, remove the access, test if they don't have access anymore.

Dev-ops

No changes

@wvangeit wvangeit added this to the Voyager milestone Aug 13, 2025
@wvangeit wvangeit self-assigned this Aug 13, 2025
@wvangeit wvangeit added the a:webserver webserver's codebase. Assigning the area is particularly useful for bugs label Aug 13, 2025
@wvangeit wvangeit requested a review from Copilot August 13, 2025 11:31

This comment was marked as outdated.

@wvangeit wvangeit requested a review from Copilot August 13, 2025 11:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR exposes two new service layer methods (set_function_group_permissions and remove_function_group_permissions) to manage group permissions for functions, preparing for function sharing in the function browser.

Key changes include:

  • Adding new permission management methods in the functions service layer
  • Refactoring existing permission functions to support both internal and external use
  • Adding comprehensive test coverage for the new permission functionality
  • Minor import organization improvements

Reviewed Changes

Copilot reviewed 8 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test_functions_service.py New test file validating permission setting and removal functionality
_functions_service.py Added two new public methods for group permission management
_functions_repository.py Refactored permission functions, added user permission checks and new remove functionality
test_functions_controller_rpc.py Removed unnecessary blank lines and import comments
studies_jobs.py Removed unnecessary pylint disable comment
functions_routes.py Reorganized imports to use relative imports
function_jobs_routes.py Reorganized imports to use relative imports
function_job_collections_routes.py Added limited_gather import and reorganized other imports

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

Thanks! left a comment

@codecov
Copy link

codecov bot commented Aug 13, 2025

Codecov Report

❌ Patch coverage is 84.44444% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.23%. Comparing base (64d6099) to head (07dc539).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8211      +/-   ##
==========================================
- Coverage   86.59%   86.23%   -0.36%     
==========================================
  Files        1910      852    -1058     
  Lines       73365    37950   -35415     
  Branches     1301      176    -1125     
==========================================
- Hits        63529    32727   -30802     
+ Misses       9448     5165    -4283     
+ Partials      388       58     -330     
Flag Coverage Δ
integrationtests 64.24% <17.14%> (-0.04%) ⬇️
unittests 88.35% <84.44%> (+3.14%) ⬆️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 77.10% <ø> (-7.95%) ⬇️
agent ∅ <ø> (∅)
api_server 93.20% <90.00%> (-0.01%) ⬇️
autoscaling ∅ <ø> (∅)
catalog ∅ <ø> (∅)
clusters_keeper ∅ <ø> (∅)
dask_sidecar ∅ <ø> (∅)
datcore_adapter ∅ <ø> (∅)
director ∅ <ø> (∅)
director_v2 77.98% <ø> (-12.90%) ⬇️
dynamic_scheduler ∅ <ø> (∅)
dynamic_sidecar 87.23% <ø> (-2.89%) ⬇️
efs_guardian ∅ <ø> (∅)
invitations ∅ <ø> (∅)
payments ∅ <ø> (∅)
resource_usage_tracker ∅ <ø> (∅)
storage ∅ <ø> (∅)
webclient ∅ <ø> (∅)
webserver 88.23% <82.85%> (+5.03%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64d6099...07dc539. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mergify
Copy link
Contributor

mergify bot commented Aug 13, 2025

🧪 CI Insights

Here's what we observed from your CI run for 07dc539.

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on base branch Retries 🔍 CI Insights 📄 Logs
CI [sys] public api (3.11, ubuntu-24.04) You had a 17% chance of failing… lucky you! 🎲 Flaky Configure an automatic retry View View
system-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View
unit-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View

@wvangeit
Copy link
Contributor Author

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Aug 13, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images
      • any of: [🛡 GitHub branch protection]
        • check-success = SonarCloud Code Analysis
        • check-neutral = SonarCloud Code Analysis
        • check-skipped = SonarCloud Code Analysis
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@wvangeit wvangeit added the 🤖-automerge marks PR as ready to be merged for Mergify label Aug 13, 2025
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

OK with me

@sonarqubecloud
Copy link

Copy link
Contributor

@bisgaard-itis bisgaard-itis left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@wvangeit
Copy link
Contributor Author

@mergify refresh

@mergify
Copy link
Contributor

mergify bot commented Aug 14, 2025

refresh

✅ Pull request refreshed

@sanderegg sanderegg merged commit 81d1bd9 into ITISFoundation:master Aug 14, 2025
92 of 95 checks passed
@wvangeit wvangeit deleted the permission_service branch August 14, 2025 09:07
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Sep 2, 2025
61 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:webserver webserver's codebase. Assigning the area is particularly useful for bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants