tests: refactor set_published_at tests into requests tests

[just1602]

What does this pull request do?

Those tests were failing due to a change of syntax bring by rubocop, and after debugging them, I release they would be way to port the tests to requests test and avoid running a browser and slow tests.

How should this be manually tested?

Make sure all specs are still passing.

Is there any background context you want to provide for reviewers?

I was debugging some spec failling due to rubocop rules fix and I realize those specs could be refactored into not using a browser.

Acceptance Criteria

Questions for the pull request author (delete any that don't apply)

• Are any needed README/wiki/documentation updates needed for this pull request?
• Does the code you updated have tests? If not, could you add some please?
• Does this pull request require any new server dependencies which need to be added to the build process? If so, please explain what.

Questions for the reviewer

• This pull request does not cause the database export script to become out of sync with the db schema

---

@Bargraph6 you seem to be the rspec domain expert around here, could you give this a look. I'm pretty sure there's a way to make those specs cleaner.

[Bargraph6]

edit: I had some time to look at this further. I ended up nesting your "creating an article when the params are missing" into a shared context, so the structure looks like:

admin article controller set_published_at before_action
  when creating an article
    is expected to eq "2018-12-24 11:59:00 UTC"
    when the publication info is missing
      when the article is marked as published
        when you are a publisher
          is expected to eq "2126-05-20 21:10:45 UTC"
        when you are a regular user
          is expected to be nil
      when the article is still draft
        is expected to be nil
  when updating an existing article
    is expected to eq "2018-12-24 11:59:00 UTC"

I did this so that the publisher/nonpublisher test cases can share the exact same setup code, so the only thing that varies between those two cases is the user role.

The output from this structure also makes it clear what parts of the logic are not explicitly tested (for example, we use regular_user in most test cases, only switching to publisher in that one context.

updated the list of patches below in case you want to experiment with them

---

@just1602 nice! it would be great to be able to cover this logic without firing up a headless browser.

I think you can continue pulling all of the common code up into the before block.

I left a couple examples of what I mean, and have uploaded them as patches too

0001-Summary-wrap-missing-date-test-in-context-blocks.patch
0002-move-sign-in-code-to-before-block.patch
0003-move-all-setup-code-out-of-it-blocks.patch
0004-re-organize-the-create-specs.patch
0005-reduce-line-noise.patch

I would just keep going with your refactor. You have the coverage in place, so now you can focus on the setup code in each example.

I would just pull out the setup code for each individual example first. So the first one might end up looking like:

RSpec.describe 'admin article controller set_published_at before_action' do
  # rubocop:enable RSpec/DescribeClass
  let(:regular_user) { create(:user, username: 'user1', password: 'c' * 31) }
  let(:publisher) { create(:user, username: 'publisher1', password: 'c' * 31, role: 'publisher') }
  # if you add the :draft trait to the factory that saves some repetition  
  let(:draft_attrs) { attributes_for(:article, :draft, published_at: nil).compact }
  let(:request_params) { { button: '', controller: 'admin/articles' } }

  before do
    post sessions_url, params: { username: user.username, password: user.password }
  end

  describe 'when creating an article' do
    # this is debatable. potentially the post should be in the subject
    # since it is really the thing under test, but doing it this way is
    # convenient 
    subject { Article.last.published_at }

    # I prefer everything here to communicate "these are the unique things
    # I am changing just for this spec". Yes, there is a layer of
    # indirection behind the super(), but ideally that communicates to
    # future readers that everything hidden behind the super() is
    # unimportant for the test at hand.
    let(:user) { regular_user }
    let(:article_attrs) { draft_attrs.merge(published_at_tz: 'UTC') }
    let(:request_params) { super().merge(published_at_date: '2018-12-24',  published_at_time: '11:59:00', action:'create') }

    before { post admin_articles_url, params: { article: article_attrs, **request_params } }

    it { is_expected.to  eq('2018-12-24 11:59:00 UTC') }
  end

I would go through that process for each example, and then you should be able to see the differences pretty easily and possible end up naming those difference as lets and putting it all in a shared example (though maybe not, I am not sure yet if that would read well in this case)

[Bargraph6]

This spec is passing, but the description says "when the publication date is missing", however it is actually included in this spec. which is a bit confusing at first glance.

It is passing because the timezone info is missing, and the admin controller doesn't set the article's publication info if any of date, time, or timezone are missing:

website/app/controllers/admin/admin_controller.rb

Line 22 in 114db4e

return if date.blank? || time.blank? || tz_string.blank?

To verify, adding the timezone below line 71 will make the spec fail:

      article_attrs[:published_at_tz] = 'UTC'

---

I think just deleting lines 75 and 76, to make the test setup match the description, would clear things up

[Bargraph6]

When comparing relative times, you'll want to freeze the time or it will be a flaky test in CI (i.e. Time.now can be a different value between when it is called the first time in the application code and when it is called a second time in this expect).

The easy fix is to use the active_support time helpers: https://api.rubyonrails.org/classes/ActiveSupport/Testing/TimeHelpers.html

an easy way for this test is just saving off a "now" value at the top-level of this spec file:

let(:now) { Time.current }

wrap the post calls in a travel_to or freeze_time block:

travel_to(now) do
  post admin_articles_url, params: {...)
end

and then you are guaranteed that when the application code called Time.current, it returned your saved off now value. So you can use the relative dates safely in the expect

# "now" is your saved off time variable
expect(Article.last.published_at.to_s).to eq((now.utc + 100.years).to_s)

[Bargraph6]

@just1602 I updated the list of patches above and it now include a fully fleshed out example of the travel_to(now) thing

[Bargraph6]

In rspec it is helpful to determine what are the different contexts you need to test under.

for this spec it can be a bit tricky to decide on how to split it up, since we have draft/published articles, and publisher/regular user.

it seems like the most important difference is the publisher/regular user code path, so I think it makes sense to focus on that for now.

---

with that said, this describe is mixing the regular user and publisher context into a single context:

  admin article controller set_published_at before_action
    when creating an article
      compute the published_at attribute properly
    when creating a draft article without publication date info
      sets published_at to nil
    when creating a published article without publication date info
      compute set published_at to nil if you're not a publisher
      compute the published_at in 100 years to avoid unwanted publication if you're a publisher
    when updating an existing article
      compute the published_at attribute properly

we should wrap those it blocks into separate context, something like this:

diff --git a/spec/requests/article_datetime_settings_spec.rb b/spec/requests/article_datetime_settings_spec.rb
index f62ee712..8857a6fd 100644
--- a/spec/requests/article_datetime_settings_spec.rb
+++ b/spec/requests/article_datetime_settings_spec.rb
@@ -64,7 +64,8 @@ RSpec.describe 'admin article controller set_published_at before_action' do
   end
 
   describe 'when creating a published article without publication date info' do
-    it "compute the published_at in 100 years to avoid unwanted publication if you're a publisher" do
+    context 'when you are a publisher' do
+      it "compute the published_at in 100 years to avoid unwanted publication" do
         post sessions_url, params: { username: publisher.username, password: publisher.password }
 
         article_attrs = attributes_for(:article)
@@ -80,12 +81,14 @@ RSpec.describe 'admin article controller set_published_at before_action' do
 
         expect(Article.last.published_at.to_s).to eq((Time.now.utc + 100.years).to_s)
       end
+    end
 
+    context 'when you are a regular user' do
       # FIXME: is this a bug? Should we catch this in the controller, and refuse
       # to set publication_status to `publishe` when it's someone who hasn't the
       # `publisher` role and set a flash notice to they know the article wasn't
       # published.
-    it "compute set published_at to nil if you're not a publisher" do
+      it "compute set published_at to nil" do
         post sessions_url, params: { username: user.username, password: user.password }
 
         article_attrs = attributes_for(:article)
@@ -103,3 +106,4 @@ RSpec.describe 'admin article controller set_published_at before_action' do
       end
     end
   end
+end

will give us a test structure more like this:

  admin article controller set_published_at before_action
    when updating an existing article
      compute the published_at attribute properly
    when creating a draft article without publication date info
      sets published_at to nil
    when creating a published article without publication date info
      when you are a publisher
        compute the published_at in 100 years to avoid unwanted publication
      when you are a regular user
        compute set published_at to nil
    when creating an article
      compute the published_at attribute properly

With that done, you can now start pulling all of the common setup code into shared before blocks. For example the sign in code can be pulled to the top:

diff --git a/spec/requests/article_datetime_settings_spec.rb b/spec/requests/article_datetime_settings_spec.rb
index 8857a6fd..512920ec 100644
--- a/spec/requests/article_datetime_settings_spec.rb
+++ b/spec/requests/article_datetime_settings_spec.rb
@@ -3,13 +3,17 @@ require 'rails_helper'
 # rubocop:disable RSpec/DescribeClass
 RSpec.describe 'admin article controller set_published_at before_action' do
   # rubocop:enable RSpec/DescribeClass
-  let(:user) { create(:user, username: 'user1', password: 'c' * 31) }
+  let(:regular_user) { create(:user, username: 'user1', password: 'c' * 31) }
   let(:publisher) { create(:user, username: 'publisher1', password: 'c' * 31, role: 'publisher') }
 
-  describe 'when creating an article' do
-    it 'compute the published_at attribute properly' do
+  before do
     post sessions_url, params: { username: user.username, password: user.password }
+  end
 
+  describe 'when creating an article' do
+    let(:user) { regular_user }
+
+    it 'compute the published_at attribute properly' do
       article_attrs = attributes_for(:article, publication_status: :draft)
       article_attrs.delete(:published_at)
       article_attrs[:published_at_tz] = 'UTC'
@@ -27,9 +31,9 @@ RSpec.describe 'admin article controller set_published_at before_action' do
   end
 
   describe 'when updating an existing article' do
-    it 'compute the published_at attribute properly' do
-      post sessions_url, params: { username: user.username, password: user.password }
+    let(:user) { regular_user }
 
+    it 'compute the published_at attribute properly' do
       article_attrs = attributes_for(:article)
       article_attrs[:published_at] = DateTime.parse('2018-12-25T03:59:00Z')
       post admin_articles_url, params: {
@@ -47,9 +51,9 @@ RSpec.describe 'admin article controller set_published_at before_action' do
   end
 
   describe 'when creating a draft article without publication date info' do
-    it 'sets published_at to nil' do
-      post sessions_url, params: { username: user.username, password: user.password }
+    let(:user) { regular_user }
 
+    it 'sets published_at to nil' do
       article_attrs = attributes_for(:article, publication_status: :draft)
       article_attrs.delete(:published_at)
       post admin_articles_url, params: {
@@ -65,6 +69,8 @@ RSpec.describe 'admin article controller set_published_at before_action' do
 
   describe 'when creating a published article without publication date info' do
     context 'when you are a publisher' do
+      let(:user) { publisher }
+
       it "compute the published_at in 100 years to avoid unwanted publication" do
         post sessions_url, params: { username: publisher.username, password: publisher.password }
 
@@ -84,6 +90,8 @@ RSpec.describe 'admin article controller set_published_at before_action' do
     end
 
     context 'when you are a regular user' do
+      let(:user) { regular_user }
+
       # FIXME: is this a bug? Should we catch this in the controller, and refuse
       # to set publication_status to `publishe` when it's someone who hasn't the
       # `publisher` role and set a flash notice to they know the article wasn't

[just1602]

@Bargraph6 I've done a second pass, but I think I'm stock.

The first before block doesn't seem to be overridden by the later / lower one, which make the skipped test fail.

Also, I'd be interested to see you make it works with a share_example.

Anyway, thanks for your review and suggestions, it's the first time I have the impression to start to properly understand rspec.

[Bargraph6]

@just1602 every nesting level in rspec inherits and runs the levels above it. So defining a nested before block doesn't overwrite the previously defined block, it extends it.

So with this setup you are running the post request twice for each spec (line 29 and then line 45/54), user and publisher, which is likely why the test is slower.

[Bargraph6]

@just1602 you can remove this before as it is already running.

It will run with request_params_with_date as that is what is defined as the top level.

you can either add a let on line 63

let(:request_params_with_date) { super().merge(published_at: nil).compact }

or you can change the top level before to use the request_params, and then in the contexts where you want a date do (same a what you did with user. The generic user is used at the top-level, and you did a let(:user) { regular_user/publisher } in the relevant context blocks

let(:request_params) { request_params_with_date }

[Bargraph6]

@Bargraph6 I've done a second pass, but I think I'm stock.

The first before block doesn't seem to be overridden by the later / lower one, which make the skipped test fail.

Also, I'd be interested to see you make it works with a share_example.

Anyway, thanks for your review and suggestions, it's the first time I have the impression to start to properly understand rspec.

@just1602 I am afk, but once I can I will pull down this branch.

I think if you invert you request params ordering you will get unstuck.

In general, when defining the test setup in rspec you go from most general on the outside to more specific as you move in through the nested contexts.

You have 'request_params_with_date' being used at the top-level, when it isn't needed in every context.

so if you put the top-level before to use request_params and then overwrite the request_params in specific contexts (like you are doing with user/normal_user/publisher), you should be able to get everything passing (I think, not sure until I can pull down the branch and take a closer look)

[just1602]

@Bargraph6 I've done a second pass, but I think I'm stock.

The first before block doesn't seem to be overridden by the later / lower one, which make the skipped test fail.

Also, I'd be interested to see you make it works with a share_example.

Anyway, thanks for your review and suggestions, it's the first time I have the impression to start to properly understand rspec.

@just1602 I am afk, but once I can I will pull down this branch.

I think if you invert you request params ordering you will get unstuck.

In general, when defining the test setup in rspec you go from most general on the outside to more specific as you move in through the nested contexts.

You have 'request_params_with_date' being used at the top-level, when it isn't needed in every context.

so if you put the top-level before to use request_params and then overwrite the request_params in specific contexts (like you are doing with user/normal_user/publisher), you should be able to get everything passing (I think, not sure until I can pull down the branch and take a closer look)

Yeah, I tried to DRY things up, but I don't think it worked out.

I'll try that.

[just1602]

We need to merge #5400 first if we want the CI to pass again.

[Bargraph6]

@just1602 nice, looks good. You still have one double post causing a slowdown (the spec with the time travel)

you can move the time travel to the top level, as all of the other specs dont have a dependency on time.

you can also collapse the rubocop directives to a single line:

0001-Remove-double-post.patch

diff --git a/spec/requests/article_datetime_settings_spec.rb b/spec/requests/article_datetime_settings_spec.rb
index 302d0974..020feb34 100644
--- a/spec/requests/article_datetime_settings_spec.rb
+++ b/spec/requests/article_datetime_settings_spec.rb
@@ -1,10 +1,7 @@
 require 'rails_helper'
 
-# rubocop:disable RSpec/MultipleMemoizedHelpers
-# rubocop:disable RSpec/NestedGroups
-# rubocop:disable RSpec/DescribeClass
+# rubocop:disable RSpec/MultipleMemoizedHelpers,RSpec/DescribeClass,RSpec/NestedGroups
 RSpec.describe 'admin article controller set_published_at before_action set published_at' do
-  # rubocop:enable RSpec/DescribeClass
   include ActiveSupport::Testing::TimeHelpers
 
   subject { Article.last.published_at }
@@ -16,9 +13,13 @@ RSpec.describe 'admin article controller set_published_at before_action set publ
   let(:published_attrs) { attributes_for(:article, publication_at_tz: 'UTC', published_at: nil).compact }
   let(:request_params) { { button: '', controller: 'admin/articles' } }
 
+  let(:now) { Time.now.utc }
+
   before do
     post sessions_url, params: { username: user.username, password: user.password }
-    post admin_articles_url, params: { article: article_attrs, **request_params }
+    travel_to(now) do
+      post admin_articles_url, params: { article: article_attrs, **request_params }
+    end
   end
 
   describe 'when creating an article' do
@@ -39,13 +40,6 @@ RSpec.describe 'admin article controller set_published_at before_action set publ
           subject { Article.last.published_at.to_s }
 
           let(:user) { publisher }
-          let(:now) { Time.now.utc }
-
-          before do
-            travel_to(now) do
-              post admin_articles_url, params: { article: article_attrs, **request_params }
-            end
-          end
 
           it { is_expected.to eq((now.utc + 100.years).to_s) }
         end
@@ -76,5 +70,4 @@ RSpec.describe 'admin article controller set_published_at before_action set publ
     it { is_expected.to eq('2018-12-24 11:59:00 UTC') }
   end
 end
-# rubocop:enable RSpec/NestedGroups
-# rubocop:enable RSpec/MultipleMemoizedHelpers
+# rubocop:enable RSpec/MultipleMemoizedHelpers,RSpec/DescribeClass,RSpec/NestedGroups

[just1602]

@just1602 nice, looks good. You still have one double post causing a slowdown (the spec with the time travel)

you can move the time travel to the top level, as all of the other specs dont have a dependency on time.

you can also collapse the rubocop directives to a single line:

0001-Remove-double-post.patch

diff --git a/spec/requests/article_datetime_settings_spec.rb b/spec/requests/article_datetime_settings_spec.rb
index 302d0974..020feb34 100644
--- a/spec/requests/article_datetime_settings_spec.rb
+++ b/spec/requests/article_datetime_settings_spec.rb
@@ -1,10 +1,7 @@
 require 'rails_helper'
 
-# rubocop:disable RSpec/MultipleMemoizedHelpers
-# rubocop:disable RSpec/NestedGroups
-# rubocop:disable RSpec/DescribeClass
+# rubocop:disable RSpec/MultipleMemoizedHelpers,RSpec/DescribeClass,RSpec/NestedGroups
 RSpec.describe 'admin article controller set_published_at before_action set published_at' do
-  # rubocop:enable RSpec/DescribeClass
   include ActiveSupport::Testing::TimeHelpers
 
   subject { Article.last.published_at }
@@ -16,9 +13,13 @@ RSpec.describe 'admin article controller set_published_at before_action set publ
   let(:published_attrs) { attributes_for(:article, publication_at_tz: 'UTC', published_at: nil).compact }
   let(:request_params) { { button: '', controller: 'admin/articles' } }
 
+  let(:now) { Time.now.utc }
+
   before do
     post sessions_url, params: { username: user.username, password: user.password }
-    post admin_articles_url, params: { article: article_attrs, **request_params }
+    travel_to(now) do
+      post admin_articles_url, params: { article: article_attrs, **request_params }
+    end
   end
 
   describe 'when creating an article' do
@@ -39,13 +40,6 @@ RSpec.describe 'admin article controller set_published_at before_action set publ
           subject { Article.last.published_at.to_s }
 
           let(:user) { publisher }
-          let(:now) { Time.now.utc }
-
-          before do
-            travel_to(now) do
-              post admin_articles_url, params: { article: article_attrs, **request_params }
-            end
-          end
 
           it { is_expected.to eq((now.utc + 100.years).to_s) }
         end
@@ -76,5 +70,4 @@ RSpec.describe 'admin article controller set_published_at before_action set publ
     it { is_expected.to eq('2018-12-24 11:59:00 UTC') }
   end
 end
-# rubocop:enable RSpec/NestedGroups
-# rubocop:enable RSpec/MultipleMemoizedHelpers
+# rubocop:enable RSpec/MultipleMemoizedHelpers,RSpec/DescribeClass,RSpec/NestedGroups

I thought it was the travel_to who was slow so I jeep it into a single example, but if it's fine I'll so the refactor.

Thanks for the rubocop comments, I tried without commas and thought it wasn't possible on one line.

[Bargraph6]

@just1602 the travel to is not slow.

I wrapped up the "create" context in a shared example, which resolved one of the rubocop issues.

I think I can further modify the test to get rid of the nesting rubocop issue, but I need to step away for a bit. Here are the patches:

0001-Remove-double-post.patch
0002-Use-shared_examples.patch

and the full code incase the patches dont apply

the full spec code

require 'rails_helper'

# rubocop:disable RSpec/DescribeClass,RSpec/NestedGroups
RSpec.describe 'admin article controller set_published_at before_action set published_at' do
  include ActiveSupport::Testing::TimeHelpers

  let(:draft_attrs) { attributes_for(:article, :draft, published_at_tz: 'UTC', published_at: nil).compact }
  let(:published_attrs) { attributes_for(:article, publication_at_tz: 'UTC', published_at: nil).compact }
  let(:request_params) { { button: '', controller: 'admin/articles' } }

  shared_examples 'handles the publication date' do |user_role|
    subject(:actual_published_at) { Article.last.published_at.to_s }

    let(:user) { create(:user, password: 'c' * 31, role: user_role) }
    let(:now) { Time.now.utc }

    before do
      post sessions_url, params: { username: user.username, password: user.password }
      travel_to(now) do
        post admin_articles_url, params: { article: article_attrs, **request_params }
      end
    end

    it "works with a role of #{user_role}" do
      expect(actual_published_at).to eq expected_published_at
    end
  end

  describe 'when creating an article' do
    let(:request_params) do
      super().merge(published_at_date: '2018-12-24', published_at_time: '11:59:00', action: 'create')
    end

    it_behaves_like 'handles the publication date', :author do
      let(:expected_published_at) { '2018-12-24 11:59:00 UTC' }
      let(:article_attrs) { draft_attrs }
    end

    context 'when the publication info is missing' do
      let(:request_params) { super().merge(published_at_date: nil, published_at_time: nil, action: 'create') }

      describe 'when the article is marked as published' do
        let(:article_attrs) { published_attrs.merge(published_at: nil) }

        it_behaves_like 'handles the publication date', :publisher do
          let(:expected_published_at) { (now.utc + 100.years).to_s }
        end

        it_behaves_like 'handles the publication date', :author do
          let(:expected_published_at) { '' }
        end
      end

      describe 'when the article is still a draft' do
        let(:article_attrs) { draft_attrs.merge(published_at: nil) }

        it_behaves_like 'handles the publication date', :publisher do
          let(:expected_published_at) { '' }
        end

        it_behaves_like 'handles the publication date', :author do
          let(:expected_published_at) { '' }
        end
      end
    end
  end

  describe 'when updating an existing article' do
    subject(:actual_published_at) { Article.last.published_at.to_s }

    let(:user) { create(:user, password: 'c' * 31, role: :author) }
    let(:request_params) do
      super().merge(published_at_date: '2018-12-24', published_at_time: '11:59:00', action: 'update')
    end
    let(:article_attrs) { draft_attrs.merge(published_at: DateTime.parse('2018-12-25T03:59:00Z')) }

    before do
      post sessions_url, params: { username: user.username, password: user.password }
      create(:article, :draft)
      put admin_article_url(Article.last.id), params: { article: draft_attrs, **request_params }
    end

    it { is_expected.to eq('2018-12-24 11:59:00 UTC') }
  end
end
# rubocop:enable RSpec/DescribeClass,RSpec/NestedGroups

with the share exmple the spec structure looks like:

admin article controller set_published_at before_action set published_at
  when updating an existing article
    is expected to eq "2018-12-24 11:59:00 UTC"
  when creating an article
    behaves like handles the publication date
      works with a role of author
    when the publication info is missing
      when the article is marked as published
        behaves like handles the publication date
          works with a role of publisher
        behaves like handles the publication date
          works with a role of author
      when the article is still a draft
        behaves like handles the publication date
          works with a role of publisher
        behaves like handles the publication date
          works with a role of author

Finished in 0.77175 seconds (files took 2.33 seconds to load)
6 examples, 0 failures

I am wondering if we should make the paths "allowed groups" so they don't count as a nesting level: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/NestedGroups

[Bargraph6]

@just1602 I was able to get rid of the other rubocop warning, and in the process maybe discovered a (very unlikely to actually happen) bug?

0003-move-update-to-shared-example.patch

I move the update specs to the shared example too, this means the update path got increased coverage. The new structure looks like this:

admin article controller set_published_at before_action set published_at
  when the publication info is missing
    when the article is still a draft
      behaves like handles the publication date
        when updating an existing article record with role: author
          is expected to eq ""
        when creating an article record with role: author
          is expected to eq ""
      behaves like handles the publication date
        when updating an existing article record with role: publisher
          is expected to eq ""
        when creating an article record with role: publisher
          is expected to eq ""
    when the article is marked as published
      behaves like handles the publication date
        when updating an existing article record with role: author
          example at ./spec/requests/article_datetime_settings_spec.rb:39 (FAILED - 1)
        when creating an article record with role: author
          is expected to eq ""
      behaves like handles the publication date
        when updating an existing article record with role: publisher
          example at ./spec/requests/article_datetime_settings_spec.rb:39 (FAILED - 2)
        when creating an article record with role: publisher
          is expected to eq "2126-05-25 06:58:02 UTC"
  with valid publication info
    behaves like handles the publication date
      when updating an existing article record with role: author
        is expected to eq "2018-12-24 11:59:00 UTC"
      when creating an article record with role: author
        is expected to eq "2018-12-24 11:59:00 UTC"

Failures:

  1) admin article controller set_published_at before_action set published_at when the publication info is missing when the article is marked as published behaves like handles the publication date when updating an existing article record with role: author 
     Failure/Error: published_at.strftime("/%Y/%m/%d/#{slug}")
     
     NoMethodError:
       undefined method 'strftime' for nil
     Shared Example Group: "handles the publication date" called from ./spec/requests/article_datetime_settings_spec.rb:64
     # ./app/models/article.rb:55:in 'Article#path'
     # ./app/models/article.rb:149:in 'Article#update_or_create_redirect'
     # ./app/controllers/admin/articles_controller.rb:56:in 'Admin::ArticlesController#update'
     # ./app/middlewares/rack/redirect.rb:22:in 'Rack::Redirect#call'
     # ./app/middlewares/rack/teapot.rb:47:in 'Rack::Teapot#call'
     # ./app/middlewares/rack/pic_twitter_redirect.rb:26:in 'Rack::PicTwitterRedirect#call'
     # ./app/middlewares/rack/clean_path.rb:68:in 'Rack::CleanPath#call'
     # ./app/middlewares/rack/domain_redirect.rb:88:in 'Rack::DomainRedirect#call'
     # ./app/middlewares/rack/apex_redirect.rb:12:in 'Rack::ApexRedirect#call'
     # ./app/middlewares/rack/json_requests.rb:17:in 'Rack::JSONRequests#call'
     # ./spec/requests/article_datetime_settings_spec.rb:34:in 'block (5 levels) in <top (required)>'
     # ./spec/requests/article_datetime_settings_spec.rb:33:in 'block (4 levels) in <top (required)>'

  2) admin article controller set_published_at before_action set published_at when the publication info is missing when the article is marked as published behaves like handles the publication date when updating an existing article record with role: publisher 
     Failure/Error: published_at.strftime("/%Y/%m/%d/#{slug}")
     
     NoMethodError:
       undefined method 'strftime' for nil
     Shared Example Group: "handles the publication date" called from ./spec/requests/article_datetime_settings_spec.rb:60
     # ./app/models/article.rb:55:in 'Article#path'
     # ./app/models/article.rb:149:in 'Article#update_or_create_redirect'
     # ./app/controllers/admin/articles_controller.rb:56:in 'Admin::ArticlesController#update'
     # ./app/middlewares/rack/redirect.rb:22:in 'Rack::Redirect#call'
     # ./app/middlewares/rack/teapot.rb:47:in 'Rack::Teapot#call'
     # ./app/middlewares/rack/pic_twitter_redirect.rb:26:in 'Rack::PicTwitterRedirect#call'
     # ./app/middlewares/rack/clean_path.rb:68:in 'Rack::CleanPath#call'
     # ./app/middlewares/rack/domain_redirect.rb:88:in 'Rack::DomainRedirect#call'
     # ./app/middlewares/rack/apex_redirect.rb:12:in 'Rack::ApexRedirect#call'
     # ./app/middlewares/rack/json_requests.rb:17:in 'Rack::JSONRequests#call'
     # ./spec/requests/article_datetime_settings_spec.rb:34:in 'block (5 levels) in <top (required)>'
     # ./spec/requests/article_datetime_settings_spec.rb:33:in 'block (4 levels) in <top (required)>'

Finished in 0.89055 seconds (files took 2.21 seconds to load)
10 examples, 2 failures

Failed examples:

rspec ./spec/requests/article_datetime_settings_spec.rb[1:2:1:2:2:1] # admin article controller set_published_at before_action set published_at when the publication info is missing when the article is marked as published behaves like handles the publication date when updating an existing article record with role: author 
rspec ./spec/requests/article_datetime_settings_spec.rb[1:2:1:1:2:1] # admin article controller set_published_at before_action set published_at when the publication info is missing when the article is marked as published behaves like handles the publication date when updating an existing article record with role: publisher 

Randomized with seed 27824

If you try updating an already published article to a publication date of nil, it blows up in the redirect creation hook:

website/app/models/article.rb

Lines 139 to 151 in d4580d0

	def update_or_create_redirect
	return if short_path.blank?
	# TODO: name this conditional's concept, extract to a private method
	if redirect.present? &&
	(short_path_changed? || slug_changed? || published_at_changed? || publication_status_changed?)
	redirect.update(source_path: absolute_short_path, target_path: path)
	elsif Redirect.exists?(source_path: absolute_short_path)
	errors.add(:short_path, ' is a path that already points to a redirect')
	elsif publication_status == 'published'
	Redirect.create(source_path: absolute_short_path, target_path: path, article_id: id)
	end
	end

We should probably just have some sort of validation that prevents us from ever getting there with a published_at to nil?

[veganstraightedge]

@just1602 @Bargraph6 thanks for digging into this! I love when the test suite gets improved. 🤩

I merged some PRs that might have been blockers for yall to continue progress on this branch. Keep up the great work! It's very appreciated. 🎉