From 2b5884b2132c8c6b34de47ed9ab0ed60b3a946aa Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Wed, 13 Dec 2023 14:30:00 -0500 Subject: [PATCH] now support books shared with multiple teams --- app/controllers/api/v1/books_controller.rb | 7 +++-- .../api/v1/import/books_controller.rb | 2 +- app/controllers/books_controller.rb | 27 +++++++++++++++---- app/models/book.rb | 25 ++++++++++++++--- app/models/case.rb | 4 +-- app/models/team.rb | 10 ++++--- app/views/books/_form.html.erb | 13 ++++++--- app/views/books/edit.html.erb | 4 +-- app/views/books/index.html.erb | 4 +-- app/views/books/show.html.erb | 6 +++-- .../20231212112445_create_books_join_table.rb | 15 +++++++++++ .../20231213180159_add_owner_id_to_books.rb | 18 +++++++++++++ db/sample_data_seeds.rb | 15 ++++++++--- db/schema.rb | 9 +++++-- .../api/v1/books_controller_test.rb | 4 +-- .../api/v1/team_books_controller_test.rb | 2 +- test/controllers/books_controller_test.rb | 4 +-- test/fixtures/books.yml | 5 +--- test/fixtures/teams.yml | 3 +++ test/models/book_test.rb | 2 +- 20 files changed, 135 insertions(+), 44 deletions(-) create mode 100644 db/migrate/20231212112445_create_books_join_table.rb create mode 100644 db/migrate/20231213180159_add_owner_id_to_books.rb diff --git a/app/controllers/api/v1/books_controller.rb b/app/controllers/api/v1/books_controller.rb index 89a7fc77c..1cc52c5ab 100644 --- a/app/controllers/api/v1/books_controller.rb +++ b/app/controllers/api/v1/books_controller.rb @@ -60,10 +60,9 @@ def show def create @book = Book.new(book_params) - + team = Team.find_by(id: params[:book][:team_id]) + @book.teams << team if @book.save - # first = 1 == current_user.cases.count - # Analytics::Tracker.track_case_created_event current_user, @case, first respond_with @book else render json: @book.errors, status: :bad_request @@ -92,7 +91,7 @@ def destroy private def book_params - params.require(:book).permit(:team_id, :scorer_id, :selection_strategy_id, :name, :support_implicit_judgements, + params.require(:book).permit(:scorer_id, :selection_strategy_id, :name, :support_implicit_judgements, :show_rank) end diff --git a/app/controllers/api/v1/import/books_controller.rb b/app/controllers/api/v1/import/books_controller.rb index cfde5b1bf..9590ec45d 100644 --- a/app/controllers/api/v1/import/books_controller.rb +++ b/app/controllers/api/v1/import/books_controller.rb @@ -16,7 +16,7 @@ def create @book = Book.new - @book.team = Team.find(team_id) + @book.teams << Team.find(team_id) scorer_name = params_to_use[:scorer][:name] unless Scorer.exists?(name: scorer_name) diff --git a/app/controllers/books_controller.rb b/app/controllers/books_controller.rb index b4765bd79..8a143b667 100644 --- a/app/controllers/books_controller.rb +++ b/app/controllers/books_controller.rb @@ -16,7 +16,7 @@ class BooksController < ApplicationController respond_to :html def index - @books = current_user.books_involved_with.includes([ :team, :scorer, :selection_strategy ]) + @books = current_user.books_involved_with.includes([ :teams, :scorer, :selection_strategy ]) respond_with(@books) end @@ -68,7 +68,20 @@ def create end def update - @book.update(book_params) + # this logic is crazy, but basically we don't want to touch the teams that are associated with + # an book that the current_user CAN NOT see, so we clear out of the relationship all the ones + # they can see, and then repopulate it from the list of ids checked. Checkboxes suck. + team_ids_belonging_to_user = current_user.teams.pluck(:id) + teams = @book.teams.reject { |t| team_ids_belonging_to_user.include?(t.id) } + @book.teams.clear + book_params[:team_ids].each do |team_id| + teams << Team.find(team_id) + end + + @book.teams.replace(teams) + + @book.update(book_params.except(:team_ids)) + respond_with(@book) end @@ -147,7 +160,8 @@ def combine # rubocop:disable Metrics/MethodLength def assign_anonymous - assignee = @book.team.members.find_by(id: params[:assignee_id]) + # assignee = @book.team.members.find_by(id: params[:assignee_id]) + assignee = User.find_by(id: params[:assignee_id]) @book.judgements.where(user: nil).find_each do |judgement| judgement.user = assignee # if we are mapping a user to a judgement, @@ -235,8 +249,11 @@ def find_user end def book_params - params.require(:book).permit(:team_id, :scorer_id, :selection_strategy_id, :name, :support_implicit_judgements, - :show_rank) + params_to_use = params.require(:book).permit(:scorer_id, :selection_strategy_id, :name, + :support_implicit_judgements, + :show_rank, team_ids: []) + params_to_use[:team_ids].compact_blank! + params_to_use end end diff --git a/app/models/book.rb b/app/models/book.rb index 1ecda7f2b..aede7f030 100644 --- a/app/models/book.rb +++ b/app/models/book.rb @@ -10,9 +10,9 @@ # support_implicit_judgements :boolean # created_at :datetime not null # updated_at :datetime not null +# owner_id :integer # scorer_id :integer # selection_strategy_id :bigint not null -# team_id :integer # # Indexes # @@ -24,7 +24,12 @@ # class Book < ApplicationRecord # Associations - belongs_to :team + # belongs_to :team + # rubocop:disable Rails/HasAndBelongsToMany + has_and_belongs_to_many :teams, + join_table: 'teams_books' + # rubocop:enable Rails/HasAndBelongsToMany + belongs_to :selection_strategy belongs_to :scorer has_many :query_doc_pairs, dependent: :destroy, autosave: true @@ -40,9 +45,10 @@ class Book < ApplicationRecord inverse_of: :book # Scopes - scope :for_user, ->(user) { + scope :for_user_via_teams, ->(user) { joins(' - LEFT OUTER JOIN `teams` ON `teams`.`id` = `books`.`team_id` + LEFT OUTER JOIN `teams_books` ON `teams_books`.`book_id` = `books`.`id` + LEFT OUTER JOIN `teams` ON `teams`.`id` = `teams_books`.`team_id` LEFT OUTER JOIN `teams_members` ON `teams_members`.`team_id` = `teams`.`id` LEFT OUTER JOIN `users` ON `users`.`id` = `teams_members`.`member_id` ').where(' @@ -50,4 +56,15 @@ class Book < ApplicationRecord ', user.id) .order(name: :desc) } + + scope :for_user_directly_owned, ->(user) { + where(' + `books`.`owner_id` = ? + ', user.id) + } + + scope :for_user, ->(user) { + ids = for_user_via_teams(user).pluck(:id) + for_user_directly_owned(user).pluck(:id) + where(id: ids.uniq) + } end diff --git a/app/models/case.rb b/app/models/case.rb index 5fa778627..fe33d9a4d 100644 --- a/app/models/case.rb +++ b/app/models/case.rb @@ -100,13 +100,13 @@ class Case < ApplicationRecord ', user.id) } - scope :public_cases, -> { where(public: true) } - scope :for_user, ->(user) { ids = for_user_via_teams(user).pluck(:id) + for_user_directly_owned(user).pluck(:id) where(id: ids.uniq) } + scope :public_cases, -> { where(public: true) } + # return cases sorted by recently either updated or viewed by the user scope :recent, -> { left_outer_joins(:metadata) diff --git a/app/models/team.rb b/app/models/team.rb index 1c2effc24..65d2d5215 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -38,11 +38,15 @@ class Team < ApplicationRecord has_and_belongs_to_many :search_endpoints, join_table: 'teams_search_endpoints' + + has_and_belongs_to_many :books, + join_table: 'teams_books' + # rubocop:enable Rails/HasAndBelongsToMany - has_many :books, -> { order(name: :asc) }, - dependent: :destroy, - inverse_of: :team + # has_many :books, -> { order(name: :asc) }, + # dependent: :destroy, + # inverse_of: :team # has_many :search_endpoints, -> { order(name: :asc) }, # dependent: :destroy, diff --git a/app/views/books/_form.html.erb b/app/views/books/_form.html.erb index 5852a3623..3f8f4743c 100644 --- a/app/views/books/_form.html.erb +++ b/app/views/books/_form.html.erb @@ -18,10 +18,15 @@
The book's name often reflects the goal for the judgements, like Baseline Measurement or a specific problem.
-
- <%= form.label :team_id, class: 'form-label' %> - <%= form.collection_select(:team_id, current_user.teams, :id, :name, { prompt: true }, required: true, class: 'form-control') %> -
Books are meant to be shared with human raters and relevance engineers, access to the book is controlled by what team you pick.
+
+ <%= form.label :teams, "Teams to Share this Book With", class: 'form-label' %> + <%= form.collection_check_boxes(:team_ids, @current_user.teams, :id, :name) do |b| %> +
+ <%= b.check_box %> + <%= b.label %> +
+ <% end %> +
Books are meant to be shared with human raters and relevance engineers, and you do this by picking which teams can use them.
diff --git a/app/views/books/edit.html.erb b/app/views/books/edit.html.erb index 1a4dac159..1b0f3f326 100644 --- a/app/views/books/edit.html.erb +++ b/app/views/books/edit.html.erb @@ -71,7 +71,7 @@ <%= form_for(@book, url: assign_anonymous_book_path(@book)) do |form| %>
<%= form.label :assignee_id, class: 'form-label' %> - <%= select_tag "assignee_id", options_from_collection_for_select(@book.team.members, :id, :name, params.dig(:post, :category_id)), required: true, prompt: "Please select assignee", class: 'form-control' %> + <%= select_tag "assignee_id", options_from_collection_for_select(@book.teams.flat_map(&:members).uniq, :id, :name, params.dig(:post, :category_id)), required: true, prompt: "Please select assignee", class: 'form-control' %>

