Merge pull request #2430 from coreinfrastructure/more_docs

Improve model docs, add utility method

Author: david-a-wheeler

app/models/additional_right.rb

@@ -1,14 +1,52 @@
 # frozen_string_literal: true
 
-# Model class for additional right data management.
-#
+# Copyright 2015-2017, the Linux Foundation, IDA, and the
+# OpenSSF Best Practices badge contributors
+# SPDX-License-Identifier: MIT
 
+# Model class for additional rights data management.
+#
+# This model represents the many-to-many relationship between users and
+# projects for granting additional editing permissions beyond project
+# ownership. It serves as a join table that allows multiple users to have
+# edit rights on a single project, and allows users to have edit rights
+# on multiple projects they don't own.
+#
+# Currently mere *presence* is what matters; it gives the user edit rights
+# to that project. In the future this *could* have 1+ additional fields
+# identifying the specific additional rights of a user over a project.
+#
+# Database schema:
+# - project_id: Foreign key to projects table
+# - user_id: Foreign key to users table
+# - created_at: When the right was granted
+# - updated_at: When the record was last modified
+#
+# Indexes:
+# - Unique index on [user_id, project_id] prevents duplicate rights
+# - Individual indexes on project_id and user_id for query performance
 class AdditionalRight < ApplicationRecord
-  # List additional rights of users for a given project.
-  # This is a simple associative table (between project and user).
-  # Currently mere *presence* is what matters; it gives the user edit rights
-  # to that project.  In the future this *could* have 1+ additional fields
-  # identifying the specific additional rights of a user over a project.
+  # Associates this additional right with a specific project.
+  # When the project is destroyed, all associated additional rights are
+  # deleted.
+  # @return [Project] the project this right applies to
   belongs_to :project
+
+  # Associates this additional right with a specific user.
+  # When the user is destroyed, all their additional rights are deleted.
+  # @return [User] the user who has this additional right
   belongs_to :user
+
+  # Validates that both project and user exist and are valid.
+  # NOTE: These explicit validations are required because this application
+  # sets belongs_to_required_by_default = false in its configuration.
+  validates :project, presence: true
+  validates :user, presence: true
+
+  # Finds all additional rights for a specific project.
+  # This scope can be used instead of AdditionalRight.where(project_id: id)
+  # for better readability and consistency.
+  # @param project_id [Integer] the ID of the project
+  # @return [ActiveRecord::Relation<AdditionalRight>] rights for project
+  scope :for_project, ->(project_id) { where(project_id: project_id) }
 end

app/models/application_record.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-# Copyright 2015-2017, the Linux Foundation, IDA, and the
+# Copyright the Linux Foundation, IDA, and the
 # OpenSSF Best Practices badge contributors
 # SPDX-License-Identifier: MIT
 

app/models/project.rb

@@ -231,7 +231,7 @@ class Project < ApplicationRecord
   # @return [String] sorted array of user IDs as string
   def additional_rights_to_s
     # "distinct" shouldn't be needed; it's purely defensive here
-    list = AdditionalRight.where(project_id: id).distinct.pluck(:user_id)
+    list = AdditionalRight.for_project(id).distinct.pluck(:user_id)
     list.sort.to_s # Use list.sort.to_s[1..-2] to remove surrounding [ and ]
   end
 

test/controllers/projects_controller_test.rb

@@ -352,7 +352,7 @@ def only_correct_criteria_selectable(level)
       user_id: users(:test_user_mark).id,
       project_id: @project.id
     ).save!
-    assert_equal 2, AdditionalRight.where(project_id: @project.id).count
+    assert_equal 2, AdditionalRight.for_project(@project.id).count
     log_in_as(@project.user)
     # Run patch (the point of the test), which invokes the 'update' method
     patch "/en/projects/#{@project.id}", params: {
@@ -362,7 +362,7 @@ def only_correct_criteria_selectable(level)
     }
     # TODO: Weird http/https discrepancy in test
     # assert_redirected_to project_path(@project, locale: :en)
-    assert_equal 0, AdditionalRight.where(project_id: @project.id).count
+    assert_equal 0, AdditionalRight.for_project(@project.id).count
   end
 
   # Negative test
@@ -375,7 +375,7 @@ def only_correct_criteria_selectable(level)
       user_id: users(:test_user_mark).id,
       project_id: @project.id
     ).save!
-    assert_equal 2, AdditionalRight.where(project_id: @project.id).count
+    assert_equal 2, AdditionalRight.for_project(@project.id).count
     log_in_as(users(:test_user_melissa))
     # Run patch (the point of the test), which invokes the 'update' method
     patch "/en/projects/#{@project.id}", params: {
@@ -386,7 +386,7 @@ def only_correct_criteria_selectable(level)
     # tries to remove an "additional rights" user, we just ignore it.
     # If we add an error report, we should check for that error report here.
     assert_redirected_to root_path(locale: 'en')
-    assert_equal 2, AdditionalRight.where(project_id: @project.id).count
+    assert_equal 2, AdditionalRight.for_project(@project.id).count
   end
 
   # Negative test