-
Notifications
You must be signed in to change notification settings - Fork 0
organisation user pins #953
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
base: main
Are you sure you want to change the base?
Changes from all commits
1b8bfe6
65a8ca0
27b575c
169e976
cc62ab6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Mutations | ||
| module Users | ||
| class UpdateOrganizationPins < BaseMutation | ||
| description 'Update pinned organizations for a user.' | ||
|
|
||
| field :user, Types::UserType, null: true, description: 'The updated user.' | ||
|
|
||
| argument :organization_ids, | ||
| [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.' | ||
|
|
||
| def resolve(user_id:, organization_ids:) | ||
| user = SagittariusSchema.object_from_id(user_id) | ||
| return { user: nil, errors: [create_error(:user_not_found, 'Invalid user with provided id')] } if user.nil? | ||
|
|
||
| organizations = organization_ids.map { |id| SagittariusSchema.object_from_id(id) } | ||
| if organizations.any?(&:nil?) | ||
| return { user: nil, errors: [create_error(:organization_not_found, 'Invalid organization with provided id')] } | ||
| end | ||
|
|
||
| ::Users::UpdateOrganizationPinsService.new( | ||
| current_authentication, | ||
| user, | ||
| organizations.map(&:id) | ||
| ).execute.to_mutation_response(success_key: :user) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,16 @@ | ||||||
| # frozen_string_literal: true | ||||||
|
|
||||||
| module Types | ||||||
| class UserOrganizationPinType < Types::BaseObject | ||||||
| description 'Represents a pinned organization of a user' | ||||||
|
|
||||||
| authorize :read_user_organization_pin | ||||||
|
|
||||||
| field :organization, Types::OrganizationType, null: false, description: 'The pinned organization' | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :priority, Integer, null: false, description: 'Ordering priority of the pin' | ||||||
| field :user, Types::UserType, null: false, description: 'The user owning this pin' | ||||||
|
|
||||||
| id_field UserOrganizationPin | ||||||
| timestamps | ||||||
| end | ||||||
| end | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -41,6 +41,14 @@ class UserType < Types::BaseObject | |||||
| description: 'Identities of this user', | ||||||
| method: :user_identities | ||||||
|
|
||||||
| field :organization_pins, [Types::UserOrganizationPinType], | ||||||
| null: false, | ||||||
| description: 'Pinned organizations of this user with explicit priority' | ||||||
|
|
||||||
| field :pinned_organizations, [Types::OrganizationType], | ||||||
| null: false, | ||||||
| description: 'Pinned organizations of this user ordered by pin priority' | ||||||
|
|
||||||
| field :mfa_status, Types::MfaStatusType, | ||||||
| null: true, | ||||||
| description: 'Multi-factor authentication status of this user' | ||||||
|
|
@@ -71,5 +79,13 @@ def mfa_status | |||||
| backup_codes_count: object.backup_codes.size, | ||||||
| } | ||||||
| end | ||||||
|
|
||||||
| def organization_pins | ||||||
| object.user_organization_pins.order(priority: :asc) | ||||||
| end | ||||||
|
|
||||||
| def pinned_organizations | ||||||
| organization_pins.map(&:organization) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would load all pins of the user and map them in memory with N+1 queries.
Suggested change
We would loose the sorting though. I think we can probably remove the whole |
||||||
| end | ||||||
| end | ||||||
| end | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,9 @@ | |||||
| class Organization < ApplicationRecord | ||||||
| include NamespaceParent | ||||||
|
|
||||||
| has_many :user_organization_pins, inverse_of: :organization, dependent: :delete_all | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use
Suggested change
|
||||||
| has_many :pinned_by_users, through: :user_organization_pins, source: :user | ||||||
|
|
||||||
| validates :name, presence: true, | ||||||
| length: { minimum: 3, maximum: 50 }, | ||||||
| allow_blank: false, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -27,6 +27,8 @@ class User < ApplicationRecord | |||||
| 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 | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use
Suggested change
|
||||||
| has_many :pinned_organizations, through: :user_organization_pins, source: :organization | ||||||
|
|
||||||
| has_one_attached :avatar | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,10 @@ | ||||||||||||||||
| # frozen_string_literal: true | ||||||||||||||||
|
|
||||||||||||||||
| class UserOrganizationPin < ApplicationRecord | ||||||||||||||||
| belongs_to :user, inverse_of: :user_organization_pins | ||||||||||||||||
| belongs_to :organization, inverse_of: :user_organization_pins | ||||||||||||||||
|
|
||||||||||||||||
| 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 } | ||||||||||||||||
|
Comment on lines
+7
to
+9
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Works, but we can use a single validates for priority
Suggested change
|
||||||||||||||||
| end | ||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,7 @@ | ||||||||||
| # frozen_string_literal: true | ||||||||||
|
|
||||||||||
| class UserOrganizationPinPolicy < BasePolicy | ||||||||||
| delegate { subject.user } | ||||||||||
|
|
||||||||||
| rule { can?(:read_user) }.enable :read_user_organization_pin | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either we don't need this here or we don't need the Also, I don't think someone should be able to read the pins of another user.
Suggested change
|
||||||||||
| end | ||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,10 @@ class UserPolicy < BasePolicy | |||||||||||
| condition(:user_is_admin) { user&.admin? || false } | ||||||||||||
| condition(:admin_status_visible) { ApplicationSetting.current[:admin_status_visible] } | ||||||||||||
|
|
||||||||||||
| rule { ~anonymous }.enable :read_user | ||||||||||||
| rule { ~anonymous }.policy do | ||||||||||||
| enable :read_user | ||||||||||||
| enable :read_user_organization_pin | ||||||||||||
| end | ||||||||||||
|
Comment on lines
+8
to
+11
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
|
|
||||||||||||
| rule { user_is_admin }.policy do | ||||||||||||
| enable :update_user | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,59 @@ | ||||||
| # frozen_string_literal: true | ||||||
|
|
||||||
| module Users | ||||||
| class UpdateOrganizationPinsService | ||||||
| include Sagittarius::Database::Transactional | ||||||
|
|
||||||
| attr_reader :current_authentication, :user, :organization_ids | ||||||
|
|
||||||
| def initialize(current_authentication, user, organization_ids) | ||||||
| @current_authentication = current_authentication | ||||||
| @user = user | ||||||
| @organization_ids = organization_ids.uniq | ||||||
| end | ||||||
|
|
||||||
| def execute | ||||||
| unless Ability.allowed?(current_authentication, :update_user, user) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using |
||||||
| return ServiceResponse.error(message: 'Missing permission', error_code: :missing_permission) | ||||||
| end | ||||||
|
|
||||||
| organizations = Organization.where(id: organization_ids) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A user should only be able to pin organizations that it is a member of
Suggested change
|
||||||
| if organizations.count != organization_ids.count | ||||||
| return ServiceResponse.error(message: 'Organization not found', error_code: :organization_not_found) | ||||||
| end | ||||||
|
|
||||||
| transactional do |t| | ||||||
| old_pins = user.user_organization_pins.map do |pin| | ||||||
| { organization_id: pin.organization_id, priority: pin.priority } | ||||||
| end | ||||||
|
|
||||||
| user.user_organization_pins.delete_all | ||||||
|
|
||||||
| organization_ids.each_with_index do |organization_id, priority| | ||||||
| pin = user.user_organization_pins.build(organization_id: organization_id, priority: priority) | ||||||
| next if pin.save | ||||||
|
|
||||||
| t.rollback_and_return! ServiceResponse.error( | ||||||
| message: 'Failed to update user organization pins', | ||||||
| error_code: :invalid_user, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably also be its own error code |
||||||
| details: pin.errors | ||||||
| ) | ||||||
| end | ||||||
|
|
||||||
| new_pins = user.user_organization_pins.reload.map do |pin| | ||||||
| { organization_id: pin.organization_id, priority: pin.priority } | ||||||
| end | ||||||
|
|
||||||
| AuditService.audit( | ||||||
| :user_updated, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be its own event |
||||||
| author_id: current_authentication.user.id, | ||||||
| entity: user, | ||||||
| target: user, | ||||||
| details: { old_pins: old_pins, new_pins: new_pins } | ||||||
| ) | ||||||
|
|
||||||
| ServiceResponse.success(message: 'Updated user organization pins', payload: user) | ||||||
| end | ||||||
| end | ||||||
| end | ||||||
| end | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class CreateUserOrganizationPins < Code0::ZeroTrack::Database::Migration[1.0] | ||
| def change | ||
| create_table :user_organization_pins do |t| | ||
| t.references :user, null: false, foreign_key: { on_delete: :cascade }, index: false | ||
| t.references :organization, null: false, foreign_key: { on_delete: :cascade }, index: false | ||
| t.integer :priority, null: false | ||
|
|
||
| t.index %i[user_id organization_id], unique: true | ||
| t.index %i[user_id priority], unique: true | ||
|
|
||
| t.timestamps_with_timezone | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| 7cf08006d8ab53c1cea495d14f91286debd633d327ea0dee7148ce805d61c32d |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| --- | ||
| title: usersUpdateOrganizationPins | ||
| --- | ||
|
|
||
| Update pinned organizations for a user. | ||
|
|
||
| ## Arguments | ||
|
|
||
| | Name | Type | Description | | ||
| |------|------|-------------| | ||
| | `clientMutationId` | [`String`](../scalar/string.md) | A unique identifier for the client performing the mutation. | | ||
| | `organizationIds` | [`[OrganizationID!]!`](../scalar/organizationid.md) | Ordered list of organization IDs to pin for the user. | | ||
| | `userId` | [`UserID!`](../scalar/userid.md) | ID of the user to update. | | ||
|
|
||
| ## Fields | ||
|
|
||
| | Name | Type | Description | | ||
| |------|------|-------------| | ||
| | `clientMutationId` | [`String`](../scalar/string.md) | A unique identifier for the client performing the mutation. | | ||
| | `errors` | [`[Error!]!`](../object/error.md) | Errors encountered during execution of the mutation. | | ||
| | `user` | [`User`](../object/user.md) | The updated user. | |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| --- | ||
| title: UserOrganizationPin | ||
| --- | ||
|
|
||
| Represents a pinned organization of a user | ||
|
|
||
| ## Fields without arguments | ||
|
|
||
| | Name | Type | Description | | ||
| |------|------|-------------| | ||
| | `createdAt` | [`Time!`](../scalar/time.md) | Time when this UserOrganizationPin was created | | ||
| | `id` | [`UserOrganizationPinID!`](../scalar/userorganizationpinid.md) | Global ID of this UserOrganizationPin | | ||
| | `organization` | [`Organization!`](../object/organization.md) | The pinned organization | | ||
| | `priority` | [`Int!`](../scalar/int.md) | Ordering priority of the pin | | ||
| | `updatedAt` | [`Time!`](../scalar/time.md) | Time when this UserOrganizationPin was last updated | | ||
| | `user` | [`User!`](../object/user.md) | The user owning this pin | |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| title: UserOrganizationPinID | ||
| --- | ||
|
|
||
| A unique identifier for all UserOrganizationPin entities of the application |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| FactoryBot.define do | ||
| sequence(:user_organization_pin_priority) | ||
|
|
||
| factory :user_organization_pin do | ||
| user | ||
| organization | ||
| priority { generate(:user_organization_pin_priority) } | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe Mutations::Users::UpdateOrganizationPins do | ||
| it { expect(described_class.graphql_name).to eq('UsersUpdateOrganizationPins') } | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?