dev/core#1661 Allow phones with types longer than 16 chars to export #17956
Conversation
|
(Standard links)
|
8a9c0eb to
5bf756a
Compare
5bf756a to
f438be5
Compare
|
test this please |
|
Test fail was not familiar but doesn't seem related CRM_Member_Form_MembershipTest::testFinancialEntiriesOnCancelledContribution |
|
@demeritcowboy you tested the version of this that wasn't passing tests - maybe you can check this? |
|
There was the case activity export. This doesn't sound familiar. I see you've also done a replacement for the case one. I can check that? |
|
@demeritcowboy yes - sorry I commented on the wrong PR |
|
@pradpnayak or @yashodha I'm wondering if either you have any time to review this - there is a preliminary cleanup PR here #17951 which has just the first commit in it (if that is merged first I will rebase this) |
| @@ -289,7 +289,7 @@ public static function &fields() { | |||
| 'phone_type_id' => [ | |||
| 'name' => 'phone_type_id', | |||
| 'type' => CRM_Utils_Type::T_INT, | |||
| 'title' => ts('Phone Type'), | |||
| 'title' => ts('Phone Type ID'), | |||
There was a problem hiding this comment.
Push-me pull-me again. I've been changing these fields to user-friendly names in the metadata (e.g. without the "ID"). That's best for form builder, search builder, and forms in general.
| @@ -148,7 +148,7 @@ | |||
| </field> | |||
| <field> | |||
| <name>phone_type_id</name> | |||
| <title>Phone Type</title> | |||
| <title>Phone Type ID</title> | |||
There was a problem hiding this comment.
So I'm a -1 on this change and would prefer to override the label in exportableFields or wherever it needs to contain ID but leave it user-friendly in the metadata.
Whichever way we go we should be consistent with all fields, and at this point most titles in the metadata do not have the "ID" appended.
There was a problem hiding this comment.
@colemanw so the fields are ID but the pseudoconstants all have names which make sense for the entity reference etc
There was a problem hiding this comment.
So for example the fields array winds up with 2 fields - Phone Type - which has a label from the option_group.label & phone_type_id which is the integer. This also manifests in qf-search builder & I assume views filters on an integer not a string when you add it in the url. We have got a few fields that are pretty confusing in search builder now as the labels got renamed so the titles are duplicated - but I think we should use the pseudoconstant titles & let the fields have the correct name
f438be5 to
0b23ab2
Compare
|
@colemanw I tried to split off the metadata part to this per ^^ but now I review what is happening there I think I'll have to change the tests twice if that merges first - if we get this merged I'll finish that one |
|
This seems consistent with the previous approach and noting that the title thing has been settled and field tittles will have ID and can be overridden in the html tittle for search purposes |
Overview
Fixes a bug whereby attempting to export aa phone type field longer than 16 characters causes a crash on export
Before
Rename a phone type id to be more than 16 characters long, do an export including 'Phone type' in the export - crash
After
It's now possible to export.
Technical Details
This builds on some cleanup - notably #17951 & #17941 are not merged at time of writing and can be rebased out of this once merged.
The handling of the phone_type field is brought into line with other pseudoconstants whereby
In order to make the former work I had to pass the phone fields through the appendPseudoConstantsToFields function (we already do this on Contribution & some other BAO).
To make the second work I had to ensure that the location-type specific fields also get entries in the
_pseudoConstantsSelectarrayComments
Replaces #16833 which failed tests