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

show that the web updater is not recommended on big instances #31537

Merged
merged 2 commits into from
Mar 28, 2022
Merged

show that the web updater is not recommended on big instances #31537

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8122f2
show warning in update admin overview
szaimen Mar 11, 2022
4e11d7d
address reviews
szaimen Mar 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion apps/updatenotification/lib/Settings/Admin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
*/
namespace OCA\UpdateNotification\Settings;

use OC\User\Backend;
use OCP\User\Backend\ICountUsersBackend;
use OCA\UpdateNotification\UpdateChecker;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\IConfig;
Expand All @@ -38,6 +40,8 @@
use OCP\Settings\ISettings;
use OCP\Support\Subscription\IRegistry;
use OCP\Util;
use OCP\IUserManager;
use Psr\Log\LoggerInterface;

class Admin implements ISettings {
/** @var IConfig */
Expand All @@ -52,21 +56,29 @@ class Admin implements ISettings {
private $l10nFactory;
/** @var IRegistry */
private $subscriptionRegistry;
/** @var IUserManager */
private $userManager;
/** @var LoggerInterface */
private $logger;

public function __construct(
IConfig $config,
UpdateChecker $updateChecker,
IGroupManager $groupManager,
IDateTimeFormatter $dateTimeFormatter,
IFactory $l10nFactory,
IRegistry $subscriptionRegistry
IRegistry $subscriptionRegistry,
IUserManager $userManager,
LoggerInterface $logger
) {
$this->config = $config;
$this->updateChecker = $updateChecker;
$this->groupManager = $groupManager;
$this->dateTimeFormatter = $dateTimeFormatter;
$this->l10nFactory = $l10nFactory;
$this->subscriptionRegistry = $subscriptionRegistry;
$this->userManager = $userManager;
$this->logger = $logger;
}

/**
Expand Down Expand Up @@ -111,6 +123,7 @@ public function getForm(): TemplateResponse {
'downloadLink' => empty($updateState['downloadLink']) ? '' : $updateState['downloadLink'],
'changes' => $this->filterChanges($updateState['changes'] ?? []),
'webUpdaterEnabled' => !$this->config->getSystemValue('upgrade.disable-web', false),
'isWebUpdaterRecommended' => $this->isWebUpdaterRecommended(),
'updaterEnabled' => empty($updateState['updaterEnabled']) ? false : $updateState['updaterEnabled'],
'versionIsEol' => empty($updateState['versionIsEol']) ? false : $updateState['versionIsEol'],
'isDefaultUpdateServerURL' => $isDefaultUpdateServerURL,
Expand Down Expand Up @@ -184,4 +197,40 @@ public function getSection(): string {
public function getPriority(): int {
return 11;
}

private function isWebUpdaterRecommended(): bool {
return $this->getUserCount() < 100;
}

// Copied from https://github.com/nextcloud/server/blob/a06001e0851abc6073af678b742da3e1aa96eec9/lib/private/Support/Subscription/Registry.php#L187-L214
szaimen marked this conversation as resolved.
Show resolved Hide resolved
private function getUserCount(): int {
$userCount = 0;
$backends = $this->userManager->getBackends();
foreach ($backends as $backend) {
// TODO: change below to 'if ($backend instanceof ICountUsersBackend) {'
szaimen marked this conversation as resolved.
Show resolved Hide resolved
if ($backend->implementsActions(Backend::COUNT_USERS)) {
/** @var ICountUsersBackend $backend */
$backendUsers = $backend->countUsers();
if ($backendUsers !== false) {
$userCount += $backendUsers;
} else {
// TODO what if the user count can't be determined?
$this->logger->warning('Can not determine user count for ' . get_class($backend), ['app' => 'updatenotification']);
szaimen marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

$disabledUsers = $this->config->getUsersForUserValue('core', 'enabled', 'false');
$disabledUsersCount = count($disabledUsers);
szaimen marked this conversation as resolved.
Show resolved Hide resolved
$userCount = $userCount - $disabledUsersCount;

if ($userCount < 0) {
$userCount = 0;

// this should never happen
$this->logger->warning("Total user count was negative (users: $userCount, disabled: $disabledUsersCount)", ['app' => 'updatenotification']);
}

return $userCount;
}
}
8 changes: 8 additions & 0 deletions apps/updatenotification/src/components/UpdateNotification.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@
</ul>
</template>

<template v-if="!isWebUpdaterRecommended && updaterEnabled && webUpdaterEnabled">
<h3 class="warning">
{{ t('updatenotification', 'Please note that the web updater is not recommended with more than 100 users! Please use the command line updater instead!') }}
</h3>
</template>

<div>
<a v-if="updaterEnabled && webUpdaterEnabled"
href="#"
Expand Down Expand Up @@ -136,6 +142,7 @@ export default {
lastCheckedDate: '',
isUpdateChecked: false,
webUpdaterEnabled: true,
isWebUpdaterRecommended: true,
updaterEnabled: true,
versionIsEol: false,
downloadLink: '',
Expand Down Expand Up @@ -328,6 +335,7 @@ export default {
this.lastCheckedDate = data.lastChecked
this.isUpdateChecked = data.isUpdateChecked
this.webUpdaterEnabled = data.webUpdaterEnabled
this.isWebUpdaterRecommended = data.isWebUpdaterRecommended
this.updaterEnabled = data.updaterEnabled
this.downloadLink = data.downloadLink
this.isNewVersionAvailable = data.isNewVersionAvailable
Expand Down
128 changes: 127 additions & 1 deletion apps/updatenotification/tests/Settings/AdminTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
use OCP\Support\Subscription\IRegistry;
use OCP\Util;
use Test\TestCase;
use OCP\IUserManager;
use Psr\Log\LoggerInterface;

class AdminTest extends TestCase {
/** @var IFactory|\PHPUnit\Framework\MockObject\MockObject */
Expand All @@ -57,6 +59,10 @@ class AdminTest extends TestCase {
private $dateTimeFormatter;
/** @var IRegistry|\PHPUnit\Framework\MockObject\MockObject */
private $subscriptionRegistry;
/** @var IUserManager|\PHPUnit\Framework\MockObject\MockObject */
private $userManager;
/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
private $logger;

protected function setUp(): void {
parent::setUp();
Expand All @@ -67,13 +73,58 @@ protected function setUp(): void {
$this->dateTimeFormatter = $this->createMock(IDateTimeFormatter::class);
$this->l10nFactory = $this->createMock(IFactory::class);
$this->subscriptionRegistry = $this->createMock(IRegistry::class);
$this->userManager = $this->createMock(IUserManager::class);
$this->logger = $this->createMock(LoggerInterface::class);

$this->admin = new Admin(
$this->config, $this->updateChecker, $this->groupManager, $this->dateTimeFormatter, $this->l10nFactory, $this->subscriptionRegistry
$this->config,
$this->updateChecker,
$this->groupManager,
$this->dateTimeFormatter,
$this->l10nFactory,
$this->subscriptionRegistry,
$this->userManager,
$this->logger
);
}

public function testGetFormWithUpdate() {
$backend1 = $this->createMock(UserInterface::class);
$backend2 = $this->createMock(UserInterface::class);
$backend3 = $this->createMock(UserInterface::class);
$backend1
->expects($this->once())
->method('implementsActions')
->with(Backend::COUNT_USERS)
->willReturn(false);
$backend2
->expects($this->once())
->method('implementsActions')
->with(Backend::COUNT_USERS)
->willReturn(true);
$backend3
->expects($this->once())
->method('implementsActions')
->with(Backend::COUNT_USERS)
->willReturn(true);
$backend1
->expects($this->never())
->method('countUsers');
$backend2
->expects($this->once())
->method('countUsers')
->with()
->willReturn(false);
$backend3
->expects($this->once())
->method('countUsers')
->with()
->willReturn(5);
$this->userManager
->expects($this->once())
->method('getBackends')
->with()
->willReturn([$backend1, $backend2, $backend3]);
$channels = [
'daily',
'beta',
Expand Down Expand Up @@ -145,6 +196,7 @@ public function testGetFormWithUpdate() {
'downloadLink' => 'https://downloads.nextcloud.org/server',
'changes' => [],
'webUpdaterEnabled' => true,
'isWebUpdaterRecommended' => true,
'updaterEnabled' => true,
'versionIsEol' => false,
'isDefaultUpdateServerURL' => true,
Expand All @@ -161,6 +213,42 @@ public function testGetFormWithUpdate() {
}

public function testGetFormWithUpdateAndChangedUpdateServer() {
$backend1 = $this->createMock(UserInterface::class);
$backend2 = $this->createMock(UserInterface::class);
$backend3 = $this->createMock(UserInterface::class);
$backend1
->expects($this->once())
->method('implementsActions')
->with(Backend::COUNT_USERS)
->willReturn(false);
$backend2
->expects($this->once())
->method('implementsActions')
->with(Backend::COUNT_USERS)
->willReturn(true);
$backend3
->expects($this->once())
->method('implementsActions')
->with(Backend::COUNT_USERS)
->willReturn(true);
$backend1
->expects($this->never())
->method('countUsers');
$backend2
->expects($this->once())
->method('countUsers')
->with()
->willReturn(false);
$backend3
->expects($this->once())
->method('countUsers')
->with()
->willReturn(5);
$this->userManager
->expects($this->once())
->method('getBackends')
->with()
->willReturn([$backend1, $backend2, $backend3]);
$channels = [
'daily',
'beta',
Expand Down Expand Up @@ -232,6 +320,7 @@ public function testGetFormWithUpdateAndChangedUpdateServer() {
'downloadLink' => 'https://downloads.nextcloud.org/server',
'changes' => [],
'webUpdaterEnabled' => false,
'isWebUpdaterRecommended' => true,
'updaterEnabled' => true,
'versionIsEol' => false,
'isDefaultUpdateServerURL' => false,
Expand All @@ -248,6 +337,42 @@ public function testGetFormWithUpdateAndChangedUpdateServer() {
}

public function testGetFormWithUpdateAndCustomersUpdateServer() {
$backend1 = $this->createMock(UserInterface::class);
$backend2 = $this->createMock(UserInterface::class);
$backend3 = $this->createMock(UserInterface::class);
$backend1
->expects($this->once())
->method('implementsActions')
->with(Backend::COUNT_USERS)
->willReturn(false);
$backend2
->expects($this->once())
->method('implementsActions')
->with(Backend::COUNT_USERS)
->willReturn(true);
$backend3
->expects($this->once())
->method('implementsActions')
->with(Backend::COUNT_USERS)
->willReturn(true);
$backend1
->expects($this->never())
->method('countUsers');
$backend2
->expects($this->once())
->method('countUsers')
->with()
->willReturn(false);
$backend3
->expects($this->once())
->method('countUsers')
->with()
->willReturn(5);
$this->userManager
->expects($this->once())
->method('getBackends')
->with()
->willReturn([$backend1, $backend2, $backend3]);
$channels = [
'daily',
'beta',
Expand Down Expand Up @@ -319,6 +444,7 @@ public function testGetFormWithUpdateAndCustomersUpdateServer() {
'downloadLink' => 'https://downloads.nextcloud.org/server',
'changes' => [],
'webUpdaterEnabled' => true,
'isWebUpdaterRecommended' => true,
'updaterEnabled' => true,
'versionIsEol' => false,
'isDefaultUpdateServerURL' => true,
Expand Down
Loading