Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export fix on long custom fields #18146

Merged
merged 1 commit into from
Sep 9, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 14, 2020

Overview

Fixes a code miss whereby improved metadata meant the code intended to handle custom
fields was not being hit, adds tests for fixed variant of a long option value

Before

Handling for 'data_type' parameter in the export class is also missed because it was based on the assumption that 'type' was not set. However, quite some time back we standardised custom fields to load the correct type in their metadata.

After

Metadata fixed, resulting in custom fields with longer option values exporting - per test

Technical Details

Needs a rebase once #18114 and #18147 are merged

Comments

@mlutfy

@civibot
Copy link

civibot bot commented Aug 14, 2020

(Standard links)

@civibot civibot bot added the master label Aug 14, 2020
@eileenmcnaughton eileenmcnaughton force-pushed the export_custom branch 2 times, most recently from 40ec956 to 0160ad0 Compare August 14, 2020 23:12
@eileenmcnaughton
Copy link
Contributor Author

eileenmcnaughton commented Aug 15, 2020

I'll wait for #18114 to merge before fixing these tests

@demeritcowboy
Copy link
Contributor

I started looking at this but got confused. For one, I was able to export long text ok without the PR, but then realized that it's different for select than other types. So this is about select. But then also, the value it stores is the value, like the number 1, but it exports the label, so it will let you have a field defined with length 255 (or less) with option choices with labels of length 512, and then export still fails even with the PR.

But, if you do have a longer field defined, then the PR makes it better and it will export it.

I was also confused about the current_employer part, but it seems to export it ok.

And there's other items in here like tags and participant fields that I wasn't sure if they were related, and I didn't look at those.

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy thanks for looking - this builds on #18147 which is the source of the other changes - basically it switches to using less guess work to get field lengths. I have been focussed on that with a plan to later rebase this. I think this kicks in when the label on your option value (or combined labels) is super long

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I think the trick to reproduce is to have a select field of type number with long labels but perhaps with @mlutfy being back now....

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Sep 3, 2020

I can reproduce it, and the patch helps the problem, it just doesn't fully fix it. You still get the error if the custom field is defined to have size e.g. 255, but the option choice label is big and the value is short.

For the other stuff I wasn't aware of the duplication with 18147.

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I'm happy with 'helps' as a criteria as it covers the bug report & the PR & fixes the fact the code intended to address this is being bypassed

@demeritcowboy
Copy link
Contributor

Ok - it helps.

Fixes a code miss wereby improved metadata meant the code intended to handle custom
fields was not being hit, adds tests for fixed variant of a long option value
@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy I've rebases so what this actually does is visible now - basically moves handling that was written for the custom fields to where they actually hit it

@seamuslee001 I think @demeritcowboy's testing from earlier is still valid

@demeritcowboy
Copy link
Contributor

I retested with the rebased patch and yes it seems the same as before - it makes life better just not a complete fix.

civicrm_custom_field.text_length > length(option_value.label) ==> OK

civicrm_custom_field.text_length > length(option_value.value) && civicrm_custom_field.text_length < length(option_value.label) ==> NOT OK (e.g. field is varchar(16) but label has 17 chars)

@eileenmcnaughton
Copy link
Contributor Author

@mlutfy are you OK to merge based on the above - ie it partially tackles the issue (the use case from the issue) & fixes the code so at least the right lines are being hit

We'd need to extend the test further for the one @demeritcowboy hit

@seamuslee001 seamuslee001 merged commit 620928b into civicrm:master Sep 9, 2020
@seamuslee001 seamuslee001 deleted the export_custom branch September 9, 2020 20:49
kenwest added a commit to kenwest/cbf-drupal that referenced this pull request Oct 6, 2020
…rted to users as labels.

The issue is described at https://lab.civicrm.org/dev/core/-/issues/877
A partial fix is at civicrm/civicrm-core#18146
This fix returns a varchar(255) even if 'maxlength' is set.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants