fix: nil scope values being excluded from scope filtering

[navaneethkp36]

Overview

This PR fixes a bug where nil scope values were being skipped in scope_values_from_instance, resulting in no scope filtering being applied instead of the intended IS NULL SQL condition.

Motivation

When using the scope option with columns that have nil values, the scope_values_from_instancemethod was incorrectly skipping those values due to unless value.nil? checks. This caused:

• Records with nil scope values to have no scope filtering applied (empty hash {}).
• build_scope_where_clause never receiving nil values, making its IS NULL handling unreachable.
• Reordering operations to affect all records instead of only those with matching nil scope values.

Behavior Change

• nil scope values now generate proper SQL filtering: When a scope column has nil value, the query now includes WHERE column IS NULL instead of having no scope filter at all.
• Reordering respects nil scopes: When calling _ct_reorder_siblings or _ct_reorder_childrenon a record with nil
  scope values, only records with matching nil scope values are reordered together. Previously, an empty scope hash caused all records (regardless of scope) to be reordered.
• nil scoped records are properly isolated: Records with nil scope values are now treated as their own distinct scope group, separate from records with non nil scope values.

Code Changes

• Bug Fix in lib/closure_tree/support.rb
  • Removed unless value.nil? check from Symbol scope type (line 241)
  • Removed unless value.nil? check from Array scope type (line 246)

Before:

scope_hash[scope_option] = value unless value.nil?
scope_hash[item] = value unless value.nil?

After:

scope_hash[scope_option] = value
scope_hash[item] = value

• New Tests in
  • test/closure_tree/scope_test.rb
    • test_scope_values_from_instance_with_nil_value_symbol_scope
      • Verifies nil values are included for Symbol scope
    • test_scope_values_from_instance_with_nil_value_array_scope
      • Verifies nil values are included for Array scope
    • test_ordering_with_nil_scope_values_symbol_scope
      • Verifies ordering works correctly with nil scope values (Symbol)
    • test_ordering_with_nil_scope_values_array_scope
      • Verifies ordering works correctly with nil scope values (Array)

Backward Compatibility

This is a bug fix that makes the scope option work as intended. Existing code using non nil scope values is unaffected. Code relying on the buggy behavior (nil scope = no filtering) will see different results.

[navaneethkp36]

Hi @seuros @mceachen - This PR fixes a bug in the scope filtering feature. When scope column values are nil, they were being skipped instead of generating IS NULL conditions.

Could you please take a look. Would appreciate your review. Thanks

[Copilot]

Pull request overview

This PR fixes a bug where nil scope values were incorrectly excluded from scope filtering, causing records with nil scope values to bypass scope restrictions during operations like reordering.

Changes:

• Removed nil value exclusion checks in scope_values_from_instance method
• Added comprehensive tests for nil scope values in both single and multi-column scope configurations
• Verified ordering behavior with nil scope values

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
lib/closure_tree/support.rb	Removed unless value.nil? checks to allow nil values in scope hash
test/closure_tree/scope_test.rb	Added tests for scope_values_from_instance with nil values and ordering with nil scopes

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[seuros]

The change is 👍🏼 , however the test need to test in the 3 database types.

Scoped Item is connected to PG. check the test/dummy app for the others models.

We just need to assert that it won't blow up with another engine.

[navaneethkp36]

The change is 👍🏼 , however the test need to test in the 3 database types.

Scoped Item is connected to PG. check the test/dummy app for the others models.

We just need to assert that it won't blow up with another engine.

Hi @seuros , Thanks for reviewing.

Added separate tests for each DB types - 3d71ad0

[navaneethkp36]

hi @seuros

I see that PR #477 has been merged and the CI is passing. When you have a moment, could you please re-run the CI on this PR and confirm whether it’s good to proceed?

[seuros]

Today, I need to test it from master.

Also test with advisory lock gem.

Can you do the same in your app ? We cannot ship untested code as the locking is pretty hard to dwbug.

[navaneethkp36]

Today, I need to test it from master.

Also test with advisory lock gem.

Can you do the same in your app ? We cannot ship untested code as the locking is pretty hard to dwbug.

Hi @seuros

I ran a sanity check on my app after the recent changes, and everything looked good. Since our app uses PostgreSQL, the testing was done against that database.

[seuros]

Same here. I will prepare a release.