--------------------------------------------------------------------------------
We shouldn't add a migration to remove the existing orphaned values?
--------------------------------------------------------------------------------
we should indeed do that. I'll take care of that tomorrow.
--------------------------------------------------------------------------------
here's a new patch including a migration to delete already existing orphaned values.

--------------------------------------------------------------------------------
The patch cannot be committed for minor releases because it has a migration.
--------------------------------------------------------------------------------
Setting the target version to 4.2.0.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Jens, I've reviewed the patch and it looks good to me, except the test that fails:

<pre><code>
➜ redmine git:(patch/32977) ✗ ruby test/unit/user_test.rb -n test_remove_custom_field_references_upon_destroy
Skipping LDAP tests.
Run options: -n test_remove_custom_field_references_upon_destroy --seed 57996

# Running:

F

Failure:
UserTest#test_remove_custom_field_references_upon_destroy [test/unit/user_test.rb:1341]:
#<Proc:0x00007fcf93fa35d8@test/unit/user_test.rb:1341 (lambda)> didn't change by -2.
Expected: 20
Actual: 19
</code></pre>

I think the problem is in the test because the following update:
<pre><code class="ruby">
issue.update(custom_field_values:
{
cf1.id => @jsmith.id,
cf2.id => [@dlopper.id, @jsmith.id]
})
</code></pre>
generates 3 entries in @CustomValue@ (one for each user) and we should expect 3 less custom values when the user jsmith is destroyed.

<pre><code class="diff">
diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb
index e2dec070c..42011f759 100644
--- a/test/unit/user_test.rb
+++ b/test/unit/user_test.rb
@@ -1336,7 +1336,7 @@ class UserTest < ActiveSupport::TestCase
assert cv2a = cv2.detect{|cv| cv.value == @dlopper.id.to_s}
assert cv2b = cv2.detect{|cv| cv.value == @jsmith.id.to_s}

- assert_difference ->{CustomValue.count}, -2 do
+ assert_difference ->{CustomValue.count}, -3 do
@jsmith.destroy
end
</code></pre>

--------------------------------------------------------------------------------
I just tried and it passes here (MySQL 5.6, Ruby 2.6).

Since we delete @jsmith@ (but not @dlopper@), the custom value for @dlopper should not be affected. I have no clue why the test fails for you though.
--------------------------------------------------------------------------------
Jens Krämer wrote:
> I just tried and it passes here (MySQL 5.6, Ruby 2.6).
>
> Since we delete @jsmith@ (but not @dlopper@), the custom value for @dlopper should not be affected. I have no clue why the test fails for you though.

Thanks, you're right, user @dlopper should not be affected, I will take again a look. It doesn't fail only on my local environment, it fails also on the "custom Gitlab CI":https://gitlab.com/redmine-org/redmine/-/jobs/1468533588#L1668.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Jens Krämer wrote:
>
> Since we delete @jsmith@ (but not @dlopper@), the custom value for @dlopper should not be affected. I have no clue why the test fails for you though.

The number of @CustomValue@ decreases with 3 not because of those 3 values that are added during the tests (as I wrongly said first time), but because @jsmith user has also a CustomValue of type User ("CustomValue#3":https://github.com/redmine/redmine/blob/master/test/fixtures/custom_values.yml#L26) which is deleted as well when user is destroyed.

Jens, it should fail also on your environment if you use the default Redmine fixtures.

I'm going to change the user in the tests and I will commit this fix.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Patch committed, thank you.

I've updated the tests to assert -3 custom values and I added a comment.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------