From fb6904f86dea99ef81304561e1ba4d7b1e586283 Mon Sep 17 00:00:00 2001 From: Ben Halpern Date: Thu, 16 May 2019 13:56:26 -0400 Subject: [PATCH] Fix up articles controller (#2855) * Fix up articles controller * Update request specs * Add proper post_under_org tests * Remove create_job_opportunity * Add user profile back in --- .../stylesheets/preact/article-form.scss | 2 +- app/controllers/articles_controller.rb | 97 ++++--------------- app/javascript/article-form/articleForm.jsx | 18 +++- app/services/article_creation_service.rb | 15 +-- app/views/articles/_v2_form.html.erb | 10 +- spec/requests/articles_create_spec.rb | 27 ++---- spec/requests/articles_update_spec.rb | 35 ++++--- 7 files changed, 69 insertions(+), 135 deletions(-) diff --git a/app/assets/stylesheets/preact/article-form.scss b/app/assets/stylesheets/preact/article-form.scss index ffe851338..5ee16c673 100644 --- a/app/assets/stylesheets/preact/article-form.scss +++ b/app/assets/stylesheets/preact/article-form.scss @@ -374,7 +374,7 @@ display: inline-block; padding: 2px 15px; border-radius: 3px; - width: 100px; + width: 110px; @media screen and (min-width: 600px) { margin-left: -5px; display: none; diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index 44b0ffc0f..aae5e5577 100644 --- a/app/controllers/articles_controller.rb +++ b/app/controllers/articles_controller.rb @@ -121,64 +121,31 @@ class ArticlesController < ApplicationController @user = current_user - respond_to do |format| - format.html do - @article = ArticleCreationService. - new(@user, article_params, job_opportunity_params: job_opportunity_params). - create! + @article = ArticleCreationService.new(@user, article_params_json).create! - redirect_after_creation - end - - format.json do - @article = ArticleCreationService.new(@user, article_params_json).create! - - render json: if @article.persisted? - @article.to_json(only: [:id], methods: [:current_state_path]) - else - @article.errors.to_json - end - end - end + render json: if @article.persisted? + @article.to_json(only: [:id], methods: [:current_state_path]) + else + @article.errors.to_json + end end def update authorize @article @user = @article.user || current_user + not_found if @article.user_id != @user.id && !@user.has_role?(:super_admin) + edited_at_date = if @article.user == current_user && @article.published + Time.current + else + @article.edited_at + end - if request.format.json? - not_found if @article.user_id != @user.id && !@user.has_role?(:super_admin) - render json: if @article.update(article_params_json) - @article.to_json(only: [:id], methods: [:current_state_path]) - else - @article.errors.to_json - end - else - @article.tag_list = [] - @article.main_image = nil - edited_at_date = if @article.user == current_user && @article.published - Time.current - else - @article.edited_at - end - - if @article.update(article_params.merge(edited_at: edited_at_date)) - handle_org_assignment - handle_hiring_tag - if @article.published - # why does it send a notification each time the published flag is true? - Notification.send_to_followers(@article, "Published") if @article.saved_changes["published_at"]&.include?(nil) - path = @article.path - else - Notification.remove_all_without_delay(notifiable_id: @article.id, notifiable_type: "Article", action: "Published") - path = "/#{@article.username}/#{@article.slug}?preview=#{@article.password}" - end - - redirect_to(params[:destination] || path) - else - redirect_to :edit - end - end + render json: if @article.update(article_params_json.merge(edited_at: edited_at_date)) + Notification.send_to_followers(@article, "Published") if @article.published && @article.saved_changes["published_at"]&.include?(nil) + @article.to_json(only: [:id], methods: [:current_state_path]) + else + @article.errors.to_json + end end def delete_confirm @@ -209,14 +176,6 @@ class ArticlesController < ApplicationController end end - def handle_hiring_tag - if job_opportunity_params.present? && @article.tag_list.include?("hiring") - create_or_update_job_opportunity - elsif @article.job_opportunity && !@article.tag_list.include?("hiring") - @article.job_opportunity.destroy! - end - end - def handle_user_or_organization_feed if (@user = User.find_by(username: params[:username])) @articles = @articles.where(user_id: @user.id) @@ -234,16 +193,6 @@ class ArticlesController < ApplicationController @articles = @articles.cached_tagged_with(@tag) end - def create_or_update_job_opportunity - if @article.job_opportunity.present? - @article.job_opportunity.update(job_opportunity_params) - else - @job_opportunity = JobOpportunity.create(job_opportunity_params) - @article.job_opportunity = @job_opportunity - @article.save - end - end - def set_article owner = User.find_by(username: params[:username]) || Organization.find_by(slug: params[:username]) found_article = if params[:slug] @@ -270,6 +219,7 @@ class ArticlesController < ApplicationController elsif params["article"]["series"] == "" params["article"]["collection_id"] = nil end + if params["article"]["version"] == "v1" params.require(:article).permit( :body_markdown, :organization_id @@ -282,15 +232,6 @@ class ArticlesController < ApplicationController end end - def job_opportunity_params - return nil if params[:article][:job_opportunity].blank? - - params[:article].require(:job_opportunity).permit( - :remoteness, :location_given, :location_city, :location_postal_code, - :location_country_code, :location_lat, :location_long - ) - end - def redirect_after_creation @article.decorate if @article.persisted? diff --git a/app/javascript/article-form/articleForm.jsx b/app/javascript/article-form/articleForm.jsx index 639325619..c649ef9fc 100644 --- a/app/javascript/article-form/articleForm.jsx +++ b/app/javascript/article-form/articleForm.jsx @@ -139,11 +139,19 @@ export default class ArticleForm extends Component { }; showPreview = response => { - this.setState({ - previewShowing: true, - helpShowing: false, - previewResponse: response, - }); + if (response.processed_html) { + this.setState({ + previewShowing: true, + helpShowing: false, + previewResponse: response, + errors: null, + }); + } else { + this.setState({ + errors: response, + submitting: false, + }); + } }; toggleOrgPosting = e => { diff --git a/app/services/article_creation_service.rb b/app/services/article_creation_service.rb index 5221cb719..7e508d112 100644 --- a/app/services/article_creation_service.rb +++ b/app/services/article_creation_service.rb @@ -1,8 +1,7 @@ class ArticleCreationService - def initialize(user, article_params, job_opportunity_params: {}) + def initialize(user, article_params) @user = user @article_params = article_params - @job_opportunity_params = job_opportunity_params end def create! @@ -27,24 +26,14 @@ class ArticleCreationService article.organization = user.organization if publish_under_org article.collection = Collection.find_series(series, user) if series.present? - create_job_opportunity(article) - if article.save! Notification.send_to_followers(article, "Published") if article.published end article.decorate end - def create_job_opportunity(article) - return if job_opportunity_params.blank? - - job_opportunity = JobOpportunity.create(job_opportunity_params) - article.job_opportunity = job_opportunity - raise unless article.tag_list.include? "hiring" - end - private - attr_reader :user, :job_opportunity_params + attr_reader :user attr_accessor :article_params end diff --git a/app/views/articles/_v2_form.html.erb b/app/views/articles/_v2_form.html.erb index ddec43dd6..17b43e249 100644 --- a/app/views/articles/_v2_form.html.erb +++ b/app/views/articles/_v2_form.html.erb @@ -3,8 +3,8 @@ id="article-form" data-article="<%= @article.to_json(only: %i[id title cached_tag_list published body_markdown main_image organization_id canonical_url], methods: %i[series all_series]) %>" data-organization="<%= @organization&.to_json(only: %i[name bg_color_hex text_color_hex], methods: [:profile_image_90]) %>" - data-version="<%= current_user.editor_version%>"> -
+ data-version="<%= @user.editor_version%>"> + <% if @organization %>
" alt="<%= @organization.name %> profile image"> @@ -14,7 +14,7 @@
<% end %> - <% if current_user.editor_version == "v2" %> + <% if @user.editor_version == "v2" %> <% if @article.main_image.present? %>
<% end %> @@ -30,7 +30,7 @@
- <% if current_user.editor_version == "v2" %> + <% if @user.editor_version == "v2" %> <% end %>
@@ -42,7 +42,7 @@
- <% if current_user.editor_version == "v2" %> + <% if @user.editor_version == "v2" %> <%= render "pages/editor_guide_text", version: "2" %> <% else %> <%= render "pages/editor_guide_text", version: "1" %> diff --git a/spec/requests/articles_create_spec.rb b/spec/requests/articles_create_spec.rb index ada04d5d4..f72d2138a 100644 --- a/spec/requests/articles_create_spec.rb +++ b/spec/requests/articles_create_spec.rb @@ -1,7 +1,8 @@ require "rails_helper" RSpec.describe "ArticlesCreate", type: :request do - let(:user) { create(:user) } + let(:organization) { create(:organization) } + let(:user) { create(:user, organization_id: organization.id) } before do sign_in user @@ -25,29 +26,15 @@ RSpec.describe "ArticlesCreate", type: :request do expect(Article.last.title).to eq("hey hey hahuu") end - it "does not allow job opportunity job to not include hiring tag" do - new_title = "NEW TITLE #{rand(100)}" - expect do - post "/articles", params: { - article: { - title: new_title, - body_markdown: "Yo ho ho#{rand(100)}", tag_list: "yoyo", - job_opportunity: { remoteness: "on_premise" } - } - } - end .to raise_error(RuntimeError) - end - - it "creates article with job opportunity nested" do - new_title = "NEW TITLE #{rand(100)}" + it "creates article with front matter params and org" do post "/articles", params: { article: { - title: new_title, - body_markdown: "Yo ho ho#{rand(100)}", tag_list: "hiring", - job_opportunity: { remoteness: "on_premise" } + body_markdown: "---\ntitle: hey hey hahuu\npublished: false\n---\nYo ho ho#{rand(100)}", + tag_list: "yo", + post_under_org: true } } - expect(Article.last.job_opportunity.remoteness).to eq("on_premise") + expect(Article.last.organization_id).to eq(organization.id) end it "creates series when series is created with frontmatter" do diff --git a/spec/requests/articles_update_spec.rb b/spec/requests/articles_update_spec.rb index f683087fb..74e0e942f 100644 --- a/spec/requests/articles_update_spec.rb +++ b/spec/requests/articles_update_spec.rb @@ -1,7 +1,10 @@ require "rails_helper" RSpec.describe "ArticlesUpdate", type: :request do - let(:user) { create(:user) } + let(:organization) { create(:organization) } + let(:organization2) { create(:organization) } + let(:user) { create(:user, organization_id: organization.id) } + let(:user2) { create(:user, organization_id: organization2.id) } let(:article) { create(:article, user_id: user.id) } before do @@ -16,25 +19,31 @@ RSpec.describe "ArticlesUpdate", type: :request do expect(Article.last.title).to eq(new_title) end - it "does not create a job opportunity if no hiring tag" do - new_title = "NEW TITLE #{rand(100)}" + it "updates article with front matter params" do put "/articles/#{article.id}", params: { article: { - title: new_title, body_markdown: "Yo ho ho#{rand(100)}", tag_list: "yo", - job_opportunity: { remoteness: "on_premise" } + title: "hello", + body_markdown: "---\ntitle: hey hey hahuu\npublished: false\n---\nYo ho ho#{rand(100)}", + tag_list: "yo" } } - expect(JobOpportunity.count).to eq(0) + expect(Article.last.edited_at).to be > 5.seconds.ago + expect(Article.last.title).to eq("hey hey hahuu") end - it "updates ordinary article with job opportunity nested" do - new_title = "NEW TITLE #{rand(100)}" + it "adds organization ID when user updates" do put "/articles/#{article.id}", params: { - article: { - title: new_title, body_markdown: "Yo ho ho#{rand(100)}", tag_list: "hiring", - job_opportunity: { remoteness: "on_premise" } - } + article: { post_under_org: true } } - expect(Article.last.job_opportunity.remoteness).to eq("on_premise") + expect(article.reload.organization_id).to eq organization.id end + + it "does not modify the organization ID when updating someone else's article as an admin" do + article.update_columns(organization_id: organization2.id, user_id: user2.id) + user.add_role(:super_admin) + put "/articles/#{article.id}", params: { + article: { post_under_org: true } + } + expect(article.reload.organization_id).to eq user2.organization_id + end end