From c87974b7ac7be69b67aa74f94401e9896b309b8e Mon Sep 17 00:00:00 2001 From: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com> Date: Fri, 2 Oct 2020 09:06:11 -0600 Subject: [PATCH] [deploy] Add co_author_ids to Articles (#10339) * Replaces second_user_id and third_user_id with co_author_ids on the Articles table * Adjusts migration that adds co_author_ids to also add array: true * Replaces second_user_id and third_user_id with co_author_ids in all applicable files - Use co_author_ids in _individual_article view - Use co_author_ids in articles and stories controllers - Use co_author_ids in article and podcast_episode models - Use co_author_ids in article_dashboard - Use co_author_ids in comments_helper - Use co_author_ids in articles_spec * Replaces second_user_id and third_user_id with co_author_ids in article.rb model * Remove unused #assign_co_authors method from the Stories::Controller * Add #co_authors and #co_author_name_and_path to article_decorator.rb * Adds a descriptive form field for co_author_ids for Admins in /admin/articles * Adds a conditional in article/show.html.erb to render co_author names on an Article in a human-readable way * Replaces get_co_author_name_and_path with proper method name, co_author_name_and_path in show.html.erb * Adds latest schema to (hopefully) resolve Travis conflicts * Adds the safe operator to show.html.erb * Adjusts articles_spec.rb to use an array in the first test * Replaces unless &.empty? with if ./present? in articles/show.html.erb * Replaces second_user_id with co_author_ids in stories_show_spec.rb test * Replaces elsif with else in articles/show.html.erb * Cleans up show.html.erb by removing conditional in favor of decorator method * Refactors splitting of params in Articles::Controller and optimizes query in #co_authors * Reverts removal of second_user_id and third_user_id and migration file * Adds a data_update script to update co_author_ids with existing second and third user_ids * Adds validations to co_authors and flash_messages to indicate whether an update was successful or not - Adds to methods to article.rb to validate the IDs of co_authors and authors - Adds flash messages to the Admin::Articles::Controller and a redirect to the show page - Removes the JS highlighting upon submit when updating an Article in Admin - Refactors #update action in Admin::Articles::Controller * Adds tests to article_spec.rb around co_author_id validations in article.rb * Adjusts #validate_co_authors_must_not_be_the_same to use .include? instead * Uses Field::Select.with_options in article_dashboard.rb to properly display co_author_ids * Reverts removal of assigning co_author_ids in the Stories::Controller * Adjusts error message and adjusts return logic in article.rb (thanks, Fernando!) * Fixes failing article_spec.rb test that checks for co_author_ids uniqueness * Adds a default array to co_author_ids and checks if they are .blank? * Refactor data_update script to use a single SQL statement (thank you, Michael) * Preserve array order of co_author_ids in article_decorator.rb * Add db file for default: [] * Add validation to fix bug related to text inputs and invalid users when adding co_authors * Adds tests to ensure that co_author_ids are both an integer and an integer > 0 * Updates admin/articles_spec.rb to default [] instead of nil * Adjusts validations in article.rb to be DRY-er and more consistent * Consolidates validations further * Refactors validations in article.rb to use procs * Refactors data_update script to remove nil values from array --- app/controllers/admin/articles_controller.rb | 14 ++++---- app/controllers/stories_controller.rb | 9 +++-- app/dashboards/article_dashboard.rb | 5 +-- app/decorators/article_decorator.rb | 10 ++++++ app/helpers/comments_helper.rb | 3 +- app/models/article.rb | 23 ++++++++++++- app/models/podcast_episode.rb | 3 +- .../articles/_individual_article.html.erb | 13 +++----- app/views/articles/show.html.erb | 7 ++-- ...910155145_add_co_author_ids_to_articles.rb | 5 +++ db/schema.rb | 1 + ...343_backfill_co_author_ids_for_articles.rb | 11 +++++++ spec/models/article_spec.rb | 33 +++++++++++++++++++ spec/requests/admin/articles_spec.rb | 15 +++------ spec/requests/stories_show_spec.rb | 2 +- 15 files changed, 109 insertions(+), 45 deletions(-) create mode 100644 db/migrate/20200910155145_add_co_author_ids_to_articles.rb create mode 100644 lib/data_update_scripts/20200916202343_backfill_co_author_ids_for_articles.rb diff --git a/app/controllers/admin/articles_controller.rb b/app/controllers/admin/articles_controller.rb index ce3af1c33..5bcd65fef 100644 --- a/app/controllers/admin/articles_controller.rb +++ b/app/controllers/admin/articles_controller.rb @@ -45,10 +45,13 @@ module Admin article.boosted_additional_articles = article_params[:boosted_additional_articles].to_s == "true" article.boosted_dev_digest_email = article_params[:boosted_dev_digest_email].to_s == "true" article.user_id = article_params[:user_id].to_i - article.second_user_id = article_params[:second_user_id].to_i - article.third_user_id = article_params[:third_user_id].to_i - article.update!(article_params) - render body: nil + article.co_author_ids = article_params[:co_author_ids].split(",").map(&:strip) + if article.save + flash[:success] = "Article saved!" + else + flash[:danger] = article.errors_as_sentence + end + redirect_to admin_article_path(article.id) end private @@ -131,8 +134,7 @@ module Admin main_image_background_hex_color featured_number user_id - second_user_id - third_user_id + co_author_ids last_buffered] params.require(:article).permit(allowed_params) end diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 80466bd7e..688ece2ab 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -294,8 +294,8 @@ class StoriesController < ApplicationController end @comments_to_show_count = @article.cached_tag_list_array.include?("discuss") ? 50 : 30 - assign_second_and_third_user set_article_json_ld + assign_co_authors @comment = Comment.new(body_markdown: @article&.comment_template) end @@ -303,11 +303,10 @@ class StoriesController < ApplicationController !@article.published && params[:preview] != @article.password end - def assign_second_and_third_user - return if @article.second_user_id.blank? + def assign_co_authors + return if @article.co_author_ids.blank? - @second_user = User.find(@article.second_user_id) - @third_user = User.find(@article.third_user_id) if @article.third_user_id.present? + @co_author_ids = User.find(@article.co_author_ids) end def assign_user_comments diff --git a/app/dashboards/article_dashboard.rb b/app/dashboards/article_dashboard.rb index 8856a69d3..6ca83bde9 100644 --- a/app/dashboards/article_dashboard.rb +++ b/app/dashboards/article_dashboard.rb @@ -10,8 +10,7 @@ class ArticleDashboard < Administrate::BaseDashboard ATTRIBUTE_TYPES = { user: Field::BelongsTo, user_id: UserIdField, - second_user_id: Field::Number, - third_user_id: Field::Number, + co_author_ids: Field::Select.with_options(collection: []), organization: Field::BelongsTo, id: Field::Number, title: Field::String, @@ -65,8 +64,6 @@ class ArticleDashboard < Administrate::BaseDashboard # on the model's form (`new` and `edit`) pages. FORM_ATTRIBUTES = %i[ user_id - second_user_id - third_user_id organization title search_optimized_title_preamble diff --git a/app/decorators/article_decorator.rb b/app/decorators/article_decorator.rb index f32e9bf98..ae79b8d38 100644 --- a/app/decorators/article_decorator.rb +++ b/app/decorators/article_decorator.rb @@ -86,4 +86,14 @@ class ArticleDecorator < ApplicationDecorator def long_markdown? body_markdown.present? && body_markdown.size > LONG_MARKDOWN_THRESHOLD end + + def co_authors + User.select(:name, :username).where(id: co_author_ids).order(created_at: :asc) + end + + def co_author_name_and_path + co_authors.map do |user| + "#{user.name}" + end.to_sentence + end end diff --git a/app/helpers/comments_helper.rb b/app/helpers/comments_helper.rb index b5095de60..0b5420b98 100644 --- a/app/helpers/comments_helper.rb +++ b/app/helpers/comments_helper.rb @@ -15,8 +15,7 @@ module CommentsHelper commentable && [ commentable.user_id, - commentable.second_user_id, - commentable.third_user_id, + commentable.co_author_ids, ].any? { |id| id == comment.user_id } end diff --git a/app/models/article.rb b/app/models/article.rb index f310541b5..14b479c63 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -79,6 +79,9 @@ class Article < ApplicationRecord validate :validate_collection_permission validate :validate_tag validate :validate_video + validate :validate_co_authors, unless: -> { co_author_ids.blank? } + validate :validate_co_authors_must_not_be_the_same, unless: -> { co_author_ids.blank? } + validate :validate_co_authors_exist, unless: -> { co_author_ids.blank? } before_validation :evaluate_markdown, :create_slug before_save :update_cached_user @@ -150,7 +153,7 @@ class Article < ApplicationRecord :video_thumbnail_url, :video_closed_caption_track_url, :social_image, :published_from_feed, :crossposted_at, :published_at, :featured_number, :last_buffered, :facebook_last_buffered, :created_at, :body_markdown, - :email_digest_eligible, :processed_html, :second_user_id, :third_user_id) + :email_digest_eligible, :processed_html, :co_author_ids) } scope :boosted_via_additional_articles, lambda { @@ -558,6 +561,24 @@ class Article < ApplicationRecord errors.add(:collection_id, "must be one you have permission to post to") end + def validate_co_authors + return if co_author_ids.exclude?(user_id) + + errors.add(:co_author_ids, "must not be the same user as the author") + end + + def validate_co_authors_must_not_be_the_same + return if co_author_ids.uniq.count == co_author_ids.count + + errors.add(:base, "co-author IDs must be unique") + end + + def validate_co_authors_exist + return if User.where(id: co_author_ids).count == co_author_ids.count + + errors.add(:co_author_ids, "must be valid user IDs") + end + def past_or_present_date return unless published_at && published_at > Time.current diff --git a/app/models/podcast_episode.rb b/app/models/podcast_episode.rb index 5985e80c3..75a761082 100644 --- a/app/models/podcast_episode.rb +++ b/app/models/podcast_episode.rb @@ -91,8 +91,7 @@ class PodcastEpisode < ApplicationRecord nil end alias user_id nil_method - alias second_user_id nil_method - alias third_user_id nil_method + alias co_author_ids nil_method private diff --git a/app/views/admin/articles/_individual_article.html.erb b/app/views/admin/articles/_individual_article.html.erb index af451d56a..d89130f04 100644 --- a/app/views/admin/articles/_individual_article.html.erb +++ b/app/views/admin/articles/_individual_article.html.erb @@ -68,7 +68,7 @@ cover image
<% end %> - <%= form_with url: admin_article_path(article.id), html: { data: { action: "submit->article#highlightElement" } } do |f| %> + <%= form_with url: admin_article_path(article.id), local: true do |f| %>
@@ -86,14 +86,9 @@ value="<%= article.user_id %>">
- - -
-
- - + + ">
diff --git a/app/views/articles/show.html.erb b/app/views/articles/show.html.erb index 46ac009f7..e7df9fb82 100644 --- a/app/views/articles/show.html.erb +++ b/app/views/articles/show.html.erb @@ -155,11 +155,8 @@ <% if @article.published_timestamp.present? %> <%= local_date(@article.published_timestamp, show_year: @article.displayable_published_at.year != Time.current.year) %> <% end %> - <% if @second_user.present? %> - with <%= @second_user.name %> - <% end %> - <% if @third_user.present? %> - and <%= @third_user.name %> + <% if @article.co_author_ids.present? %> + with <%= @article.co_author_name_and_path.html_safe %> <% end %> <% if should_show_updated_on?(@article) %> diff --git a/db/migrate/20200910155145_add_co_author_ids_to_articles.rb b/db/migrate/20200910155145_add_co_author_ids_to_articles.rb new file mode 100644 index 000000000..3973a2cf6 --- /dev/null +++ b/db/migrate/20200910155145_add_co_author_ids_to_articles.rb @@ -0,0 +1,5 @@ +class AddCoAuthorIdsToArticles < ActiveRecord::Migration[6.0] + def change + add_column :articles, :co_author_ids, :bigint, array: true, default: [] + end +end diff --git a/db/schema.rb b/db/schema.rb index 959b94af2..7e35af2b3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -90,6 +90,7 @@ ActiveRecord::Schema.define(version: 2020_10_02_102303) do t.string "cached_user_name" t.string "cached_user_username" t.string "canonical_url" + t.bigint "co_author_ids", default: [], array: true t.bigint "collection_id" t.integer "comment_score", default: 0 t.string "comment_template" diff --git a/lib/data_update_scripts/20200916202343_backfill_co_author_ids_for_articles.rb b/lib/data_update_scripts/20200916202343_backfill_co_author_ids_for_articles.rb new file mode 100644 index 000000000..f2be36b3b --- /dev/null +++ b/lib/data_update_scripts/20200916202343_backfill_co_author_ids_for_articles.rb @@ -0,0 +1,11 @@ +module DataUpdateScripts + class BackfillCoAuthorIdsForArticles + def run + articles = Article.where.not(second_user_id: nil).or(Article.where.not(third_user_id: nil)) + articles.find_each do |article| + co_author_ids = [article.second_user_id, article.third_user_id].compact + article.update_columns(co_author_ids: co_author_ids) + end + end + end +end diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index a522ebff1..15eb025c5 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -62,6 +62,39 @@ RSpec.describe Article, type: :model do end end + describe "#validate co_authors" do + it "is invalid if the co_author is the same as the author" do + article.co_author_ids = [user.id] + + expect(article).not_to be_valid + end + + it "is invalid if there are duplicate co_authors for the same article" do + co_author1 = create(:user) + article.co_author_ids = [co_author1, co_author1] + + expect(article).not_to be_valid + end + + it "is invalid if the co_author is entered as a text value rather than an integer" do + article.co_author_ids = [user.id, "abc"] + + expect(article).not_to be_valid + end + + it "is invalid if the co_author ID is not greater than 0" do + article.co_author_ids = [user.id, 0] + + expect(article).not_to be_valid + end + + it "is valid if co_author_ids is nil" do + article.co_author_ids = nil + + expect(article).to be_valid + end + end + describe "#after_commit" do it "on update enqueues job to index article to elasticsearch" do article.save diff --git a/spec/requests/admin/articles_spec.rb b/spec/requests/admin/articles_spec.rb index ce78b0a04..1a003de62 100644 --- a/spec/requests/admin/articles_spec.rb +++ b/spec/requests/admin/articles_spec.rb @@ -18,19 +18,14 @@ RSpec.describe "/admin/articles", type: :request do get request expect do - article.update_columns(second_user_id: second_user.id) - end.to change(article, :second_user_id).from(nil).to(second_user.id) + article.update_columns(co_author_ids: [1]) + end.to change(article, :co_author_ids).from([]).to([1]) end - it "allows an Admin to add multiple co-authors to an individual article" do - article.update_columns(second_user_id: second_user.id, third_user_id: third_user.id) - + it "allows an Admin to add co-authors to an individual article" do + article.update_columns(co_author_ids: [2, 3]) get request - - article.reload - - expect(article.second_user_id).to eq(second_user.id) - expect(article.third_user_id).to eq(third_user.id) + expect(article.co_author_ids).to eq([2, 3]) end end end diff --git a/spec/requests/stories_show_spec.rb b/spec/requests/stories_show_spec.rb index 9b1ef5935..69c1361c5 100644 --- a/spec/requests/stories_show_spec.rb +++ b/spec/requests/stories_show_spec.rb @@ -103,7 +103,7 @@ RSpec.describe "StoriesShow", type: :request do it "renders second and third users if present" do # 3rd user doesn't seem to get rendered for some reason user2 = create(:user) - article.update(second_user_id: user2.id) + article.update(co_author_ids: [user2.id]) get article.path expect(response.body).to include "with " end