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] Update testGetSiteStats to match earlier name change #25150

Merged
merged 1 commit into from
Dec 11, 2022

Conversation

demeritcowboy
Copy link
Contributor

Overview

Similar to https://lab.civicrm.org/dev/core/-/issues/3057#note_69324, if the expected entity name has changed this test will fail in betas and releases, but not in alpha versions.

Before

test fail

After

test not fail

Technical Details

Entity short name was changed recently in e45a8e7

Comments

@civibot
Copy link

civibot bot commented Dec 10, 2022

(Standard links)

@civibot civibot bot added the 5.57 label Dec 10, 2022
@seamuslee001
Copy link
Contributor

This seems fine to me

@seamuslee001 seamuslee001 merged commit 1c3a496 into civicrm:5.57 Dec 11, 2022
@demeritcowboy demeritcowboy deleted the sitestats branch December 11, 2022 01:12
@totten
Copy link
Member

totten commented Dec 13, 2022

Do I understand correctly that this sort of locks-in the rename as part of the wire-protocol and kicks the problem over to backend? So reports like this would need to be revised as well:

https://lab.civicrm.org/infra/stats-collection/-/blob/4a7b8616c044b76f211e3b812d05fec89d397fea/bin/pingback/getdata.php#L30

I would guess:

-   LEFT JOIN " . DBPING . ".entities e5 ON e5.stat_id = s.id AND e5.name = 'Delivered'
+   LEFT JOIN " . DBPING . ".entities e5 ON e5.stat_id = s.id AND e5.name IN ('Delivered', 'MailingEventDelivered')

Not sure who the best person is to look at the repo - maybe @bgm knows who?

@demeritcowboy
Copy link
Contributor Author

I saw your comment in chat but was more interested in fixing the issue and I don't know who even has access to these stats. And to be honest if they aren't public it enters into contentious territory.

@totten
Copy link
Member

totten commented Dec 13, 2022

Yeah, I suppose you sometimes see news stories where stats-reporting becomes contentious if it's forced on folks -- but in Civi, this report is optional. Moreover, as I understand, the key things are public (github.com / lab.c.o / stats.c.o). If there is some general issue with arrangement, then it should probably go on lab.c.o.

AFAICS, the test here isn't about that. It merely says, "When stat reports are generated, they should behave consistently/predictably."

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

Successfully merging this pull request may close these issues.

3 participants