From be4a738f1d08ecb9356241bdb797a579be5dc8db Mon Sep 17 00:00:00 2001
From: Jorge Manrubia <jorge@37signals.com>
Date: Fri, 4 Apr 2025 14:53:39 +0200
Subject: [PATCH] Make reason not null

---
 app/models/bubble/poppable.rb                 |  4 +--
 app/models/pop.rb                             |  4 ---
 app/views/bubbles/_card.html.erb              |  2 +-
 .../20250404083727_add_reason_to_pops.rb      |  6 ++++
 db/schema.rb                                  |  2 +-
 db/schema_cache.yml                           | 32 +++++++++----------
 test/fixtures/pops.yml                        |  1 +
 test/models/pop_test.rb                       |  8 -----
 8 files changed, 27 insertions(+), 32 deletions(-)
 delete mode 100644 test/models/pop_test.rb

diff --git a/app/models/bubble/poppable.rb b/app/models/bubble/poppable.rb
index a3425bfe6..ab28408ba 100644
--- a/app/models/bubble/poppable.rb
+++ b/app/models/bubble/poppable.rb
@@ -15,7 +15,7 @@ module Bubble::Poppable
   class_methods do
     def auto_pop_all_due
       due_to_be_popped.find_each do |bubble|
-        bubble.pop!(user: bubble.bucket.account.users.system)
+        bubble.pop!(user: bubble.bucket.account.users.system, reason: "Closed")
       end
     end
   end
@@ -40,7 +40,7 @@ module Bubble::Poppable
     pop&.created_at
   end
 
-  def pop!(user: Current.user, reason: nil)
+  def pop!(user: Current.user, reason: Account::PopReasons::FALLBACK_LABEL)
     unless popped?
       transaction do
         create_pop! user: user, reason: reason
diff --git a/app/models/pop.rb b/app/models/pop.rb
index 08c7dbcff..45ccb2411 100644
--- a/app/models/pop.rb
+++ b/app/models/pop.rb
@@ -1,8 +1,4 @@
 class Pop < ApplicationRecord
   belongs_to :bubble, touch: true
   belongs_to :user, optional: true
-
-  def reason
-    super || Account::PopReasons::FALLBACK_LABEL
-  end
 end
diff --git a/app/views/bubbles/_card.html.erb b/app/views/bubbles/_card.html.erb
index a557b5a94..7c1c71c53 100644
--- a/app/views/bubbles/_card.html.erb
+++ b/app/views/bubbles/_card.html.erb
@@ -51,7 +51,7 @@
 
     <% if bubble.popped? %>
       <div class="card__popped flex flex-column">
-        <span class="card__popped-title txt-uppercase"><%= bubble.popped_by.system? ? "Closed" : bubble.pop.reason %></span>
+        <span class="card__popped-title txt-uppercase"><%= bubble.pop.reason %></span>
         <strong><%= bubble.popped_at.strftime("%b %d, %Y") %></strong>
         <span>by <span class="card__popped-by"><%= bubble.popped_by.name %></span></span>
       </div>
diff --git a/db/migrate/20250404083727_add_reason_to_pops.rb b/db/migrate/20250404083727_add_reason_to_pops.rb
index 25d2f72f9..598a36361 100644
--- a/db/migrate/20250404083727_add_reason_to_pops.rb
+++ b/db/migrate/20250404083727_add_reason_to_pops.rb
@@ -8,5 +8,11 @@ class AddReasonToPops < ActiveRecord::Migration[8.1]
 
       t.timestamps
     end
+
+    execute <<~SQL
+      update pops set reason = 'Closed'
+    SQL
+
+    change_column_null :pops, :reason, false
   end
 end
diff --git a/db/schema.rb b/db/schema.rb
index d273dee5b..4f9ef6bac 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -246,7 +246,7 @@ ActiveRecord::Schema[8.1].define(version: 2025_04_04_083727) do
   create_table "pops", force: :cascade do |t|
     t.integer "bubble_id", null: false
     t.datetime "created_at", null: false
-    t.string "reason"
+    t.string "reason", null: false
     t.datetime "updated_at", null: false
     t.integer "user_id"
     t.index ["bubble_id"], name: "index_pops_on_bubble_id", unique: true
diff --git a/db/schema_cache.yml b/db/schema_cache.yml
index 9d9673f00..39798750e 100644
--- a/db/schema_cache.yml
+++ b/db/schema_cache.yml
@@ -468,16 +468,6 @@ columns:
     default_function:
     collation:
     comment:
-  - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column
-    auto_increment:
-    name: last_active_at
-    cast_type: *3
-    sql_type_metadata: *4
-    'null': true
-    default:
-    default_function:
-    collation:
-    comment:
   - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column
     auto_increment:
     name: stage_id
@@ -509,6 +499,16 @@ columns:
     collation:
     comment:
   - *9
+  - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column
+    auto_increment:
+    name: last_active_at
+    cast_type: *3
+    sql_type_metadata: *4
+    'null': true
+    default:
+    default_function:
+    collation:
+    comment:
   buckets:
   - *5
   - &32 !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column
@@ -792,7 +792,7 @@ columns:
     name: reason
     cast_type: *7
     sql_type_metadata: *8
-    'null': true
+    'null': false
     default:
     default_function:
     collation:
@@ -1354,10 +1354,11 @@ indexes:
   bubbles:
   - !ruby/object:ActiveRecord::ConnectionAdapters::IndexDefinition
     table: bubbles
-    name: index_bubbles_on_stage_id
+    name: index_bubbles_on_last_active_at_and_status
     unique: false
     columns:
-    - stage_id
+    - last_active_at
+    - status
     lengths: {}
     orders: {}
     opclasses: {}
@@ -1370,11 +1371,10 @@ indexes:
     valid: true
   - !ruby/object:ActiveRecord::ConnectionAdapters::IndexDefinition
     table: bubbles
-    name: index_bubbles_on_last_active_at_and_status
+    name: index_bubbles_on_stage_id
     unique: false
     columns:
-    - last_active_at
-    - status
+    - stage_id
     lengths: {}
     orders: {}
     opclasses: {}
diff --git a/test/fixtures/pops.yml b/test/fixtures/pops.yml
index cbb58be3f..555e1dd8d 100644
--- a/test/fixtures/pops.yml
+++ b/test/fixtures/pops.yml
@@ -1,3 +1,4 @@
 shipping:
   bubble: shipping
   user: kevin
+  reason: Closed
diff --git a/test/models/pop_test.rb b/test/models/pop_test.rb
deleted file mode 100644
index c9b7a4cb9..000000000
--- a/test/models/pop_test.rb
+++ /dev/null
@@ -1,8 +0,0 @@
-require "test_helper"
-
-class PopTest < ActiveSupport::TestCase
-  test "default to the fallback reason if no reason is given" do
-    assert_equal "Not now", Pop.new(reason: "Not now").reason
-    assert_equal Account::PopReasons::FALLBACK_LABEL, Pop.new.reason
-  end
-end