Patch #32977

Remove references to deleted user from "user"-Format CustomFields

Added by Jens Krämer over 1 year ago. Updated about 1 month ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Marius BALTEANU% Done:

0%

Category:Custom fields
Target version:5.0.0

Description

When a user record is destroyed, custom field values for custom fields with field_format == 'user' referencing the destroyed user are left unchanged.

This leads to problems with queries on such a custom field when using either the none or any operators, since these match against custom_values.value (not) being null or '' - records that have the destroyed user's ID set will not turn up in the none query, but in the any query, despite being displayed with an empty value in the UI.

The attached patch adds a test case and addresses the issue by removing custom_values records that reference the destroyed user.

0001-removes-references-to-deleted-users-in-custom-field-.patch Magnifier (2.71 KB) Jens Krämer, 2020-02-10 10:09

0001-removes-references-to-deleted-users-in-custom-field-.patch Magnifier - updated patch including migration (3.65 KB) Jens Krämer, 2020-02-11 06:29

Associated revisions

Revision 21207
Added by Marius BALTEANU about 1 month ago

Remove references to deleted user from "user"-Format CustomFields when destroying an user (#32977).

Patch by Jens Krämer.

Revision 21208
Added by Marius BALTEANU about 1 month ago

Use ids instead of pluck(:id) (#32977).

History

#1 Updated by Go MAEDA over 1 year ago

  • Target version set to Candidate for next minor release

#2 Updated by Marius BALTEANU over 1 year ago

We shouldn't add a migration to remove the existing orphaned values?

#3 Updated by Jens Krämer over 1 year ago

we should indeed do that. I'll take care of that tomorrow.

#4 Updated by Jens Krämer over 1 year ago

here's a new patch including a migration to delete already existing orphaned values.

#5 Updated by Go MAEDA over 1 year ago

  • Target version changed from Candidate for next minor release to Candidate for next major release

The patch cannot be committed for minor releases because it has a migration.

#6 Updated by Go MAEDA over 1 year ago

  • Assignee set to Jean-Philippe Lang
  • Target version changed from Candidate for next major release to 4.2.0

Setting the target version to 4.2.0.

#7 Updated by Marius BALTEANU 7 months ago

  • Target version changed from 4.2.0 to 5.0.0

#8 Updated by Marius BALTEANU 2 months ago

  • Status changed from New to Needs feedback
  • Assignee changed from Jean-Philippe Lang to Jens Krämer

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


➜  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

I think the problem is in the test because the following update:

issue.update(custom_field_values:
{
  cf1.id => @jsmith.id,
  cf2.id => [@dlopper.id, @jsmith.id]
})

generates 3 entries in CustomValue (one for each user) and we should expect 3 less custom values when the user jsmith is destroyed.

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

#9 Updated by Jens Krämer about 1 month ago

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.

#10 Updated by Marius BALTEANU about 1 month ago

  • Assignee changed from Jens Krämer to Marius BALTEANU

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.

#11 Updated by Marius BALTEANU about 1 month ago

  • Status changed from Needs feedback to New

#12 Updated by Marius BALTEANU about 1 month ago

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) 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.

#13 Updated by Marius BALTEANU about 1 month ago

  • Status changed from New to Resolved

#14 Updated by Marius BALTEANU about 1 month ago

Patch committed, thank you.

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

#15 Updated by Marius BALTEANU about 1 month ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF