View Issue Details

IDProjectCategoryView StatusLast Update
0022408mantisbtcustom fieldspublic2021-01-05 18:59
Reporterjensberke Assigned Todregad  
PrioritynormalSeverityminorReproducibilityalways
Status assignedResolutionopen 
Product Version1.3.5 
Target Version2.26.0 
Summary0022408: Custom field's value logged as changed in history, when it wasn't changed
Description

If a custom field of type "multiselection list" in an issue already has a value and you change another value in the ticket, the custom field's value also gets logged in the issue history though it hasn't changed.

Steps To Reproduce
  1. Assume a custom field of type "multiseletion list" with "possible values" set to "not relevant|relevant", and that this field is assigned to a project.
  2. Create an issue and set this custom field to "not relevant".
  3. Change the issue by editing any other field value but leave the custom field unchanged.
  4. Though the custom field hasn't changed, the issue history shows an entry for it with the custom field's name in column "field" and the value as "not relevant => not relevant" in column "change"
TagsNo tags attached.

Relationships

has duplicate 0022421 closedatrol On Issue Update, history always lists the multiselection custom fields though they have not been updated 
has duplicate 0023756 closedatrol Multiselection list field in Issue History Always Shows New Value Instead of Old Value 
has duplicate 0026750 closedatrol multilist and checkbox customfields are listed in history without change 
related to 0022464 assigneddregad Loose type comparison can prevent custom field update 

Activities

jensberke

jensberke

2017-02-22 07:35

reporter   ~0055738

I just checked Mantis 2.1.0 and it happens there as well.

atrol

atrol

2017-02-22 08:14

developer   ~0055741

Last edited: 2017-02-22 08:14

I just checked Mantis 2.1.0 and it happens there as well.

Right.
The same applies to check box fields

The problem is caused by the function that get executed when storing/restoring the values to/from database
'#function_value_to_database' => 'cfdef_prepare_list_value_to_database', '#function_database_to_value' => 'cfdef_prepare_list_database_to_value',

No time to have a deeper look at the moment.

dregad

dregad

2017-02-22 10:50

developer   ~0055742

The root cause is that bug_update.php does not compare old vs new value when building the list of custom fields to update [1].

The problem can be fixed there or in custom field API [2], by adding a comparison between old and new values; the latter is less efficient (we can avoid calling custom_field_set_value() completely, since we already know the values), but would ensure consistent behavior from SOAP API, so maybe we should do both.

[1] https://github.com/mantisbt/mantisbt/blob/release-2.1.0/bug_update.php#L307-L308
[2] https://github.com/mantisbt/mantisbt/blob/release-2.1.0/core/custom_field_api.php#L1308

dregad

dregad

2017-02-22 11:05

developer   ~0055743

PR https://github.com/mantisbt/mantisbt/pull/1035

atrol

atrol

2017-02-22 11:21

developer   ~0055744

Last edited: 2017-02-22 11:49

@dregad, not sure if you are aware that it works for other custom field types because there is a comparison of old and new value in function history_log_event_direct [1]

The comparison fails in this case as we store the values by adding | at the begin and the end)

function cfdef_prepare_list_value_to_database( $p_value ) {
    if( '' == $p_value ) {
        return '';
    } else {
        return '|' . $p_value . '|';
    }
}

but compare after removing the |

function cfdef_prepare_list_database_to_value( $p_value ) {
    return rtrim( ltrim( $p_value, '|' ), '|' );
}

[1] https://github.com/mantisbt/mantisbt/blob/release-2.1.0/core/history_api.php#L78

EDIT (dregad) use Markdown `` instead of for proper formatting of the code

dregad

dregad

2017-02-22 11:38

developer   ~0055746

@atrol I did not realize that, thanks for pointing me to it. I'll have a look at the logic there.

That being said, I think it makes sense to merge the proposed change anyway, as there is no point in executing an UPDATE query when it's not needed.

dregad

dregad

2017-02-22 11:43

developer   ~0055747

I think the logic in the call to history_log_event_direct() in custom_field_set_value() is wrong.

  1. $t_value contains the new field value, in DB format
  2. old field value is converted to display value format

So history_log_event_direct() is effectively comparing apples with oranges.

dregad

dregad

2017-02-23 05:48

developer   ~0055761

@atrol your feedback on 0022408:0055747 would be appreciated. Do you agree with my analysis ?

atrol

atrol

2017-02-23 06:13

developer   ~0055763

I don't have time to have a deeper look and for tests.
So you think that call of history_log_event_direct has to be changed in function custom_field_set_value to

        history_log_event_direct( $p_bug_id, $t_name, $t_row[$t_value_field], $t_value );
dregad

dregad

2017-02-23 06:18

developer   ~0055764

I didn't test either, but yes this is what I think

dregad

dregad

2017-02-24 09:25

developer   ~0055776

@vboctor, what's your take on 0022408:0055747 ?

mantisiator

mantisiator

2017-02-26 11:30

reporter   ~0055785

Hi,
This fix is OK for me further to testing in version 2.0.0.
Thanks a lot !

MarcoW

MarcoW

2017-03-02 06:55

reporter   ~0055863

Hi,

i think the correct fix is in the function custom_field_set_value :

history_log_event_direct( $c_bug_id, $t_name, custom_field_database_to_value( $row['value'], $t_type ), $p_value );

So was this line in Mantis 1.2.10. So instead of $t_value it has to be $p_value, which is the new not formatted value.

vboctor

vboctor

2017-04-01 17:22

manager   ~0056315

@dregad seems reasonable since it seems to fix the issue :)

sandyj

sandyj

2018-11-12 20:01

reporter   ~0060948

Hi! Any update on this issue? from the above history it looks like the PR is there but we're still seeing this issue so are there plans to roll it out?

atrol

atrol

2018-11-13 02:12

developer   ~0060949

@sandyj there is a good reason that the PR is not merged, see my comment

jingshaochen

jingshaochen

2019-05-19 16:24

reporter   ~0062097

In file custom_field_api.php, line 1352:

history_log_event_direct( $p_bug_id, $t_name, custom_field_database_to_value( $t_row[$t_value_field], $t_type ), $t_value );

can we call a db_affected_rows before it to see if a history is needed to be inserted?

jingshaochen

jingshaochen

2019-05-19 16:57

reporter   ~0062099

created a pull request: https://github.com/mantisbt/mantisbt/pull/1514

chadmiss

chadmiss

2020-03-03 05:16

reporter   ~0063728

still happening in 2.23.0