Skip to content

[stable23] show that the web updater is not recommended on big instances #31740

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

Merged
szaimen merged 2 commits into from
Mar 30, 2022
Merged

[stable23] show that the web updater is not recommended on big instances #31740

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
71ff8e5
show warning in update admin overview
szaimen Mar 11, 2022
5d01de8
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
Next Next commit
show warning in update admin overview 
Signed-off-by: szaimen <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
  • Loading branch information
@szaimen
szaimen committed Mar 28, 2022
commit 71ff8e58dd053059cbabc72e410bc10f9d88524f
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
private function getUserCount(): int {
$userCount = 0;
$backends = $this->userManager->getBackends();
foreach ($backends as $backend) {
// TODO: change below to 'if ($backend instanceof ICountUsersBackend) {'
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']);
}
}
}

$disabledUsers = $this->config->getUsersForUserValue('core', 'enabled', 'false');
$disabledUsersCount = count($disabledUsers);
$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