@@ -89,7 +89,7 @@ <%= form_for(@book, url: delete_ratings_by_assignee_book_path(@book), method: :delete, data: { confirm: "Are you sure?" }) do |form| %>
<%= form.label :user_id, class: 'form-label' %> - <%= select_tag "user_id", options_from_collection_for_select(@book.team.members, :id, :name, params.dig(:post, :category_id)), required: true, prompt: "Please select judge", class: 'form-control' %> + <%= select_tag "user_id", options_from_collection_for_select(@book.teams.flat_map(&:members).uniq, :id, :name, params.dig(:post, :category_id)), required: true, prompt: "Please select judge", class: 'form-control' %>

diff --git a/app/views/books/index.html.erb b/app/views/books/index.html.erb index 159fd302b..915ef2196 100644 --- a/app/views/books/index.html.erb +++ b/app/views/books/index.html.erb @@ -26,7 +26,7 @@ Name - Team + Teams Scorer Selection strategy @@ -37,7 +37,7 @@ <% @books.each do |book| %> <%= link_to book.name, book_path(book) %> - <%= book.team.name %> + <%= book.teams.pluck(:name).join(',') %> <%= book.scorer.name %> <%= book.selection_strategy.name %> <%= book.rated_query_doc_pairs.count %> rated query/doc pairs diff --git a/app/views/books/show.html.erb b/app/views/books/show.html.erb index 82e40d6ca..0c0742579 100644 --- a/app/views/books/show.html.erb +++ b/app/views/books/show.html.erb @@ -28,8 +28,10 @@ Books organize all the query/doc pairs that are needed for evaluating your searc <% end %>

- Team: - <%= link_to @book.team.name, teams_core_path(@book.team) %> + Teams: + <% @book.teams.each do |team| %> + <%= link_to team.name, teams_core_path(team) %> + <% end %>

diff --git a/db/migrate/20231212112445_create_books_join_table.rb b/db/migrate/20231212112445_create_books_join_table.rb new file mode 100644 index 000000000..506435b8e --- /dev/null +++ b/db/migrate/20231212112445_create_books_join_table.rb @@ -0,0 +1,15 @@ +class CreateBooksJoinTable < ActiveRecord::Migration[7.1] + def change + create_join_table :books, :teams, table_name: :teams_books do |t| + end + + CreateBooksJoinTable.connection.execute( + " + insert into teams_books (team_id, book_id) select team_id, id from books + " + ) + + remove_column :books, :team_id + + end +end diff --git a/db/migrate/20231213180159_add_owner_id_to_books.rb b/db/migrate/20231213180159_add_owner_id_to_books.rb new file mode 100644 index 000000000..24781c33c --- /dev/null +++ b/db/migrate/20231213180159_add_owner_id_to_books.rb @@ -0,0 +1,18 @@ +class AddOwnerIdToBooks < ActiveRecord::Migration[7.1] + def change + add_column :books, :owner_id, :integer + + AddOwnerIdToBooks.connection.execute( + " + UPDATE books + SET owner_id = ( + SELECT tm.member_id + FROM teams_books tb, teams_members tm + WHERE tb.book_id = books.id + AND tb.team_id = tm.team_id + LIMIT 1 + ); + " + ) + end +end diff --git a/db/sample_data_seeds.rb b/db/sample_data_seeds.rb index 9ecfd983e..541a454d0 100755 --- a/db/sample_data_seeds.rb +++ b/db/sample_data_seeds.rb @@ -24,7 +24,6 @@ def print_user_info info tmdb_es_endpoint = SearchEndpoint.find_or_create_by search_engine: :es, endpoint_url: "http://quepid-elasticsearch.dev.o19s.com:9206/tmdb/_search", api_method: 'POST' - print_step "End of seeding search endpoints................" # Users @@ -127,6 +126,15 @@ def print_user_info info realistic_activity_user = seed_user user_params print_user_info user_params +# go ahead and assign the end point to this person. +statedecoded_solr_endpoint.owner = realistic_activity_user +tmdb_solr_endpoint.owner = realistic_activity_user +tmdb_es_endpoint.owner = realistic_activity_user + +statedecoded_solr_endpoint.save +tmdb_solr_endpoint.save +tmdb_es_endpoint.save + ###################################### # OSC Team Owner ###################################### @@ -289,7 +297,6 @@ def print_case_info the_case new_try = tens_of_queries_case.tries.build try_params - try_number = tens_of_queries_case.last_try_number + 1 new_try.try_number = try_number @@ -340,7 +347,9 @@ def print_case_info the_case # Books print_step "Seeding books................" -book = Book.where(name: "Book of Ratings", team:osc, scorer: Scorer.system_default_scorer, selection_strategy: SelectionStrategy.find_by(name:'Multiple Raters')).first_or_create +book = Book.where(name: "Book of Ratings", scorer: Scorer.system_default_scorer, selection_strategy: SelectionStrategy.find_by(name:'Multiple Raters')).first_or_create +book.teams << osc +boo.save # this code copied from populate_controller.rb and should be in a service... # has a hacked in judgement creator... diff --git a/db/schema.rb b/db/schema.rb index ee2688e5d..b4d564be0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2023_12_06_202557) do +ActiveRecord::Schema[7.1].define(version: 2023_12_13_180159) do create_table "annotations", id: :integer, charset: "utf8mb3", force: :cascade do |t| t.text "message" t.string "source" @@ -29,7 +29,6 @@ end create_table "books", charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t| - t.integer "team_id" t.integer "scorer_id" t.bigint "selection_strategy_id", null: false t.string "name" @@ -37,6 +36,7 @@ t.datetime "updated_at", null: false t.boolean "support_implicit_judgements" t.boolean "show_rank", default: false + t.integer "owner_id" t.index ["selection_strategy_id"], name: "index_books_on_selection_strategy_id" end @@ -221,6 +221,11 @@ t.index ["owner_id"], name: "owner_id" end + create_table "teams_books", id: false, charset: "utf8mb4", collation: "utf8mb4_bin", force: :cascade do |t| + t.bigint "book_id", null: false + t.bigint "team_id", null: false + end + create_table "teams_cases", primary_key: ["case_id", "team_id"], charset: "latin1", force: :cascade do |t| t.integer "case_id", null: false t.integer "team_id", null: false diff --git a/test/controllers/api/v1/books_controller_test.rb b/test/controllers/api/v1/books_controller_test.rb index 98afe8795..4de7613d6 100644 --- a/test/controllers/api/v1/books_controller_test.rb +++ b/test/controllers/api/v1/books_controller_test.rb @@ -39,7 +39,7 @@ class BooksControllerTest < ActionController::TestCase end describe 'Updating book' do - let(:the_book) { books(:book_of_star_wars_judgements) } + let(:the_book) { books(:james_bond_movies) } describe 'when book does not exist' do test 'returns not found error' do @@ -61,7 +61,7 @@ class BooksControllerTest < ActionController::TestCase describe 'Deleting a book' do describe 'when it is the last/only case' do let(:doug) { users(:doug) } - let(:the_book) { books(:book_of_star_wars_judgements) } + let(:the_book) { books(:james_bond_movies) } before do login_user doug diff --git a/test/controllers/api/v1/team_books_controller_test.rb b/test/controllers/api/v1/team_books_controller_test.rb index b059189b0..3c140bafe 100644 --- a/test/controllers/api/v1/team_books_controller_test.rb +++ b/test/controllers/api/v1/team_books_controller_test.rb @@ -30,7 +30,7 @@ class TeamBooksControllerTest < ActionController::TestCase ids = books.map { |book| book['id'] } assert_includes ids, book1.id - # assert_includes ids, book2.id + assert_includes ids, book2.id end end end diff --git a/test/controllers/books_controller_test.rb b/test/controllers/books_controller_test.rb index f4c207dc3..761b9139d 100644 --- a/test/controllers/books_controller_test.rb +++ b/test/controllers/books_controller_test.rb @@ -71,7 +71,7 @@ def test_more def test_differing_scales_blows_up login_user - book_to_merge = Book.new(name: 'Book with a 1,2,3,4 scorer', team: book.team, scorer: communal_scorer, + book_to_merge = Book.new(name: 'Book with a 1,2,3,4 scorer', teams: book.teams, scorer: communal_scorer, selection_strategy: SelectionStrategy.find_by(name: 'Multiple Raters')) book_to_merge.save! @@ -88,7 +88,7 @@ def test_combining_single_rater_strategy_into_multiple_rater_strategy_book_works login_user book_with_multiple_raters = Book.create(name: 'Book with a 1,2,3,4 scorer', - team: single_rater_book.team, + teams: single_rater_book.teams, scorer: single_rater_book.scorer, selection_strategy: SelectionStrategy.find_by(name: 'Multiple Raters')) diff --git a/test/fixtures/books.yml b/test/fixtures/books.yml index 660f9dbae..5ace6d22a 100644 --- a/test/fixtures/books.yml +++ b/test/fixtures/books.yml @@ -8,9 +8,9 @@ # support_implicit_judgements :boolean # created_at :datetime not null # updated_at :datetime not null +# owner_id :integer # scorer_id :integer # selection_strategy_id :bigint not null -# team_id :integer # # Indexes # @@ -22,19 +22,16 @@ # book_of_star_wars_judgements: - team: :shared scorer: :quepid_default_scorer selection_strategy: :single_rater name: TMDB Star Wars Judgements book_of_comedy_films: - team: :another_shared_team scorer: :quepid_default_scorer selection_strategy: :single_rater name: TMDB Greatest Comedies james_bond_movies: # this has some query doc pairs and ratings. - team: :shared scorer: :quepid_default_scorer selection_strategy: :multiple_raters name: James Bond Movies diff --git a/test/fixtures/teams.yml b/test/fixtures/teams.yml index 33b1668ef..2af048162 100644 --- a/test/fixtures/teams.yml +++ b/test/fixtures/teams.yml @@ -31,6 +31,7 @@ shared: scorers: shared_scorer, shared_scorer1, shared_scorer2 cases: shared_with_team search_endpoints: one, for_shared_team_case + books: james_bond_movies, book_of_star_wars_judgements case_finder_owned_team: name: An team owned by the Case Finder User @@ -56,6 +57,7 @@ shared_team: cases: shared_team_case search_endpoints: for_shared_team_case scorers: random_scorer + books: book_of_star_wars_judgements team_owner_team: name: Team owned by Team Owner User @@ -80,6 +82,7 @@ another_shared_team: owner: :random_2 members: random_1, random, random_2 cases: shared_case + books: book_of_comedy_films team_for_case_shared_with_owner: name: Team for case shared with owner diff --git a/test/models/book_test.rb b/test/models/book_test.rb index 34ed233af..40fb83075 100644 --- a/test/models/book_test.rb +++ b/test/models/book_test.rb @@ -10,9 +10,9 @@ # support_implicit_judgements :boolean # created_at :datetime not null # updated_at :datetime not null +# owner_id :integer # scorer_id :integer # selection_strategy_id :bigint not null -# team_id :integer # # Indexes #