Ensuring /new remains a call to action end point for DEV (#17294)

There are two problems engrained in this:

1. The undocumented expectation of `/new` needed to other sites linking
with prefill information.
2. The prefill parameters being lost during the authentication process.

The first bug was introduced as part forem/forem#16536.  The second is
likely introduced as part of a Rails upgrade (namely `request.path` most
likely once included the query parameters but with our current
implementation it does not).

Closes forem/forem#17292

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

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

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 %}',
+      );
+  });
 });

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

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