Validate and normalize auto-postpone period to days (#2672)
* Validate and normalize auto-postpone period to days - Add Entropy::AUTO_POSTPONE_PERIODS and validate auto_postpone_period against the allowed set - Introduce auto_postpone_period_in_days for forms and entropy update endpoints - Fall back to index 0 in knob partial when current value isn't in options - Remove entropy_auto_close_options helper in favor of model constant - Update tests to use allowed period values * Use auto_postpone_period_in_days for JSON entropy updates The JSON API was accepting auto_postpone_period (seconds) which bypassed the days normalization and validation. Switch to auto_postpone_period_in_days consistently and add explicit wrap_parameters so flat JSON params are wrapped correctly for the virtual attribute. * Default to account or 30-day fallback when knob value is invalid * Address PR review feedback for entropy validation - Fall back to first knob option (index 0) when persisted value isn't in the allowed set, preventing nil index errors for legacy values - Use integer division (1.day.to_i) for consistent day calculations * Default to account entropy period instead of first option for knob fallback * Test that default auto-postpone period is in the allowed periods * Return 422 instead of 500 for invalid entropy auto-postpone values Rescue ActiveRecord::RecordInvalid in entropy controllers so invalid auto_postpone_period_in_days values return 422 Unprocessable Entity instead of raising a 500. * Fix NoMethodError when board entropy falls back to account default Use container.account.entropy.auto_postpone_period_in_days instead of container.account.auto_postpone_period_in_days since Account doesn't delegate that method. * Test board entropy fallback to account default for invalid periods
This commit is contained in:
@@ -1,4 +1,6 @@
|
||||
class Account::EntropiesController < ApplicationController
|
||||
wrap_parameters :entropy, include: [ :auto_postpone_period_in_days ]
|
||||
|
||||
before_action :ensure_admin
|
||||
|
||||
def update
|
||||
@@ -8,10 +10,12 @@ class Account::EntropiesController < ApplicationController
|
||||
format.html { redirect_to account_settings_path, notice: "Account updated" }
|
||||
format.json { head :no_content }
|
||||
end
|
||||
rescue ActiveRecord::RecordInvalid
|
||||
head :unprocessable_entity
|
||||
end
|
||||
|
||||
private
|
||||
def entropy_params
|
||||
params.expect(entropy: [ :auto_postpone_period ])
|
||||
params.expect(entropy: [ :auto_postpone_period_in_days ])
|
||||
end
|
||||
end
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
class Boards::EntropiesController < ApplicationController
|
||||
wrap_parameters :board
|
||||
wrap_parameters :board, include: [ :auto_postpone_period_in_days ]
|
||||
|
||||
include BoardScoped
|
||||
|
||||
@@ -12,10 +12,12 @@ class Boards::EntropiesController < ApplicationController
|
||||
format.turbo_stream
|
||||
format.json { head :no_content }
|
||||
end
|
||||
rescue ActiveRecord::RecordInvalid
|
||||
head :unprocessable_entity
|
||||
end
|
||||
|
||||
private
|
||||
def entropy_params
|
||||
params.expect(board: [ :auto_postpone_period ])
|
||||
params.expect(board: [ :auto_postpone_period_in_days ])
|
||||
end
|
||||
end
|
||||
|
||||
@@ -88,7 +88,7 @@ class BoardsController < ApplicationController
|
||||
end
|
||||
|
||||
def board_params
|
||||
params.expect(board: [ :name, :all_access, :auto_postpone_period, :public_description ])
|
||||
params.expect(board: [ :name, :all_access, :auto_postpone_period_in_days, :public_description ])
|
||||
end
|
||||
|
||||
def grantees
|
||||
|
||||
@@ -1,8 +1,4 @@
|
||||
module EntropyHelper
|
||||
def entropy_auto_close_options
|
||||
[ 3, 7, 30, 90, 365, 11 ]
|
||||
end
|
||||
|
||||
def entropy_bubble_options_for(card)
|
||||
{
|
||||
daysBeforeReminder: card.entropy.days_before_reminder,
|
||||
|
||||
@@ -6,9 +6,7 @@ module Board::AutoPostponing
|
||||
end
|
||||
|
||||
private
|
||||
DEFAULT_AUTO_POSTPONE_PERIOD = 30.days
|
||||
|
||||
def set_default_auto_postpone_period
|
||||
self.auto_postpone_period ||= DEFAULT_AUTO_POSTPONE_PERIOD unless attribute_present?(:auto_postpone_period)
|
||||
self.auto_postpone_period ||= Entropy::DEFAULT_AUTO_POSTPONE_PERIOD_IN_DAYS.days unless attribute_present?(:auto_postpone_period)
|
||||
end
|
||||
end
|
||||
|
||||
@@ -2,7 +2,7 @@ module Board::Entropic
|
||||
extend ActiveSupport::Concern
|
||||
|
||||
included do
|
||||
delegate :auto_postpone_period, to: :entropy
|
||||
delegate :auto_postpone_period, :auto_postpone_period_in_days, to: :entropy
|
||||
has_one :entropy, as: :container, dependent: :destroy
|
||||
end
|
||||
|
||||
@@ -12,6 +12,10 @@ module Board::Entropic
|
||||
|
||||
def auto_postpone_period=(new_value)
|
||||
entropy ||= association(:entropy).reader || self.build_entropy
|
||||
entropy.update auto_postpone_period: new_value
|
||||
entropy.update! auto_postpone_period: new_value
|
||||
end
|
||||
|
||||
def auto_postpone_period_in_days=(value)
|
||||
self.auto_postpone_period = value.to_i.days.to_i
|
||||
end
|
||||
end
|
||||
|
||||
@@ -1,6 +1,35 @@
|
||||
class Entropy < ApplicationRecord
|
||||
DEFAULT_AUTO_POSTPONE_PERIOD_IN_DAYS = 30
|
||||
AUTO_POSTPONE_PERIODS_IN_DAYS = [ 3, 7, 30, 90, 365, 11 ].freeze
|
||||
AUTO_POSTPONE_PERIODS_IN_SECONDS = AUTO_POSTPONE_PERIODS_IN_DAYS.map { |n| n.day.in_seconds }.freeze
|
||||
|
||||
belongs_to :account, default: -> { container.account }
|
||||
belongs_to :container, polymorphic: true
|
||||
|
||||
validates :auto_postpone_period, inclusion: { in: AUTO_POSTPONE_PERIODS_IN_SECONDS }
|
||||
|
||||
after_commit -> { container.cards.touch_all if container }
|
||||
|
||||
def auto_postpone_period_in_days
|
||||
days = auto_postpone_period / 1.day.to_i
|
||||
|
||||
if days.in?(AUTO_POSTPONE_PERIODS_IN_DAYS)
|
||||
days
|
||||
else
|
||||
default_auto_postpone_period_in_days
|
||||
end
|
||||
end
|
||||
|
||||
def auto_postpone_period_in_days=(new_value)
|
||||
self.auto_postpone_period = new_value.to_i.days.to_i
|
||||
end
|
||||
|
||||
private
|
||||
def default_auto_postpone_period_in_days
|
||||
if container.is_a?(Board)
|
||||
container.account.entropy.auto_postpone_period_in_days
|
||||
else
|
||||
DEFAULT_AUTO_POSTPONE_PERIOD_IN_DAYS
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@@ -6,8 +6,9 @@
|
||||
<%= form_with model: model, url: url, data: { controller: "form" } do |form| %>
|
||||
<%= render "entropy/knob",
|
||||
form: form,
|
||||
name: :auto_postpone_period,
|
||||
knob_options: entropy_auto_close_options,
|
||||
name: :auto_postpone_period_in_days,
|
||||
current_value: model.auto_postpone_period_in_days,
|
||||
knob_options: Entropy::AUTO_POSTPONE_PERIODS_IN_DAYS,
|
||||
label: "Days until auto-close",
|
||||
disabled: disabled %>
|
||||
<% end %>
|
||||
|
||||
@@ -1,6 +1,5 @@
|
||||
<%
|
||||
period = form.object.send(name)
|
||||
current_index = knob_options.index(period.to_i / 1.day)
|
||||
current_index = knob_options.index(current_value) || knob_options.index(Entropy::DEFAULT_AUTO_POSTPONE_PERIOD_IN_DAYS)
|
||||
%>
|
||||
|
||||
<fieldset class="knob" data-controller="knob" data-knob-target="field" style="--knob-options: <%= knob_options.length %>; --knob-index: <%= current_index %>">
|
||||
@@ -8,7 +7,7 @@
|
||||
<% knob_options.each_with_index do |value, index| %>
|
||||
<label class="knob__option" style="--i: <%= index %>">
|
||||
<%= form.radio_button name,
|
||||
value.days,
|
||||
value,
|
||||
data: {
|
||||
action: "change->knob#optionChanged change->form#submit",
|
||||
index: index,
|
||||
|
||||
@@ -6,24 +6,33 @@ class Account::EntropiesControllerTest < ActionDispatch::IntegrationTest
|
||||
end
|
||||
|
||||
test "update" do
|
||||
put account_entropy_path, params: { entropy: { auto_postpone_period: 1.day } }
|
||||
put account_entropy_path, params: { entropy: { auto_postpone_period_in_days: 7 } }
|
||||
|
||||
assert_equal 1.day, entropies("37s_account").auto_postpone_period
|
||||
assert_equal 7.days, entropies("37s_account").auto_postpone_period
|
||||
|
||||
assert_redirected_to account_settings_path
|
||||
end
|
||||
|
||||
test "update as JSON" do
|
||||
put account_entropy_path, params: { entropy: { auto_postpone_period: 2.days } }, as: :json
|
||||
put account_entropy_path, params: { entropy: { auto_postpone_period_in_days: 7 } }, as: :json
|
||||
|
||||
assert_response :no_content
|
||||
assert_equal 2.days, entropies("37s_account").reload.auto_postpone_period
|
||||
assert_equal 7.days, entropies("37s_account").reload.auto_postpone_period
|
||||
end
|
||||
|
||||
test "update requires admin" do
|
||||
logout_and_sign_in_as :david
|
||||
|
||||
put account_entropy_path, params: { entropy: { auto_postpone_period: 1.day } }
|
||||
put account_entropy_path, params: { entropy: { auto_postpone_period_in_days: 7 } }
|
||||
assert_response :forbidden
|
||||
end
|
||||
|
||||
test "update rejects invalid auto_postpone_period" do
|
||||
original_period = entropies("37s_account").auto_postpone_period
|
||||
|
||||
put account_entropy_path, params: { entropy: { auto_postpone_period_in_days: 1 } }
|
||||
|
||||
assert_response :unprocessable_entity
|
||||
assert_equal original_period, entropies("37s_account").reload.auto_postpone_period
|
||||
end
|
||||
end
|
||||
|
||||
@@ -39,17 +39,17 @@ class FlatJsonParamsTest < ActionDispatch::IntegrationTest
|
||||
test "update board entropy with flat JSON" do
|
||||
board = boards(:writebook)
|
||||
|
||||
put board_entropy_path(board), params: { auto_postpone_period: 99.days }, as: :json
|
||||
put board_entropy_path(board), params: { auto_postpone_period_in_days: 90 }, as: :json
|
||||
|
||||
assert_response :no_content
|
||||
assert_equal 99.days, board.entropy.reload.auto_postpone_period
|
||||
assert_equal 90.days, board.entropy.reload.auto_postpone_period
|
||||
end
|
||||
|
||||
test "update account entropy with flat JSON" do
|
||||
put account_entropy_path, params: { auto_postpone_period: 2.days }, as: :json
|
||||
put account_entropy_path, params: { auto_postpone_period_in_days: 7 }, as: :json
|
||||
|
||||
assert_response :no_content
|
||||
assert_equal 2.days, Current.account.entropy.reload.auto_postpone_period
|
||||
assert_equal 7.days, Current.account.entropy.reload.auto_postpone_period
|
||||
end
|
||||
|
||||
test "create push subscription with flat JSON" do
|
||||
|
||||
@@ -8,19 +8,19 @@ class Boards::EntropiesControllerTest < ActionDispatch::IntegrationTest
|
||||
|
||||
test "update" do
|
||||
assert_no_difference -> { Current.account.entropy.reload.auto_postpone_period } do
|
||||
put board_entropy_path(@board, format: :turbo_stream), params: { board: { auto_postpone_period: 123.days } }
|
||||
put board_entropy_path(@board, format: :turbo_stream), params: { board: { auto_postpone_period_in_days: 90 } }
|
||||
|
||||
assert_equal 123.days, @board.entropy.reload.auto_postpone_period
|
||||
assert_equal 90.days, @board.entropy.reload.auto_postpone_period
|
||||
|
||||
assert_turbo_stream action: :replace, target: dom_id(@board, :entropy)
|
||||
end
|
||||
end
|
||||
|
||||
test "update as JSON" do
|
||||
put board_entropy_path(@board), params: { board: { auto_postpone_period: 99.days } }, as: :json
|
||||
put board_entropy_path(@board), params: { board: { auto_postpone_period_in_days: 90 } }, as: :json
|
||||
|
||||
assert_response :no_content
|
||||
assert_equal 99.days, @board.entropy.reload.auto_postpone_period
|
||||
assert_equal 90.days, @board.entropy.reload.auto_postpone_period
|
||||
end
|
||||
|
||||
test "update requires board admin permission" do
|
||||
@@ -28,9 +28,18 @@ class Boards::EntropiesControllerTest < ActionDispatch::IntegrationTest
|
||||
|
||||
original_period = @board.entropy.auto_postpone_period
|
||||
|
||||
put board_entropy_path(@board, format: :turbo_stream), params: { board: { auto_postpone_period: 1.day } }
|
||||
put board_entropy_path(@board, format: :turbo_stream), params: { board: { auto_postpone_period_in_days: 7 } }
|
||||
|
||||
assert_response :forbidden
|
||||
assert_equal original_period, @board.entropy.reload.auto_postpone_period
|
||||
end
|
||||
|
||||
test "update rejects invalid auto_postpone_period" do
|
||||
original_period = @board.entropy.auto_postpone_period
|
||||
|
||||
put board_entropy_path(@board, format: :turbo_stream), params: { board: { auto_postpone_period_in_days: 1 } }
|
||||
|
||||
assert_response :unprocessable_entity
|
||||
assert_equal original_period, @board.entropy.reload.auto_postpone_period
|
||||
end
|
||||
end
|
||||
|
||||
@@ -41,12 +41,22 @@ class BoardsControllerTest < ActionDispatch::IntegrationTest
|
||||
assert_response :success
|
||||
end
|
||||
|
||||
test "edit renders 11-day auto-close option last on the knob" do
|
||||
get edit_board_path(boards(:writebook))
|
||||
assert_response :success
|
||||
|
||||
assert_select "input[type=radio][name='board[auto_postpone_period_in_days]']" do |options|
|
||||
assert_equal Entropy::AUTO_POSTPONE_PERIODS_IN_DAYS.map(&:to_s), options.map { |option| option["value"] }
|
||||
assert_equal "11", options.last["value"]
|
||||
end
|
||||
end
|
||||
|
||||
test "update" do
|
||||
patch board_path(boards(:writebook)), params: {
|
||||
board: {
|
||||
name: "Writebook bugs",
|
||||
all_access: false,
|
||||
auto_postpone_period: 1.day
|
||||
auto_postpone_period_in_days: 7
|
||||
},
|
||||
user_ids: users(:kevin, :jz).pluck(:id)
|
||||
}
|
||||
@@ -54,7 +64,7 @@ class BoardsControllerTest < ActionDispatch::IntegrationTest
|
||||
assert_redirected_to edit_board_path(boards(:writebook))
|
||||
assert_equal "Writebook bugs", boards(:writebook).reload.name
|
||||
assert_equal users(:kevin, :jz).sort, boards(:writebook).users.sort
|
||||
assert_equal 1.day, entropies(:writebook_board).auto_postpone_period
|
||||
assert_equal 7.days, entropies(:writebook_board).auto_postpone_period
|
||||
assert_not boards(:writebook).all_access?
|
||||
end
|
||||
|
||||
|
||||
@@ -9,17 +9,17 @@ class Card::EntropicTest < ActiveSupport::TestCase
|
||||
freeze_time
|
||||
|
||||
entropies(:writebook_board).destroy
|
||||
entropies("37s_account").reload.update! auto_postpone_period: 456.days
|
||||
entropies("37s_account").reload.update! auto_postpone_period: 365.days
|
||||
cards(:layout).update! last_active_at: 2.day.ago
|
||||
assert_equal (456 - 2).days.from_now, cards(:layout).entropy.auto_clean_at
|
||||
assert_equal (365 - 2).days.from_now, cards(:layout).entropy.auto_clean_at
|
||||
end
|
||||
|
||||
test "auto_postpone_at infers the period from the board when present" do
|
||||
freeze_time
|
||||
|
||||
entropies(:writebook_board).update! auto_postpone_period: 123.days
|
||||
entropies(:writebook_board).update! auto_postpone_period: 90.days
|
||||
cards(:layout).update! last_active_at: 2.day.ago
|
||||
assert_equal (123 - 2).days.from_now, cards(:layout).entropy.auto_clean_at
|
||||
assert_equal (90 - 2).days.from_now, cards(:layout).entropy.auto_clean_at
|
||||
end
|
||||
|
||||
test "setting auto_postpone_period in the board without entropy will create it, without affecting the account entropy" do
|
||||
@@ -27,7 +27,7 @@ class Card::EntropicTest < ActiveSupport::TestCase
|
||||
original_period = account_entropy.auto_postpone_period
|
||||
|
||||
entropies(:writebook_board).destroy
|
||||
boards(:writebook).update! auto_postpone_period: 999.days
|
||||
boards(:writebook).update! auto_postpone_period: 365.days
|
||||
|
||||
assert_equal original_period, account_entropy.reload.auto_postpone_period
|
||||
end
|
||||
|
||||
@@ -3,15 +3,26 @@ require "test_helper"
|
||||
class Entropy::Test < ActiveSupport::TestCase
|
||||
test "touch cards when entropy changes for board" do
|
||||
assert_changes -> { boards(:writebook).cards.first.updated_at } do
|
||||
boards(:writebook).entropy.update!(auto_postpone_period: 15.days)
|
||||
boards(:writebook).entropy.update!(auto_postpone_period: 7.days)
|
||||
end
|
||||
end
|
||||
|
||||
test "default auto-postpone period is included in allowed periods" do
|
||||
assert_includes Entropy::AUTO_POSTPONE_PERIODS_IN_DAYS, Entropy::DEFAULT_AUTO_POSTPONE_PERIOD_IN_DAYS
|
||||
end
|
||||
|
||||
test "board entropy falls back to account entropy period when value is invalid" do
|
||||
board = boards(:writebook)
|
||||
board.entropy.update_column(:auto_postpone_period, 999.days.to_i)
|
||||
|
||||
assert_equal Current.account.entropy.auto_postpone_period_in_days, board.entropy.auto_postpone_period_in_days
|
||||
end
|
||||
|
||||
test "touch cards when entropy changes for account container" do
|
||||
account = Current.account
|
||||
|
||||
assert_changes -> { account.cards.first.updated_at } do
|
||||
boards(:writebook).entropy.update!(auto_postpone_period: 15.days)
|
||||
boards(:writebook).entropy.update!(auto_postpone_period: 7.days)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
Reference in New Issue
Block a user