organisation user pins by raphael-goetz · Pull Request #953 · code0-tech/sagittarius

raphael-goetz · 2026-05-04T14:43:55Z

Resolves: #556

github-actions · 2026-05-04T14:46:37Z

GitLab Pipeline Action

General information

Link to pipeline: https://gitlab.com/code0-tech/development/sagittarius/-/pipelines/2499021059

Status: Passed
Duration: 6 minutes

Job summaries

rspec: [cloud]

Coverage report available at https://code0-tech.gitlab.io/-/development/sagittarius/-/jobs/14206894193/artifacts/tmp/coverage/index.html
Test summary available at https://gitlab.com/code0-tech/development/sagittarius/-/pipelines/2499021059/test_report
Finished in 22.85 seconds (files took 13.25 seconds to load)
1403 examples, 0 failures
Line Coverage: 92.56% (4565 / 4932)
[TEST PROF INFO] Time spent in factories: 00:12.395 (42.53% of total time)

rspec: [ee]

Coverage report available at https://code0-tech.gitlab.io/-/development/sagittarius/-/jobs/14206894192/artifacts/tmp/coverage/index.html
Test summary available at https://gitlab.com/code0-tech/development/sagittarius/-/pipelines/2499021059/test_report
Finished in 24.84 seconds (files took 12.26 seconds to load)
1381 examples, 0 failures
Line Coverage: 92.92% (4465 / 4805)
[TEST PROF INFO] Time spent in factories: 00:13.294 (42.19% of total time)

rspec: [ce]

Coverage report available at https://code0-tech.gitlab.io/-/development/sagittarius/-/jobs/14206894191/artifacts/tmp/coverage/index.html
Test summary available at https://gitlab.com/code0-tech/development/sagittarius/-/pipelines/2499021059/test_report
Finished in 21.78 seconds (files took 14.22 seconds to load)
1331 examples, 0 failures
Line Coverage: 92.44% (4294 / 4645)
[TEST PROF INFO] Time spent in factories: 00:12.204 (39.24% of total time)

docs:preview

Documentation preview available at https://code0-tech.gitlab.io/-/development/telescopium/-/jobs/14206929445/artifacts/out/index.html

rubocop

788 files inspected, no offenses detected

Taucher2003 · 2026-05-09T13:39:31Z

+               [Types::GlobalIdType[::Organization]],
+               required: true,
+               description: 'Ordered list of organization IDs to pin for the user.'
+      argument :user_id, Types::GlobalIdType[::User], required: true, description: 'ID of the user to update.'


I would have used the current user rather than passing it as an argument. Or do you see a use case for an user updating the pins of another user?

Taucher2003 · 2026-05-09T13:41:20Z

+
+    authorize :read_user_organization_pin
+
+    field :organization, Types::OrganizationType, null: false, description: 'The pinned organization'


If a user isn't allowed to see the organization (maybe they got removed from it after pinning it), this field will be null due to authorization. If the field couldn't be null, this will bubble up to the first nullable field and return an error.

Suggested change

field :organization, Types::OrganizationType, null: false, description: 'The pinned organization'

field :organization, Types::OrganizationType, null: true, description: 'The pinned organization'

Taucher2003 · 2026-05-09T13:42:40Z

+    end
+
+    def pinned_organizations
+      organization_pins.map(&:organization)


This would load all pins of the user and map them in memory with N+1 queries.

Suggested change

organization_pins.map(&:organization)

object.pinned_organizations

We would loose the sorting though. I think we can probably remove the whole pinned_organizations field and only expose organization_pins.

Taucher2003

I think the Namespaces::Members::DeleteService should remove an organization pin when a user is removed from that organization.

Taucher2003 · 2026-05-09T16:38:13Z

  has_many :namespaces, through: :namespace_memberships, inverse_of: :users

  has_many :user_identities, inverse_of: :user
+  has_many :user_organization_pins, -> { order(priority: :asc) }, inverse_of: :user, dependent: :delete_all


We use ON DELETE CASCADE in the schema, so we don't need this. Let the database handle the deletion of these records rather than rails doing it manually

Suggested change

has_many :user_organization_pins, -> { order(priority: :asc) }, inverse_of: :user, dependent: :delete_all

has_many :user_organization_pins, -> { order(priority: :asc) }, inverse_of: :user

Taucher2003 · 2026-05-09T16:38:25Z

 class Organization < ApplicationRecord
  include NamespaceParent

+  has_many :user_organization_pins, inverse_of: :organization, dependent: :delete_all


We use ON DELETE CASCADE in the schema, so we don't need this. Let the database handle the deletion of these records rather than rails doing it manually

