-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
CRM-18048: keep civicrm_value table names to a minimum length to avoid report errors #11183
Conversation
Jenkins re test this please |
@agileware @jusfreeman I think this might be one you may be interested in, i think it fits in well with the work you were doing on the exporting |
I think this sounds like a good solution to trim the group name by 13 characters. In this PR #11126 we trim the alias name at the run time instead of group name, which works too but this PR solves it in right way I guess. By changes this PR should work as expected. However, have not tested this yet. |
Tested this PR & it's working as expected. |
@jmcclelland can you rename the test file to |
Thanks everyone for the feedback. I've renamed the test file as requested and am now investigating why the other report tests are failing. |
I seem to have fixed the problem - but github seems to think there is still an error. Does anyone understand what this means: "No report files were found. Configuration error?" |
Jenkins test this please |
@jmcclelland on console output I get this error You might need to squash your commits into 1. Here's the step to do that:
|
Ensure custom group names are never a length that can cause this problem. Also, update test infrastructure to prevent this problem from being masked.
144c290
to
2caf0fe
Compare
@monishdeb thank for the tip! |
And it passed 🎉 |
test this please |
The test build passed earlier, also I retested in my local again it works for me. Confirmed that the test build failure is not related. Hence merging this PR |
@jmcclelland in return can you please QA #11510 ? |
Overview
The Activity Detail report was failing because a custom group table name exceeded the length limit allowed to be used in a temporary table.
Rather than fix the report, I'm proposing we enforce a limit on the length of custom value table names.
This will fix this problem moving forward but not fix the problem for already existing custom value tables.
Before
If you run an Activity Report and select a custom field from a custom table with a table name that is too long you get a trace back.
After
The activity report works fine.
Technical Details
There are significant and questionable changes here.
First - I removed code from the test suite that truncates custom group names to 13 characters. I don't know why we had this before. And, it was masking the problem with the report. I've removed it.
Second, I've added code to truncate the "name" part of the a custom group table to just 13 characters (it was 42 characters). This limit was previously aimed at preventing table names longer than 64 characters (I presume). I'm drastically reducing it because some reports autogenerate field names in temporary tables based on a combination of a custom table name and custom field name.
Since all custom table names should end with an underscore and and id of the custom data group, we should not end up with duplicates.
Comments
Also, I refactored more test code so I can grab and run a report without generating a temp file with the CSV results.