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.

Sign up for GitHub

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

Jump to bottom

Add code to check plugin license status and download link #22567

Open
wants to merge 40 commits into
base: 5.x-dev
Choose a base branch
from
Open

Add code to check plugin license status and download link #22567

wants to merge 40 commits into from
+1,306 −201

Conversation

AltamashShaikh
Copy link
Contributor

Description:

Adds code to check plugin wise license status and download link
Fixes: PG-3814, #PG-3770

Review

michalkleiner
michalkleiner reviewed Sep 9, 2024
View reviewed changes
plugins/Marketplace/Plugins.php Outdated
@@ -296,6 +298,9 @@ private function enrichPluginInformation($plugin)
}
}

$plugin['licenseStatus'] = $consumer[$plugin['name']]['licenseStatus'] ?? '';
$plugin['hasDownloadLink'] = !empty($plugin['versions'][0]['download']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's correct to use the first version here, is it the latest one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yaa seeing the marketplace code, it does look like we get the latest version

Uploading Screenshot from 2024-09-09 14-09-04.png…

Copy link
Member

Choose a reason for hiding this comment

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

Actually it looks like it's the last element holding the latest version. At least we are handling it that way here:

matomo/plugins/Marketplace/Api/Client.php

Lines 320 to 332 in 9a3ef94

public function getDownloadUrl($pluginOrThemeName)
{
$plugin = $this->getPluginInfo($pluginOrThemeName);
if (empty($plugin['versions'])) {
throw new Exception('Plugin has no versions.');
}
$latestVersion = array_pop($plugin['versions']);
$downloadUrl = $latestVersion['download'];
return $this->service->getDomain() . $downloadUrl . '?coreVersion=' . $this->environment->getPiwikVersion();
}

@AltamashShaikh
Copy link
Contributor Author

@michalkleiner @sgiehl Do you think this PR changes are good ?
I can think of adding tests around it, if this looks good to you both

@AltamashShaikh AltamashShaikh marked this pull request as ready for review September 12, 2024 09:19
@AltamashShaikh AltamashShaikh added the Needs Review PRs that need a code review label Sep 12, 2024
@michalkleiner michalkleiner changed the title Adds code to check plugin wise license status and download link Add code to check plugin license status and download link Sep 13, 2024
@AltamashShaikh
Copy link
Contributor Author

@sgiehl @michalkleiner Can you review the PR again and merge if all looks good

sgiehl
sgiehl requested changes Sep 19, 2024
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left a couple of comments for changes that are unclear to me

plugins/Marketplace/Consumer.php Outdated
Comment on lines 74 to 86
public function getConsumerPluginLicenseInfo(): array
{
if (!$this->pluginLicenseInfo) {
$consumer = $this->getConsumer();
if (!empty($consumer['licenses'])) {
foreach ($consumer['licenses'] as $license) {
$this->pluginLicenseInfo[$license['plugin']['name']] = ['licenseStatus' => $license['status']];
}
}
}

return $this->pluginLicenseInfo ?? [];
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not fully happy with this method. It's called getConsumerPluginLicenseInfo, but actually only returns the status.
If only the status is relevant, then we could simply rename the method to getConsumerPluginLicenseStatus and return a flat array like 'pluginName' => 'licenseStatus', instead of having the extra array level.
If the method is supposed to return more details of a license, why don't we directly return all information we have, instead of limiting it to what we currently need?

plugins/Marketplace/Plugins.php Outdated
@@ -296,6 +298,9 @@ private function enrichPluginInformation($plugin)
}
}

$plugin['licenseStatus'] = $consumer[$plugin['name']]['licenseStatus'] ?? '';
$plugin['hasDownloadLink'] = !empty($plugin['versions'][0]['download']);
Copy link
Member

Choose a reason for hiding this comment

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

Actually it looks like it's the last element holding the latest version. At least we are handling it that way here:

matomo/plugins/Marketplace/Api/Client.php

Lines 320 to 332 in 9a3ef94

public function getDownloadUrl($pluginOrThemeName)
{
$plugin = $this->getPluginInfo($pluginOrThemeName);
if (empty($plugin['versions'])) {
throw new Exception('Plugin has no versions.');
}
$latestVersion = array_pop($plugin['versions']);
$downloadUrl = $latestVersion['download'];
return $this->service->getDomain() . $downloadUrl . '?coreVersion=' . $this->environment->getPiwikVersion();
}

...ketplace/tests/UI/expected-screenshots/Marketplace_paid_plugin3_plugin_details_superuser.png Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Is it an purpose to show the install button when the license is cancelled? 🧐

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sgiehl Yes if a user has cancelled and made payment for that month, marketplace will return an URL and hence we should show the install button in that case.

...sts/UI/expected-screenshots/Marketplace_paid_plugin4_plugin_details_multiUserEnvironment.png Outdated
Copy link
Member

Choose a reason for hiding this comment

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

The notification text reads a bit unexpected in that case. It sounds like it tried to install, but failed. While it actually should say, that it's not possible to install it automatically.
Maybe Unable to install plugin %plugin%. would be usable for both cases instead of %plugin% could not be installed.

Also in case of a multi server env it doesn't make sense to show both messages. If they need to install the plugin manually, they need to download it manually anyway.

...lace/tests/UI/expected-screenshots/Marketplace_paid_plugin_details_add_to_cart_superuser.png Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why should we show the notification here? I guess we only show the add to cart button when there is no license, right? So why should we tell them it can't be installed at the same time?

...rketplace/tests/UI/expected-screenshots/Marketplace_paid_plugin_details_add_to_cart_user.png Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Same here. The request trial button is displayed, so there seems to be no license - thus we don't need a warning if the plugin can't be installed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl requested changes

@michalkleiner michalkleiner Awaiting requested review from michalkleiner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Needs Review PRs that need a code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AltamashShaikh @michalkleiner @sgiehl