-
-
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-18251 - Domain stats and VersionCheck #8525
Conversation
twomice
commented
Jun 7, 2016
•
edited
Loading
edited
- CRM-18251: Pingback improvements
Jenikins retest this please. |
@nganivet Can you review this please? Thanks! |
$this->stats['domain_country_iso'] = $country_result['iso_code']; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we explicitly:
} else {
$this->stats['domain_country_iso'] = NULL; (or '';)
}
??
Heh Allen as Release Manager this month, I'm trying to recruit people to help pare down the backlog of almost 100 PRs, some going back to last summer. I'm wondering if you would be able to help QA another PR if I got someone to QA this PR? |
Yes, sure. But note that this PR is not ready, per Nicola's review comment above, so we don't want to review it yet, not until tomorrow when I should have this fixed. |
Jenikins retest this please. |
jenkins, test this please |
2 similar comments
jenkins, test this please |
jenkins, test this please |
@twomice: can you send a test ping from one of your instances and I will check that all params are received? I have just checked in the stats DB and there does not seem to be any values for either the country ISO or the Delivered entity. |
@nganivet OK, I've done that now. This site reported domain_isoCode = 'AF'. |
Jenins retest this please |
Just a quick note that we are nearly to the end of review for this cycle - looks like it's almost in but you probably need to merge it tomorrow to make 4.7.10. But if you 'just miss it' I think it's Ok to merge when done rather than wait for next review week as review is actively happening here |
I'm setting the JIRA to 'Unscheduled' - please set the fix version when this is ready for review |
@nganivet I think this is ready for review; can you comment? |
Alan - we need to test this before it is released. Please tell me a time On Mon, Jul 25, 2016 at 6:42 PM, twomice notifications@github.com wrote:
Nicolas Ganivet | Principal | nicolas@cividesk.com |
I did that back on June 19 (see #8525 (comment)). Perhaps you can check that? @nganivet |
I looked at the Sprint and the tests did not make it to the pingback database. not sure if this is client side or server side though. I have asked @twomice to dump the data sent client-side so I can have a test data set to check on the server-side side. |
Thanks for the reminder @nganivet . I've run the version_check scheduled job (just now, Oct 1, 2016, 8:12pm Central time) on my dev site, and here's what it should have sent (note "domain_isoCode=TM"):
This appears to have reached the remote server properly, as it returned this JSON string: |
done! |
It's great to see this get in, finally. Thanks Eileen and Nicolas! |