FieldVisibilityWrapper collides with the Registration Template Forms

[nlBayside]

Prerequisites

• [X] Put an X between the brackets on this line if you have done all of the following:
  • Did you perform a search at https://github.com/issues?q=is%3Aissue+user%3ASparkDevNetwork+-repo%3ARock to see if your bug or enhancement is already reported?
  • Can you reproduce the problem on a fresh install or the demo site?
  • Did you include your Rock version number and client culture setting?

Description

This just occurred for one of our church's registrations and after looking through the code, it appears there is a problem with how the FieldVisibilityWrapper works. Here is the exception's stack trace that appeared today:

> Keep in mind that although it says "Simple Donation", this block acts very similarly to Rock's internal RegistrationEntry block.

Digging through the code, it appears to me like this is a problem with this section in the FieldVisibilityWrapper.cs file

When an attribute has the same Id as the RegistrationTemplateFormField, it will always grab the attribute's field type and not even consider the actual FormField. This only happens when an attribute exists with the same Id as the RegistrationTemplateFormField, and that attribute's field type has an EditValue that conflicts with the original. This isn't often, but this shouldn't have had a collision in the first place.

For example, one of the Form Fields on our registration was a PersonField; however, there existed a Workflow attribute with a KeyValueFilter Field Type. When Rock accesses that field type's EditValue property, it will stumble onto an exception. The PersonField control doesn't have a "filter" property, so when a method like ToJson() is called on it, it will create a Null Reference Exception.

I am unconcerned with the Field Type classes and null reference exceptions. What is concerning is that The FieldVisibilityWrapper class will grab an Attribute unassociated with a RegistrationTemplateForm, and continue to use it. Not sure what fixes would solve this issue, but it would be great to see those collisions avoided in the first place. This change has been around for 16 months, but I'm not sure what is the earliest version of Rock that it appears in.

Versions

• Rock Version: [v13]
• Client Culture Setting: [us-en]

[nlBayside]

I am reopening this (https://github.com/SparkDevNetwork/Rock/issues/5181) because no work was actually done. I was asked a question, and then 19 minutes later the issue was closed. That is not sufficient time to respond if I'm not checking my email or Github with that frequency.

This issue does not just happen in a third party block. There is no difference between the third party block and Rock's core block at the lines where the exception occurs. This is a legitimate issue where the Workflow forms and Registration Template forms will collide thanks to them both using the FieldVisibilityWrapper. This is not an issue in the Obsidian blocks because they skirt around using a component like that. This is a high priority issue for us because we use many templates for events/registrations. It isn't even an editable piece of the code because it's compiled into a dll in the end.

[nlBayside]

This issue was discovered in a third party block, but that block does not differ from the core one in the lines provided. The fact that we're using a third party block is not the root of the problem. The problem originates from the FieldVisibilityWrapper class creating a collision when it accesses its GetEditValue property.

If you look closely at the first two lines after the accessor, one would notice that a potential collision occurs.

var field = GetRegistrationTemplateFormField();
var attribute = GetAttributeCache() ?? GetFormField();

if (attribute != null) {
 Some code here...
} else if (field != null && FieldVisibilityrules.IsFieldSupported( field.PersonFieldType)) {
 Some more code...
} else {
 throw new NotImplementedException()
}

If GetAttributeCache() returns null, it will check GetFormField(). If this returns an attribute from the AttributeCache, then it will execute on the first clause of the condition block, even if the actual field is from a Registration Template, it will ignore this entirely. This means that the second condition will never be hit, so long as we hit an attribute first.

[nlBayside]

I ran into another problem from the RegistrationEntry block that at first I thought would be this one, but the exception is slightly different. It still hits the ApplyFieldVisibilityRules() method, but it bugs out on an insert to a dictionary.

 at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
   at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   at Rock.Web.UI.Controls.FieldVisibilityWrapper.ApplyFieldVisibilityRules(Control parentControl) in C:\Github\Rock\Rock\Web\UI\Controls\FieldVisibilityWrapper.cs:line 250
   at RockWeb.Blocks.Event.RegistrationEntry.CreateRegistrantControls(Boolean setValues)
   at RockWeb.Blocks.Event.RegistrationEntry.SetPanel(PanelIndex currentPanelIndex)
   at RockWeb.Blocks.Event.RegistrationEntry.ShowRegistrant()
   at RockWeb.Blocks.Event.RegistrationEntry.btnRegistrationAttributesStartNext_Click(Object sender, EventArgs e)
   at RockWeb.Blocks.Event.RegistrationEntry.ShowRegistrationAttributesStart(Boolean forward)
   at System.Web.UI.WebControls.LinkButton.OnClick(EventArgs e)
   at System.Web.UI.WebControls.LinkButton.RaisePostBackEvent(String eventArgument)
   at System.Web.UI.Page.ProcessRequestMain(Boolean includeStagesBeforeAsyncPoint, Boolean includeStagesAfterAsyncPoint)

Looking at the ApplyFieldVisibilityRules() method, I noticed that GetRegistrationTemplateFormField() and GetFormField() are both executed and use to find fieldAttributeId. I believe this is because they are getting coalesced into each other when an Attribute exists with the same Id as a RegistrationTemplateFormField.

In my exception, we had a RegistrationTemplateFormField with a Person Field Type(Gender) and another FormField that used a Person Attribute(T-Shirt size). The Gender question had an Id of 7426 with no AttributeId, and the T-Shirt size question had an Id of 7432 with an AttributeId of 7426.

The field variable will find the RTFF from cache, and the fieldAttribute variable will find the Attribute from cache. Both of these will be using the FormFieldId in their search. So even though Gender doesn't have an AttributeId, fieldAttributeId will get its value from the T-Shirt Size's Attribute Id (since it's coalesced). This then passes the if condition (fieldAttributeId.HasValue) and adds it to the attributeValues Dictionary. Once the for loop reaches the actual T-Shirt size attribute, it will think it's already been added to the dictionary.

Here is the section of ApplyFieldVisibilityRules that I think the error occurs at.

Here is a screenshot of the RegistrationTemplateFormFields that we were used. As you can see, the AttributeId matches one of the Ids for a RegistrationTemplateFormField.

I know this exception was different than the previous one; however, it originates from the same class (FieldVisibilityWrapper.cs) and occurs when coalesced variables collide. I wonder if a Boolean property could be added to the class, something like "IsRegistrationFormField" that would let the code know whether it should coalesce the values. The class is only used in 3 blocks so it wouldn't be a massive change, but it could still end up not being that simple. I hope this information was helpful! I can try to provide more if needed.

[nlBayside]

I saw this issue had a Pull Request accepted about two weeks ago (https://github.com/SparkDevNetwork/Rock/issues/5181) and I left some concerns regarding this fix's implementation. I wasn't sure if the Rock Demo site had these changes, but I was able to go the Rock Latest site to verify my concerns.

Steps to recreate

1. Create a Workflow and give it three Workflow attributes (2 Text and 1 Boolean).
2. Create a Form for the first Workflow action.
3. Allow the first text attribute to be visible always, but make the second text attribute conditionally visible dependent on the value of the boolean attribute.
4. Save Workflow changes.
5. Run Workflow.

Screenshots

Expected Results

The user should expect to see only two questions at the start of the form, but when they satisfy the conditions for the conditional Attribute then it should become visible as well.

This change doesn't occur in previous versions prior to the Pull Request:

Actual Results

The user sees all three questions in the Form without care for conditional visibility rules.

Possible Fixes

I think a simple change that could solve this problem is adding a boolean attribute to the FieldVisibilityWrapper class. It's possible to keep the previously used methods if the code is able to detect which path to take with a boolean condition.

Versions

• Rock Versions: [v14.1, v15.0]
• Client Culture Setting: [en-us]

[nlBayside]

This also breaks for any conditional fields in Workflow Type's created using the FormBuilder block.

[nlBayside]

Fixed in https://github.com/SparkDevNetwork/Rock/commit/0eea46a442ca6cec718e6e02be44a7c29938cf7f. I will close this issue. Thanks for the fix!