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

Introduce anonymous product analytics #746

Merged
merged 15 commits into from
Jul 11, 2024

Conversation

Quetzacoalt91
Copy link
Member

Questions Answers
Description? This PR introduces anonymous analytics to know how many times the module is used to upgrade / restore and how.
Type? new feature
BC breaks? Yes, some files are being moved, i.e Tasks
Deprecations? Nope
Fixed ticket? Fixes PrestaShop/PrestaShop#36432
Sponsor company @PrestaShopCorp
How to test? Triggering updates and restores will be counted, as well as their ending (Successful or not).

@Quetzacoalt91 Quetzacoalt91 self-assigned this Jun 28, 2024
@Quetzacoalt91 Quetzacoalt91 added this to the 6.0.0 milestone Jul 1, 2024
matks
matks previously approved these changes Jul 1, 2024
{
$matches = [];
preg_match(
'/^V(?<version>[1-9\.]+)_/',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: explain in phpDoc why you exclude some patterns

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I got your message properly. What do you mean by excluding when I try to pickup the version from a string?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I call getRestoreVersion(), you check $this->getRestoreName() and verify if it matches V(?<version>[1-9\.]+)

If it matches, you return it. If it does not you return null. So I said you "exclude" patterns which do not match.

An implementation that does not do exclusion would be simply returning `$this->getRestoreName()

I suggest explaining the reason behind the above logic

Copy link
Member Author

Choose a reason for hiding this comment

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

Here you go

Comment on lines 49 to 51
/**
* @var array
*/
Copy link
Contributor

@M0rgan01 M0rgan01 Jul 2, 2024

Choose a reason for hiding this comment

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

This will not work with the new level of PHPstan 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

Fortunately PHPStan's PR should be merged first, and I'll get all the new feedback from it.

Copy link
Contributor

@AureRita AureRita left a comment

Choose a reason for hiding this comment

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

Hi @Quetzacoalt91

Thank you for your PR, I tested it and it seems to works as you can see :

image

Tested from :
1.7.6.4 to 8.1.7
8.0.5 to 8.1.7
8.1.5 to 8.1.7
8.1.7 to 9.0.0
8.1.5 to 9.0.0
8.0.5 to 9.0.0

Because the PR seems to works as expected, It's QA ✔️

Thank you

@Quetzacoalt91
Copy link
Member Author

Quetzacoalt91 commented Jul 11, 2024

It appeared during the tests that failing upgrade are not reported in case of a PHP timeout. There is room for improvement.

Thanks @AureRita

@Quetzacoalt91 Quetzacoalt91 merged commit 8ef3d2a into PrestaShop:dev Jul 11, 2024
34 checks passed
@Quetzacoalt91 Quetzacoalt91 deleted the analytics branch July 11, 2024 13:31
@matks
Copy link
Contributor

matks commented Jul 11, 2024

@Quetzacoalt91 Maybe it's the moment where register_shutdown_function() shines

@Quetzacoalt91
Copy link
Member Author

Indeed, we already have it to send responses gracefully, so it may be completed with this feature.

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

Successfully merging this pull request may close these issues.

5 participants