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

Analytics viewlet: make webstats_js a property #229

Merged
merged 1 commit into from
Feb 6, 2020

Conversation

ale-rt
Copy link
Member

@ale-rt ale-rt commented Feb 6, 2020

Analytics viewlet: make webstats_js a property, so that it does not rely on an a call to the update method to be correctly evaluated

Fixes #227

Analytics viewlet: make webstats_js a property, so that it does not rely on an a call to the update method to be correctly evaluated

Fixes #227
@ale-rt ale-rt requested a review from erral February 6, 2020 10:10
@mister-roboto
Copy link

@ale-rt thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@ale-rt ale-rt requested a review from mauritsvanrees February 6, 2020 10:10
@ale-rt
Copy link
Member Author

ale-rt commented Feb 6, 2020

@jenkins-plone-org please run jobs

Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

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

Thank you, I can confirm that this works. Let's see what Jenkins says.

For me this is not a breaking change. I would say: take the text from 227.breaking and put it in 227.bugfix. (Can be done after merge too, no need to run the tests again after such a change.)

Copy link
Member

@erral erral left a comment

Choose a reason for hiding this comment

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

LGTM

@ale-rt
Copy link
Member Author

ale-rt commented Feb 6, 2020

Ok for the bugfix, the chances that people are fiddling with this are pretty low (even if not null).

@ale-rt
Copy link
Member Author

ale-rt commented Feb 6, 2020

Build timed out (after 180 minutes). Marking the build as aborted...

@mauritsvanrees
Copy link
Member

The other two jobs pass. Good enough for me. I will merge.
I will handle the news snippet after merge.

@mauritsvanrees mauritsvanrees merged commit d4934fc into master Feb 6, 2020
@mauritsvanrees mauritsvanrees deleted the 227-analytics-viewlet branch February 6, 2020 13:19
mauritsvanrees added a commit that referenced this pull request Feb 6, 2020
I call this a bugfix, not a breaking change.
See #229 (review)

[ci skip]
talarias pushed a commit that referenced this pull request Dec 9, 2021
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.

analytics viewlet breaks if registry record is not defined
4 participants