diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index 942412955..c9e5e410c 100644 --- a/app/controllers/articles_controller.rb +++ b/app/controllers/articles_controller.rb @@ -65,13 +65,25 @@ class ArticlesController < ApplicationController } end + # @note The /new path is a unique creature. We want to ensure that folks coming to the /new with + # a prefill of information are first prompted to sign-in, and then given a form that + # prepopulates with that pre-fill information. This is a feature that StackOverflow and + # CodePen use to have folks post on Dev. def new base_editor_assignments - @article, store_location = Articles::Builder.call(@user, @tag, @prefill) + @article, needs_authorization = Articles::Builder.call(@user, @tag, @prefill) - authorize(Article) - store_location_for(:user, request.path) if store_location + if needs_authorization + authorize(Article) + else + skip_authorization + + # We want the query params for the request (as that is where we have the prefill). The + # `request.path` excludes the query parameters, so we're going with the `request.url` which + # includes the parameters. + store_location_for(:user, request.url) + end end def edit diff --git a/app/services/articles/builder.rb b/app/services/articles/builder.rb index 43516b2f0..7190d088f 100644 --- a/app/services/articles/builder.rb +++ b/app/services/articles/builder.rb @@ -14,30 +14,17 @@ module Articles new(...).call end - # the Builder returns a pair of [article, store_location] - # => store_location can be either true or false - # - # @note [@jeremyf] I renamed the boolean return value from - # needs_authorization to store_location. Why? because in the - # ArticlesController#new action (the one place that instantiates the - # Articles::Builder) when "needs_authorization" was true, we'd call - # authorize(Article). But at the time of writing, the implementation - # details of ArticlePolicy#new? always returned true. So the - # "needs_authorization" was in fact behaving as a "should we store - # this location or not?" Hence the rename and "switching the - # polarity" of the boolean. - # - # @see https://github.com/forem/forem/issues/16529 for snapshot of past - # state + # the Builder returns a pair of [article, needs_authorization?] + # => `needs_authorization? can be either true or false def call - return [tag_user_editor_v2, false] if tag && editor_version2 - return [tag_user, false] if tag&.submission_template.present? && user - return [prefill_user_editor_v2, false] if prefill.present? && editor_version2 - return [prefill_user, false] if prefill.present? && user - return [tag_article, true] if tag - return [user_editor_v2, true] if editor_version2 + return [tag_user_editor_v2, true] if tag && editor_version2 + return [tag_user, true] if tag&.submission_template.present? && user + return [prefill_user_editor_v2, true] if prefill.present? && editor_version2 + return [prefill_user, true] if prefill.present? && user + return [tag_article, false] if tag + return [user_editor_v2, false] if editor_version2 - [user_editor_v1, true] + [user_editor_v1, false] end private diff --git a/cypress/integration/seededFlows/loginFlows/userLogin.spec.js b/cypress/integration/seededFlows/loginFlows/userLogin.spec.js index 7fc13de2c..a71eadf2f 100644 --- a/cypress/integration/seededFlows/loginFlows/userLogin.spec.js +++ b/cypress/integration/seededFlows/loginFlows/userLogin.spec.js @@ -33,4 +33,44 @@ describe('User Login', () => { const { baseUrl } = Cypress.config(); cy.url().should('equal', `${baseUrl}?signin=true`); }); + + it('should propogate the /new?prefill parameter', () => { + cy.fixture('users/changePasswordUser.json').as('user'); + + // Go to home page + cy.visit( + '/new?prefill=---%0Atitle%3A%20A%20swarm%20in%20a%20box%0Apublished%3A%20true%0Atags%3A%20codepen%0A---%0A%0A%0A%0A%7B%25%20embed%20https%3A%2F%2Fcodepen.io%2Ftrajektorijus%2Fpen%2FBaJOVeE%20%25%7D', + ); + + cy.url().should('contains', '/new?prefill='); + + cy.findByTestId('login-form').as('loginForm'); + + cy.get('@user').then((user) => { + cy.get('@loginForm') + .findByText(/^Email$/) + .type(user.email); + cy.get('@loginForm') + .findByText(/^Password$/) + .type(user.password); + }); + + // Submit the form + cy.get('@loginForm').findByText('Continue').click(); + + cy.url().should('contains', '/new?prefill='); + + cy.findByRole('form', { name: /^Edit post$/i }).as('articleForm'); + + cy.get('@articleForm') + .findByLabelText('Post Title') + .should('have.value', 'A swarm in a box'); + + cy.get('@articleForm') + .findByLabelText('Post Content') + .should( + 'have.value', + '{% embed https://codepen.io/trajektorijus/pen/BaJOVeE %}', + ); + }); }); diff --git a/spec/requests/editor_spec.rb b/spec/requests/editor_spec.rb index 9a61e0f2e..407d59025 100644 --- a/spec/requests/editor_spec.rb +++ b/spec/requests/editor_spec.rb @@ -7,20 +7,7 @@ RSpec.describe "Editor", type: :request do let(:user) { create(:user) } context "when not authenticated" do - it { within_block_is_expected.to raise_error ApplicationPolicy::UserRequiredError } - end - - context "when authenticated but not authorized" do - before do - login_as user - allow(ArticlePolicy).to receive(:limit_post_creation_to_admins?).and_return(true) - end - - # [@jeremyf] We're handling the authentication and authorization exceptions just a bit - # differently. In this case (e.g. they don't have permission) we are relying on - # the application configuration to gracefully handle the authorization error (as it - # has prior and up to <2022-02-17 Thu>). - it { within_block_is_expected.to raise_error(Pundit::NotAuthorizedError) } + it { is_expected.to eq(200) } end context "when authenticated and authorized" do diff --git a/spec/services/articles/builder_spec.rb b/spec/services/articles/builder_spec.rb index 7b448717d..ecb0261ab 100644 --- a/spec/services/articles/builder_spec.rb +++ b/spec/services/articles/builder_spec.rb @@ -24,7 +24,7 @@ RSpec.describe Articles::Builder, type: :service do expect(subject).to be_an_instance_of(Article) expect(subject).to have_attributes(correct_attributes) - expect(store_location).to be false + expect(store_location).to be true end end @@ -43,7 +43,7 @@ RSpec.describe Articles::Builder, type: :service do expect(subject).to be_an_instance_of(Article) expect(subject).to have_attributes(correct_attributes) - expect(store_location).to be false + expect(store_location).to be true end end @@ -65,7 +65,7 @@ RSpec.describe Articles::Builder, type: :service do expect(subject).to be_an_instance_of(Article) expect(subject).to have_attributes(correct_attributes) - expect(store_location).to be false + expect(store_location).to be true end end @@ -84,7 +84,7 @@ RSpec.describe Articles::Builder, type: :service do expect(subject).to be_an_instance_of(Article) expect(subject).to have_attributes(correct_attributes) - expect(store_location).to be false + expect(store_location).to be true end end @@ -105,7 +105,7 @@ RSpec.describe Articles::Builder, type: :service do expect(subject).to be_an_instance_of(Article) expect(subject).to have_attributes(correct_attributes) - expect(store_location).to be true + expect(store_location).to be false end end @@ -122,7 +122,7 @@ RSpec.describe Articles::Builder, type: :service do expect(subject).to be_an_instance_of(Article) expect(subject).to have_attributes(correct_attributes) - expect(store_location).to be true + expect(store_location).to be false end end @@ -145,7 +145,7 @@ RSpec.describe Articles::Builder, type: :service do expect(subject).to be_an_instance_of(Article) expect(subject).to have_attributes(correct_attributes) - expect(store_location).to be true + expect(store_location).to be false end end end