Suggested change

has_many :user_organization_pins, inverse_of: :organization, dependent: :delete_all

has_many :user_organization_pins, inverse_of: :organization

Taucher2003 · 2026-05-09T16:41:52Z

+  validates :priority, presence: true, numericality: { only_integer: true, greater_than_or_equal_to: 0 }
+  validates :organization_id, uniqueness: { scope: :user_id }
+  validates :priority, uniqueness: { scope: :user_id }


Works, but we can use a single validates for priority

Suggested change

validates :priority, presence: true, numericality: { only_integer: true, greater_than_or_equal_to: 0 }

validates :organization_id, uniqueness: { scope: :user_id }

validates :priority, uniqueness: { scope: :user_id }

validates :priority, presence: true,

numericality: { only_integer: true, greater_than_or_equal_to: 0 },

uniqueness: { scope: :user_id }

validates :organization_id, uniqueness: { scope: :user_id }

Taucher2003 · 2026-05-09T16:43:52Z

+class UserOrganizationPinPolicy < BasePolicy
+  delegate { subject.user }
+
+  rule { can?(:read_user) }.enable :read_user_organization_pin


Either we don't need this here or we don't need the enable for this ability in the UserPolicy.

Also, I don't think someone should be able to read the pins of another user.

Suggested change

rule { can?(:read_user) }.enable :read_user_organization_pin

condition(:user_is_self) { subject.id == user&.id }

rule { user_is_self }.enable :read_user_organization_pin

Taucher2003 · 2026-05-09T16:44:10Z

+  rule { ~anonymous }.policy do
+    enable :read_user
+    enable :read_user_organization_pin
+  end


Suggested change

rule { ~anonymous }.policy do

enable :read_user

enable :read_user_organization_pin

end

rule { ~anonymous }.enable :read_user

Taucher2003 · 2026-05-09T16:49:54Z

+        return ServiceResponse.error(message: 'Missing permission', error_code: :missing_permission)
+      end
+
+      organizations = Organization.where(id: organization_ids)


A user should only be able to pin organizations that it is a member of

Suggested change

organizations = Organization.where(id: organization_ids)

organizations = OrganizationsFinder.new(id: organization_ids, namespace_member_user: user).execute

Taucher2003 · 2026-05-09T16:51:13Z

+        end
+
+        AuditService.audit(
+          :user_updated,


This should be its own event

Taucher2003 · 2026-05-09T16:52:24Z

+
+          t.rollback_and_return! ServiceResponse.error(
+            message: 'Failed to update user organization pins',
+            error_code: :invalid_user,


This should probably also be its own error code

Taucher2003 · 2026-05-09T16:56:31Z

+
+      pins = user.reload.user_organization_pins.order(:priority)
+      expect(pins.pluck(:organization_id)).to eq(organization_ids)
+      expect(pins.pluck(:priority)).to eq([0, 1])


This will always be successful because the test orders the pins by priority

Taucher2003 · 2026-05-09T17:02:10Z

+    end
+
+    def execute
+      unless Ability.allowed?(current_authentication, :update_user, user)


Using update_user allows an admin to change the pins of a user. If we don't want that, we need a separate ability

raphael-goetz added 4 commits May 4, 2026 16:42

feat: added user organisation pins migrations

1b8bfe6

feat: added pin organisation user service

65a8ca0

docs: regen

27b575c

feat: added specs for user organisation pins

169e976

raphael-goetz self-assigned this May 4, 2026

ref: satisfied rubocop

cc62ab6

Taucher2003 reviewed May 9, 2026

View reviewed changes


		authorize :read_user_organization_pin

		field :organization, Types::OrganizationType, null: false, description: 'The pinned organization'

	organization_pins.map(&:organization)
	object.pinned_organizations

	has_many :user_organization_pins, -> { order(priority: :asc) }, inverse_of: :user, dependent: :delete_all
	has_many :user_organization_pins, -> { order(priority: :asc) }, inverse_of: :user

	has_many :user_organization_pins, inverse_of: :organization, dependent: :delete_all
	has_many :user_organization_pins, inverse_of: :organization

-  rule { can?(:read_user) }.enable :read_user_organization_pin
+  condition(:user_is_self) { subject.id == user&.id }
+  rule { user_is_self }.enable :read_user_organization_pin

	organizations = Organization.where(id: organization_ids)
	organizations = OrganizationsFinder.new(id: organization_ids, namespace_member_user: user).execute

Conversation

raphael-goetz commented May 4, 2026

Uh oh!

github-actions Bot commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GitLab Pipeline Action

General information

Job summaries

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taucher2003 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented May 4, 2026 •

edited

Loading