dev/event#66: allow duplicate price field labels

[twomice]

Overview

Reference ticket: https://lab.civicrm.org/dev/event/-/issues/66

Community discussion in that ticket indicates it would be desirable to allow two price fields to have the same label, which is currently prevented by CiviCRM. This PR removes that restriction, allowing two price fields to have the same label.

Before

If a price field labeled "Test" already exists, user will receive a form validation error when trying to create a second price field in that price set with label "Test".

After

If a price field labeled "Test" already exists, user will NOT receive a form validation error when trying to create a second price field in that price set with label "Test"; instead, the price field is simply created.

Technical Details

This PR simply removes the form validation rule that was enforcing unique field labels. In the OP ticket, someone mentioned that other civicrm entities append an integer to the name column value where name is not unique, but no attempt was made to emulate that behavior. (In a quick scan of the code and some trial-and-error, I was unable to find any entities that do that, and I'm convinced, along with other commenters in that ticket, that failure to enforce a unique name column is not going to cause problems, as there are many entities which do not enforce that requirement.)

Comments

Thanks to DaveD for pointing out the code block referenced in this PR.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[Stoob]

Thanks @twomice :-)

[colemanw]

I see there's general support for this change in the community. I don't love the idea of duplicate names in a table, but I note that the deleted code wasn't actually preventing that (not reliably anyway), so it's not a blocker.

[eileenmcnaughton]

I agree label shouldn't be unique but I think having it be unique is making name pseudo-unique and there are all sorts of ways we expect name to be unique

I think I feel like we are setting ourselves up for problems down the track if we don't enforce name to be unique now - in future we have talked about the possibility of being able to mix and match price fields from different price sets - so even unique within a price set will still be tricky later on I think

[twomice]

Hi @eileenmcnaughton In "writing" this PR I did think at first that I'd like to emulate some other entities that are (as @colemanw pointed out in the ticket) appending an integer to non-unique name values. I agree, that seems wise and could save us from the kind of headache you're describing. Unfortunately my light research didn't turn up any such examples to emulate, and I didn't think it wise to just "think up" a new way to do that. If you or @colemanw point me to some entities that are doing that, I'd be glad to try and emulate that approach here.

[colemanw]

I don't think there's a good, tested, generic function to do that. But there should be. Here's an example of an ad-hoc solution in a single BAO. Let me see if I can turn it into something reusable.

civicrm-core/CRM/Contact/BAO/SavedSearch.php

Lines 345 to 357 in ddc4b6b

	// Auto-create unique name from label if supplied
	if (empty($params['id']) && empty($params['name']) && !empty($params['label'])) {
	$name = CRM_Utils_String::munge($params['label']);
	$existing = Civi\Api4\SavedSearch::get(FALSE)
	->addWhere('name', 'LIKE', $name . '%')
	->addSelect('name')
	->execute()->column('name');
	$suffix = '';
	while (in_array($name . $suffix, $existing)) {
	$suffix = '_' . (1 + str_replace('_', '', $suffix));
	}
	$params['name'] = $name . $suffix;
	}

[colemanw]

OK @twomice I've refactored the above into a centralized function that's smart enough to use db indices to consider what combination of fields constitutes a "unique" name: #22570

Unfortunately that index doesn't exist (yet) on the entity. IMO the correctest fix is to:

1. Add an update script which will fix any non-unique names in the table (appending _1, _2 etc.)
2. Add the name + price_set_id (edit: name only) unique index (maybe definitely also drop the existing non-unique index).

Since it already uses writeRecords(), those 2 steps would automatically opt-in the entity to the new behavior once #22570 is merged, and then we'd have guaranteed unique names.

[eileenmcnaughton]

@colemanw I think we should make name unique rather than name + price set - it doesn't matter for the UI because label can be non-unique - but I don't think it is a good assumption that the price fields will always be tightly coupled to the price fields. Preliminary discussions on afform payments have assumed we might mix in fields from more than one price set (some underlying work would need to be done)

[twomice]

I'm also inclined to think that name should be unique across all price fields, regardless of price set association, just because I expect name to be a single unique identifier for an entity. That's not a very technical reason; rather it just fits my expectations.

[colemanw]

[demeritcowboy]

Would like to make a comment:

1. There is name when 3rd party code is creating the price set for its own programmatic reference. Here it would pick its name.
2. There is name for civi's programmatic use when it has some semantic meaning, like the hidden magic price set with name default_contribution_amount. This is hardcoded in the install.
3. For all other purposes the name seems irrelevant, because civi internally is usually using id to reference those. Except for as in 2 above, I'm not aware of anywhere it uses name, and I don't think it should. (There are civicase xml files but that's an unusual situation - when the xml lives in the db the name is irrelevant.)

So what I'm getting at is in situation 3 name could be any random string and doesn't have to be related to label, and in fact that would help with confusion of name vs label, and make it easier to make it unique.

[colemanw]

@demeritcowboy I appreciate that name can be anything, but I don't see a strong reason to break our pattern of deriving name from label using CRM_Utils_String::munge(), since that's what most entities do and what most code expects, that's the easiest path forward for me to write a generic handler for this.

[demeritcowboy]

Ok. Just wanted to make the comment about what name actually is. (Fun fact: munge() returns a random string for non-latin input.)

[eileenmcnaughton]

I just merged the other - is this now mergeable @colemanw ? I think it will enforce a unique name - although I think we should have riffed on the above commentary & used random star trek terms as our new naming convention

[colemanw]

[eileenmcnaughton]

Make it sew.....

[colemanw]

So yes, this is mergeable (merged in fact) but it's not actually going to enforce a unique name because there's no unique index on that column. To fully sew up this issue, we'd need to:

1. Add an update script which will fix any non-unique names in the table (appending _1, _2 etc.)
2. Make the current index on name a unique index.

[eileenmcnaughton]

@colemanw did ^^ get done ?

I guess if not we might have a window where people are making it sorta worse but the update script would still address