Control tags are not appearing in account information section #376

[grana-equinix]

Issue no #376

Before this fix, only users' tags were showing

After this fix, both control and users' tags are showing

Recording.4.mp4

[NarothamSai]

I understand here we are reverting back to before of Rubucop update to get all tags as before.

but object_type never will be falsey and tag_definition.is_control_tag is never taken into consideration or will it?
object_type || tag_definition.is_control_tag
what is the significance of is_control_tag here? or can we remove it from OR condition?

[NarothamSai]

• even if tag_definition.is_control_tag is considered (assume object_type is nil or false)
• is_control_tag is a boolean value
• tag_definition.applicable_object_types.include?(tag_definition.is_control_tag) will be always false
• as per documention, See ObjectType for a list of the possible values.

so is tag_definition.is_control_tag condition required?

[pierre]

Yup, you have a good point.

Actually, before, it was (see https://github.com/killbill/killbill-admin-ui/pull/135/files):

!tag_definition.applicable_object_types.include? object_type || tag_definition.is_control_tag

which I'm guessing is/was equivalent to:

(!tag_definition.applicable_object_types.include?(object_type)) || (tag_definition.is_control_tag)

which makes more sense: return all tag definitions for object_type by fetching all tag definitions, excluding (delete_if) both the ones which don't apply to that object_type and the control tags.

But it sounds like we want: return all tag definitions for object_type by fetching all tag definitions, excluding (delete_if) the ones which don't apply to that object_type UNLESS it's a control tag?

Could we add a few unit tests to make it clearer? Has precedence of operators in Ruby somehow changed in 3.0? 🤔 (we bumped Ruby compatibility version in master - have you tried on various Ruby versions?).

[grana-equinix]

Good suggestion, here is the test case added to check if this !tag_definition.applicable_object_types.include?(object_type || tag_definition.is_control_tag) should alway return all the tags uesrs' or control tags as expected.

[pierre]

But to @NarothamSai's point, object_type || tag_definition.is_control_tag doesn't make sense?

[grana-equinix]

But to @NarothamSai's point, object_type || tag_definition.is_control_tag doesn't make sense?

Do you have any other suggestion to replace this logic with what? As per what i understood reverted the code as it was earlier because it was not intentional but due to rubocope update?

We have also added a test case to make sure that whatever logic is written here should not break tag_definition functionality and should keep returning tag's list.

[grana-equinix]

We have removed the check of tag_definition.is_control_tag form above logic and added some test case to make sure it should work as expected.

[pierre]

Does this list only contain a single element (WRITTEN_OFF)? If so, let's write the test to check this (and check the name of that single tag definition), instead of doing a for loop (a bit confusing as tag definition names should be unique);

[grana-equinix]

Done