Incorrect warning about custom fields being nil (NULL or non-existant)

[dipanm]

The generate release note tab provides a warning saying "There are X issues which doesn't have a value for the release notes custom field" . This is incorrect warning.

In a specific case - i found that of a version containing 15 issues showed a notice "There are 15 issues which doesn't have a value for the notes custom fields" - however, it has all release note custom field values properly set. At the same time, it also shows There are 2 issues which still needs release notes" - obviously 15 being total issues in the version, it cann't have 2 issues with Release note cf being TODO + 15 issues with Release note being Empty/NULL or non-existent.

The warning count is wrong.

[dipanm]

OK - upon inspecting closely - I found that the method def self.release_notes_custom_value_nil has the bug. Basically, it filters all issues which satisfies the condition conditions_b = "( (custom_values.value IS NULL" conditions_b << " OR custom_values.value = '')"

This means, that it will filter all issues which has at least 1 custom field being 'NULL'. So even if the release note custom field is properly set to say TODO - but some other custom field is blank - it will show in the count of "issues which doesn't have value for the notes custom field"

The correct solution is: conditions_b = "( (custom_values.value IS NULL" conditions_b << " OR custom_values.value = '')" conditions_b << " AND custom_field_id = #{cf_id} "

It should pick up only those custom_values where custom_field_id = cf_id and custom_values.value IS NULL. not for ANY custom field.

[dipanm]

I was facing this - problem while I was working on #75 - without solving this, I am facing some issues.

So please confirm that above understanding is correct - than I will try to first fix this issue and proceed on to #75.

[hdgarrood]

You're absolutely right -- thanks for reporting this. Your solution's good, except that I'd prefer to use custom_values.custom_field_id (explicitly name the table). Plus you're missing a close paren ;)

[hdgarrood]

Actually something along the lines of where(conditions_a).union(joins_release_notes.where(conditions_b)) would probably be nicer. I'm not sure if ActiveRecord::Relations respond to #union, though.

[dipanm]

I agree about your explicit-table name and ')' - it was just typo. If you want, I can push the above mentioned solution right away. I am not sure about the union thing -if you want to go that route, I would request you to explore and fix it.

[hdgarrood]

Fair enough. I think the union way probably isn't worth the effort. Go ahead and push; I'll merge.

[dipanm]

There are too many exceptions to handle here before things become perfect : Here are total number of scenario's and how we shall classify -

_1. CF value == TODO - release_notes_todo _2. CF value == DONE - release_notes_done _3. CF value == NONE - release_notes_not_required

so far so good.

_4. CF not defined for issue ? release_notes_not_required OR release_notes_nil ? - not eligible for release note! _5. CF value NULL ? release_notes_custom_value_nil _6. CF value '' ? release_notes_custom_value_nil _7. CF value == 'RANDOM' (basically what if the list has 4th value) ? release_notes_invalid

A. I think case 4 - is likely to imply that the issue of the tracker types where release notes are not written. - So it is more appropriate to count under - "not_required" - rather than "nil" as it is right now.

B. All other cases - CF value being NULL, ' ' or 'RANDOM' all put together should be clubbed under 'release_notes_invalid' - release_notes_custom_value_nil is not worth finding explicitly.

Currently case 7 is not handled at all and results in bug.

This will simplify things better. What do you think? If this makes sense, I would include this under the pull request #80

[hdgarrood]

Yep, that all makes sense :) go ahead!