Add row for guest users to command occ user:report#38742
Conversation
AlexAndBear
force-pushed
the
enterprise/issues/3467
branch
from
5d63328 to
b65f995
Compare
|
@jvillafanez not quite sure if we need to introduce \OC::$server->getUserTypeHelper |
|
Double-check the following scenario:
I think the guests will be counted as normal users in step 4. It doesn't seem right. Honestly, I don't know how much knowledge core has about guests, nor what is the exact relationship with the guests app. It seems that we're adding responsibilities into core that wasn't designed for. |
|
@jvillafanez We can easily overcome this issue by adjusting the UserTypeHelper::isGuestUser, it pre-checks if the app is enabled and if not, it returns false. I introduced this helper and feel confident with removing the check |
|
@jvillafanez changed check to find out if a user is guest user, now your provided example give correct results |
| * @return bool | ||
| */ | ||
| public function isGuestUser($uid) { | ||
| $guestsAppEnabled = $this->appManager->isEnabledForUser('guests'); |
There was a problem hiding this comment.
@VicDeo any expected side effect for this removal? I don't remember if there was a reason for this check. Removing it looks fine to me.
There was a problem hiding this comment.
@jvillafanez well... my initial logic was like this: if the guests app is disabled the instance has no guests at all. Because the guests capability is provided by the respective app
In this context 'guest' is a user managed by the guests app.
There was a problem hiding this comment.
Yes, that was my idea too. Technically, it makes sense.
We should properly implement something in core to provide different user types
|
I'd wait a bit for confirmation on #38742 (comment) . The rest is mostly style things easy to fix. |
|
doc relevant, pls file an issue in docs close to merging. |
AlexAndBear
force-pushed
the
enterprise/issues/3467
branch
from
9bc9015 to
8f3b6df
Compare
|
@mmattel can you link the relevant paragraph? |
|
|
Kudos, SonarCloud Quality Gate passed!
|
|
@VicDeo your opinion is highly required 🚀 |
jvillafanez
left a comment
jvillafanez
left a comment
There was a problem hiding this comment.
Good to go for the short term
4 participants
Description
Enhancement: Command occ user:report shows additional row for guests
With this improvement, a new row will be rendered for guest user count on the
occ user:report command.
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: