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

[NFC] dev/mail#103 Add in unit test to demonstate issue with API… #22045

Merged
merged 1 commit into from
Nov 13, 2021

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Nov 12, 2021

Overview

This test is currently failing but does replicate the issue
See https://lab.civicrm.org/dev/mail/-/issues/103

Before

No Test

After

Test

ping @colemanw @eileenmcnaughton

@civibot
Copy link

civibot bot commented Nov 12, 2021

(Standard links)

@colemanw
Copy link
Member

I see, so the problem is that the custom group name contains an illegal character (a space).
Perhaps the fix needs to be an upgrade query such as

UPDATE civicrm_custom_group SET name = REPLACE(name, ' ', '_');

@colemanw
Copy link
Member

And/or maybe this: #22049

@colemanw
Copy link
Member

Before merging this PR I would add a comment to the test that spaces in machine names are not allowed!
If those backticks solve the problem, great, but it's still not something we want to encourage.
Also, note that you had to resort to using sql directly to get spaces in the name, because the api won't let you do it!

@seamuslee001 seamuslee001 changed the title [WIP][NFC] dev/mail#103 Add in unit test to demonstate issue with API… [NFC] dev/mail#103 Add in unit test to demonstate issue with API… Nov 12, 2021
@seamuslee001
Copy link
Contributor Author

@colemanw I have added a comment to the test now and I think this is mergeable now

@colemanw
Copy link
Member

retest this please

@colemanw
Copy link
Member

Actually @seamuslee001 there is a BaseCustomValueTest class in the APIv4 tests to help with custom field cleanup so we probably want this test to extend that class.

@seamuslee001
Copy link
Contributor Author

ok done @colemanw

…d joins

Add comment to test along the lines from Coleman and fix test cleanup

Update test to extend from BaseCustomValueTest
@eileenmcnaughton eileenmcnaughton merged commit 8b32c93 into civicrm:master Nov 13, 2021
@eileenmcnaughton eileenmcnaughton deleted the dev_mail_103 branch November 13, 2021 02:39
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