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

Enhancement/3012 analytics account summaries #3081

Merged
merged 8 commits into from
Apr 15, 2021

Conversation

eugene-manuilov
Copy link
Collaborator

@eugene-manuilov eugene-manuilov commented Apr 2, 2021

Summary

Addresses issue #3012

Relevant technical choices

Checklist

  • My code is tested and passes existing unit tests.
  • My code has an appropriate set of unit tests which all pass.
  • My code is backward-compatible with WordPress 4.7 and PHP 5.6.
  • My code follows the WordPress coding standards.
  • My code has proper inline documentation.
  • I have added a QA Brief on the issue linked above.
  • I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

@google-cla google-cla bot added the cla: yes label Apr 2, 2021
@felixarntz
Copy link
Member

@eugene-manuilov Please keep in mind to connect PRs to their respective issues in the future (via ZenHub UI) :)

@eugene-manuilov
Copy link
Collaborator Author

@felixarntz it doesn't work anymore. It stopped working last week :(

@felixarntz
Copy link
Member

Hmm weird, it does for me. You don't see the UI at all or how is it not working?

@eugene-manuilov
Copy link
Collaborator Author

@felixarntz this is what I see when I try to connect this PR with the issue:

github com_google_site-kit-wp_pull_3081

@eugene-manuilov
Copy link
Collaborator Author

Please, ignore the wrong issue number on my screenshot. It behaves the same when I try to connect a PR with a correct issue.

Copy link
Collaborator

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good; one tiny documentation tweak but it's just a single word so I'll make it then approve 🙂

includes/Modules/Analytics.php Outdated Show resolved Hide resolved
@tofumatt
Copy link
Collaborator

@googlebot I consent.

Copy link
Collaborator

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great—so much simpler!

Can you add a QA brief to the issue? After that we can merge it 👍🏻

@eugene-manuilov
Copy link
Collaborator Author

@tofumatt QAB is added.

Copy link
Collaborator

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is a lot less complicated which is great 👍🏻

I tested this locally and it worked for me. Nice!

@tofumatt tofumatt merged commit d6ea055 into develop Apr 15, 2021
@tofumatt tofumatt deleted the enhancement/3012-analytics-account-summaries branch April 15, 2021 19:43
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