Skip to content

Allow subscription to indicate that a userlimit is reached - #23278

Merged
ChristophWurst merged 2 commits into
masterfrom
enh/noid/user-limits
Dec 2, 2020
Merged

Allow subscription to indicate that a userlimit is reached#23278
ChristophWurst merged 2 commits into
masterfrom
enh/noid/user-limits

Conversation

@MorrisJobke

@MorrisJobkeMorrisJobke commented Oct 8, 2020

Copy link
Copy Markdown
Member

No description provided.

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice stuff!

Comment threadlib/private/Support/Subscription/Registry.php Outdated
Comment threadlib/private/Support/Subscription/Registry.php Outdated
@skjnldsvskjnldsv added the 2. developing Work in progress label Oct 31, 2020
@MorrisJobke
MorrisJobkeforce-pushed the enh/noid/user-limits branch 2 times, most recently from 24d9915 to 21da73aCompareDecember 2, 2020 08:45
@MorrisJobke
MorrisJobke marked this pull request as ready for review December 2, 2020 08:45
@MorrisJobkeMorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 2, 2020
Comment threadlib/private/Support/Subscription/Registry.php Outdated
@MorrisJobke

Copy link
Copy Markdown
MemberAuthor

/backport to stable20

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke

Copy link
Copy Markdown
MemberAuthor

Ready for review 🚀

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! Didn't test in depth but a quick smoke test showed it OK. And the test seems sane!

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good!

Comment on lines 301 to +307
public function createUser($uid, $password) {
// DI injection is not used here as IRegistry needs the user manager itself for user count and thus it would create a cyclic dependency
if (\OC::$server->get(IRegistry::class)->delegateIsHardUserLimitReached()) {
$l = \OC::$server->getL10N('lib');
throw new HintException($l->t('The user limit has been reached and the user was not created.'));
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

heads up, this will not catch users provisioned by other backends

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes we know
will come in follup PRs

@jospoortvlietjospoortvliet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, just these changes would be nice 👍


if ($notification->getSubject() === 'user_limit_reached') {
$notification->setParsedSubject($l->t('The user limit of this instance is reached.'));
$notification->setParsedMessage($l->t('Add a subscription key to increase the user limit of this instance. For more information have a look at the Enterprise subscription page.'));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
$notification->setParsedMessage($l->t('Add a subscription key to increase the user limit of this instance. For more information have a look at the Enterprise subscription page.'));
$notification->setParsedMessage($l->t('Enter your subscription key to increase the user limit. For more information about Nextcloud Enterprise see our website.'));

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added to #24516

if ($notification->getSubject() === 'user_limit_reached') {
$notification->setParsedSubject($l->t('The user limit of this instance is reached.'));
$notification->setParsedMessage($l->t('Add a subscription key to increase the user limit of this instance. For more information have a look at the Enterprise subscription page.'));
$notification->setLink('https://nextcloud.com/enterprise/order/');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
$notification->setLink('https://nextcloud.com/enterprise/order/');
$notification->setLink('https://nextcloud.com/enterprise/');

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added to #24516

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

Labels

3. to reviewWaiting for reviewsenhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@MorrisJobke@rullzer@jospoortvliet@ChristophWurst@blizzz@skjnldsv