Skip to content

Admins can change display name when flag set to false#29132

Merged
PVince81 merged 1 commit intomasterfrom
admin-change-display-name
Nov 6, 2017
Merged

Admins can change display name when flag set to false#29132
PVince81 merged 1 commit intomasterfrom
admin-change-display-name

Conversation

@sharidas
Copy link
Contributor

@sharidas sharidas commented Oct 2, 2017

When 'allow_user_to_change_display_name' => false, is
set in the config.php then admin user should be
able to set the display name.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

When 'allow_user_to_change_display_name' => false, is set in the config.php, then the admin user should be able to set the display name of users.

Related Issue

#27442

Motivation and Context

When 'allow_user_to_change_display_name' => false, is set in the config.php, the admin user wasn't able to set the display name. This change will help to allow the admin to set the display name for users.

How Has This Been Tested?

  • create 3 users admin, user1, user2
  • Add 'allow_user_to_change_display_name' => false, to the config.php
  • Login as admin user and try to rename the admin to "Foo". It worked.
  • Create a new group test and add user1 and user2 to the group test.
  • Make user1 as the group admin for test1.
  • Now try to login as user then try to change the display name for user1, it worked.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@sharidas sharidas self-assigned this Oct 2, 2017
@sharidas sharidas added this to the development milestone Oct 2, 2017
@sharidas sharidas force-pushed the admin-change-display-name branch from 3aacaa5 to 4e57dff Compare October 3, 2017 07:06
@sharidas
Copy link
Contributor Author

sharidas commented Oct 3, 2017

Made modification so that admin and group admins can change the display name. It would be nice to know if we want to have only

  1. admins and group admins to change the display name of users
  2. or only admins allowed to change display name

@sharidas sharidas changed the title [WIP] Admins can change display name when flag set to false Admins can change display name when flag set to false Oct 3, 2017
@sharidas sharidas force-pushed the admin-change-display-name branch from 4e57dff to 803770b Compare October 3, 2017 11:04
@DeepDiver1975
Copy link
Member

Any chance to get this unit tested? THX

@PVince81
Copy link
Contributor

@sharidas I think any kind of admin should be able to change the display name. Non-admins aka users should not.

@sharidas
Copy link
Contributor Author

So if I understand correctly

admins and group admins to change the display name of users
Admins and group admins are allowed to change the display name if allow_user_to_change_display_name' => false, is added to config.php

@PVince81
Copy link
Contributor

@sharidas correct

@PVince81
Copy link
Contributor

@pmaier1 FYI ^

@sharidas
Copy link
Contributor Author

Now an admin and group admin can change the users display name

@sharidas sharidas force-pushed the admin-change-display-name branch from 803770b to d2fe69f Compare October 10, 2017 15:21
@PVince81
Copy link
Contributor

@sharidas please adjust unit tests. If there are none, please add some. Thanks

@sharidas
Copy link
Contributor Author

sharidas commented Oct 10, 2017

please adjust unit tests. If there are none, please add some. Thanks

Thats next task.

@sharidas sharidas force-pushed the admin-change-display-name branch from d2fe69f to 648f118 Compare November 3, 2017 06:55
Copy link
Contributor Author

@sharidas sharidas Nov 3, 2017

Choose a reason for hiding this comment

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

Injecting GroupManager and UserSession here. Didn't liked the previous version of the patch. So used this approach. Let me know if it looks ok to inject here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have also added unit test for the same.

@codecov
Copy link

codecov bot commented Nov 3, 2017

Codecov Report

Merging #29132 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #29132      +/-   ##
============================================
+ Coverage     60.45%   60.46%   +<.01%     
- Complexity    17222    17226       +4     
============================================
  Files          1032     1032              
  Lines         57360    57368       +8     
============================================
+ Hits          34677    34686       +9     
+ Misses        22683    22682       -1
Impacted Files Coverage Δ Complexity Δ
lib/private/User/User.php 82.82% <100%> (+1.53%) 63 <0> (+4) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2654cbc...6a75852. Read the comment docs.

@sharidas sharidas force-pushed the admin-change-display-name branch 2 times, most recently from 8ba23cf to 6e1e30c Compare November 3, 2017 14:44
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@sharidas
Copy link
Contributor Author

sharidas commented Nov 3, 2017

Will create backport.

@sharidas
Copy link
Contributor Author

sharidas commented Nov 3, 2017

Backport to stable10: #29442

@DeepDiver1975
Copy link
Member

16:07:28 [owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q] Running shell script
16:07:28 + export NOCOVERAGE=1
16:07:28 + NOCOVERAGE=1
16:07:28 + unset USEDOCKER
16:07:28 + phpenv local 5.6
16:07:28 + make test-php TEST_DATABASE=pgsql
16:07:28 PHPUNIT="/var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/composer/phpunit/phpunit/phpunit" build/autotest.sh pgsql 
16:07:28 Using PHP executable /opt/phpenv/shims/php
16:07:28 Parsing all files in lib/public for the presence of @since or @deprecated on each method...
16:07:28 
16:07:30 Using database oc_autotest5
16:07:30 Setup environment for pgsql testing on local storage ...
16:07:34 Installing ....
16:07:52 PHP Fatal error:  Cannot use OC\Group\Manager as Manager because the name is already in use in /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/private/User/User.php on line 41
16:07:52 PHP Stack trace:
16:07:52 PHP   1. {main}() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/occ:0
16:07:52 PHP   2. require_once() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/occ:11
16:07:52 PHP   3. OC\Console\Application->run() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/console.php:106
16:07:52 PHP   4. Symfony\Component\Console\Application->run() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/private/Console/Application.php:162
16:07:52 PHP   5. Symfony\Component\Console\Application->doRun() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/composer/symfony/console/Application.php:130
16:07:52 PHP   6. Symfony\Component\Console\Application->doRunCommand() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/composer/symfony/console/Application.php:228
16:07:52 PHP   7. Symfony\Component\Console\Command\Command->run() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/composer/symfony/console/Application.php:874
16:07:52 PHP   8. OC\Core\Command\Maintenance\Install->execute() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/composer/symfony/console/Command/Command.php:264
16:07:52 PHP   9. OC\Setup->install() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/core/Command/Maintenance/Install.php:86
16:07:52 PHP  10. OC\User\Manager->createUser() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/private/Setup.php:347
16:07:52 PHP  11. OC\User\Manager->getUserObject() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/private/User/Manager.php:336
16:07:52 PHP  12. spl_autoload_call() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/private/User/Manager.php:180
16:07:52 PHP  13. Composer\Autoload\ClassLoader->loadClass() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/private/User/Manager.php:180
16:07:52 PHP  14. Composer\Autoload\includeFile() /var/lib/jenkins/workspace/owncloud-core_core_PR-29132-2PBBEYQ3IXYKPHPI4JLRKSIPJOYVTQEFY26WO6EJ72XKCBDAG24Q/lib/composer/composer/ClassLoader.php:322

Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

When 'allow_user_to_change_display_name' => false, is
set in the config.php then admin user should be
able to set the display name.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@sharidas sharidas force-pushed the admin-change-display-name branch from 6e1e30c to 6a75852 Compare November 3, 2017 16:04
@sharidas
Copy link
Contributor Author

sharidas commented Nov 5, 2017

All checks passed.

@PVince81 PVince81 merged commit 6a3a485 into master Nov 6, 2017
@PVince81 PVince81 deleted the admin-change-display-name branch November 6, 2017 09:33
@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants