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

Address display of tags #919

Merged
merged 1 commit into from
Oct 29, 2023
Merged

Conversation

demeritcowboy
Copy link
Collaborator

Overview

This should address the test fail but I'm not sure yet what api3 will return for return => tag lower down. Let's find out.

@petednz
Copy link

petednz commented Oct 19, 2023

I clicked the link to see what failings were happening with Addresses - lol.

@demeritcowboy
Copy link
Collaborator Author

Ha made you look.

But good point I should update the title.

@demeritcowboy demeritcowboy changed the title Address test fail Address test fail with tags Oct 19, 2023
@KarinG
Copy link
Collaborator

KarinG commented Oct 19, 2023

😂 Dave is looking at Major Donor now being Major_Donor in dev-master. Investing in automated tests has been the best thing ever.

@demeritcowboy
Copy link
Collaborator Author

It's currently failing because civicrm_api3('contact', 'get', 'return' => ['tag']) will return the name for the tag. I'm going to argue that this is "correct", simply because there's no robust way to get from label to name if instead it returned label, but you can always get from name to label. In the context of this test, it's using it for comparison/validation, so name is appropriate. I can see an argument that some people might be using such a call for display purposes and so now that same call will display the internal name, but it has to be one or the other, so it's going to be "wrong" for half of the people either way.

@KarinG
Copy link
Collaborator

KarinG commented Oct 19, 2023

Funny the branch is major-donor (perhaps another variation we need to account for) :-)

@demeritcowboy demeritcowboy changed the title Address test fail with tags Address display of tags Oct 24, 2023
@KarinG
Copy link
Collaborator

KarinG commented Oct 27, 2023

@jitendrapurohit - I've reviewed and tested. Since it's not trivial - could you please have a look this weekend as well? If you concur - then go ahead and hit the Merge button!

@jitendrapurohit jitendrapurohit merged commit b4dd3d9 into colemanw:6.x Oct 29, 2023
5 checks passed
@jitendrapurohit
Copy link
Collaborator

ah, maybe we missed the tagset. Follow up PR - #921

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants