[WIP] Move user creation to a separate service #36145

sharidas · 2019-09-03T12:23:05Z

Move user creation to a separate service.

Signed-off-by: Sujith H [email protected]

Description

Move the user creation to the service class.

Related Issue

Fixes <issue_link>

Motivation and Context

Add the create user to a service class. It is moved from user_management apps UsersController. This will help us in integrating the change with the user:add command support creating users with username and email address. It will also help for provisioning API.

How Has This Been Tested?

This PR is same as Move user creation to a separate service #35972. Meaning all the tests are applicable to Move user creation to a separate service #35972 are applicable here too.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

sharidas · 2019-09-03T12:24:08Z

This PR is marked as deffered. The changes are same as #35972.

codecov · 2019-12-06T08:42:20Z

Codecov Report

Merging #36145 (76c356c) into master (7efdf97) will increase coverage by 0.08%.
The diff coverage is 78.98%.

@@             Coverage Diff              @@
##             master   #36145      +/-   ##
============================================
+ Coverage     64.68%   64.77%   +0.08%     
- Complexity    19118    19175      +57     
============================================
  Files          1269     1281      +12     
  Lines         74775    75039     +264     
  Branches       1320     1320              
============================================
+ Hits          48369    48603     +234     
- Misses        26018    26048      +30     
  Partials        388      388

Flag	Coverage Δ	Complexity Δ
javascript	`54.12% <ø> (ø)`	`0.00 <ø> (ø)`
phpunit	`65.94% <78.98%> (+0.08%)`	`0.00 <53.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
core/Application.php	`22.83% <0.00%> (-0.75%)`	`1.00 <0.00> (ø)`
core/register_command.php	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
core/routes.php	`48.71% <ø> (ø)`	`0.00 <0.00> (ø)`
core/templates/new_user/resendtokenbymail.php	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
core/templates/new_user/setpassword.php	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
core/templates/new_user/tokensendnotify.php	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
core/Command/User/Add.php	`61.79% <48.64%> (+6.50%)`	`19.00 <1.00> (+3.00)`
apps/provisioning_api/lib/Users.php	`91.97% <81.81%> (-0.60%)`	`124.00 <0.00> (+2.00)`	⬇️
lib/private/User/Service/CreateUserService.php	`90.00% <90.00%> (ø)`	`22.00 <22.00> (?)`
apps/provisioning_api/appinfo/routes.php	`100.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`
... and 22 more

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 7efdf97...76c356c. Read the comment docs.

karakayasemi · 2019-12-07T10:33:43Z

With Do not change the old behavior of user creation commit I tried to resolve the problems in that issue https://github.com/owncloud/enterprise/issues/2831 .

Additional string at the exception response of provisioning API removed.
Old text messages of occ user:add command now coming from stdErr again.

@phil-davis @sharidas is there anything that I miss related to #36019 and #36020?

phil-davis

This looks good. There are acceptance tests in cliProvisioning suite for the occ user and group commands. They check the success and the command output text... Those are still passing, so the occ command interface has not been affected.

lib/private/User/Service/CreateUserService.php

phil-davis · 2019-12-09T09:13:02Z

Is this just the first stage refactoring?
Or
does it also fix any issues with the occ user commands, e.g. creating users with just an email address?

If (2) then we should add some acceptance tests to cover the new/improved behavior.

karakayasemi · 2019-12-09T18:00:41Z

1. Is this just the first stage refactoring?
   Or

2. does it also fix any issues with the `occ` `user` commands, e.g. creating users with just an email address?

@phil-davis I just worked on #36019 and #36020. In addition, rebased the PR and made it compatible with the new PHPUnit version. @sharidas has more knowledge than me about the rest of the things.

sharidas · 2019-12-10T05:39:20Z

@phil-davis The change which @karakayasemi had made is with respect to the wording issues that was caused because of the commit 128b83a. Basically now with this change, the UI and cli (including the provisioning api) would be aligned with user creation.

phil-davis · 2019-12-10T05:46:07Z

unit tests fail with

[PHP Fatal error:  Declaration of Tests\Core\Controller\UserControllerTest::setUp() must be compatible with Test\TestCase::setUp(): void in /drone/src/tests/Core/Controller/UserControllerTest.php on line 477]

needs some more refactoring...

apps/provisioning_api/lib/Users.php

core/Command/User/Add.php

core/Controller/UserController.php

lib/private/User/Service/CreateUserService.php

lib/private/User/Service/UserSendMailService.php

core/Controller/UserController.php

lib/private/User/Service/UserSendMailService.php

karakayasemi · 2019-12-18T20:16:45Z

@jvillafanez I made some changes for aligning the code to your previous review. Also, I found some problems with translations and resolved them.
@sharidas @jvillafanez please help me by reviewing the PR one more time. To make the PR review process easier, please resolve conversations if you think it is resolved. Thanks.

jvillafanez · 2019-12-19T09:35:34Z

core/Controller/LostController.php

@@ -252,8 +250,7 @@ public function setPassword($token, $userId, $password, $proceed) {
 				throw new \Exception();
 			}

-			\OC_Hook::emit('\OC\Core\LostPassword\Controller\LostController', 'post_passwordReset', ['uid' => $userId, 'password' => $password]);


is this intended?

Yes, I think this way is better, explained here: #36145 (comment) . I would like to learn your opinion also.

jvillafanez · 2019-12-19T09:38:27Z

core/Controller/LostController.php

@@ -252,8 +250,7 @@ public function setPassword($token, $userId, $password, $proceed) {
 				throw new \Exception();
 			}

-			\OC_Hook::emit('\OC\Core\LostPassword\Controller\LostController', 'post_passwordReset', ['uid' => $userId, 'password' => $password]);
-			@\OC_User::unsetMagicInCookie();
+			$this->userSession->unsetMagicInCookie();


Quick question: has this been verified that there is no error showing anywhere? Since this touches cookies, I think there could be errors / warnings if any output has been sent to the client prior this point. I don't think this is the case, but maybe we should confirm this.

Explained in more detail in here: #36145 (comment)

No error log appears in manual tests with the described scenarios in there: #35972 (comment)

jvillafanez · 2019-12-19T09:51:19Z

lib/private/User/Service/CreateUserService.php

+			throw new UserAlreadyExistsException('A user with that name already exists.');
+		}
+
+		if (\array_key_exists('password', $arguments)) {


In theory, you could use $arguments['password'] = null and you'll be trying to set $password as null. I don't really know if this is handled properly.

If this is consider, we should include some information in the PHPDoc (and probably around here) explaining setting null as password (and / or email) is acceptable, and what are the consequences. Otherwise, I think it's better to use isset

lib/private/User/Service/CreateUserService.php

jvillafanez · 2019-12-19T10:02:30Z

lib/private/User/Service/UserSendMailService.php

+			'url' => $this->urlGenerator->linkToRouteAbsolute('core.user.setPasswordForm', ['userId' => $userId, 'token' => $token])
+		];
+
+		$mail = new TemplateResponse('core', 'new_user/email-html', $mailData, 'blank');


I think it's more common to use Template than TemplateResponse. Any reason to use TemplateResponse?

One of the reason I recall for the usage of TemplateResponse is to pass the value in $mailData.

lib/private/User/Service/UserSendMailService.php

jvillafanez · 2019-12-19T10:10:03Z

lib/private/User/Service/UserSendMailService.php

+			} catch (\Exception $exception) {
+				throw new EmailSendFailedException($this->l10n->t("Email could not be sent."));
+			}
+		}


any action to take if no email is set? should we log something?
Maybe we could add a comment if we don't want to take any action to remark doing nothing is intentional.

Added a comment.

Move user creation to a separate service. Signed-off-by: Sujith H <[email protected]>

DeepDiver1975 · 2023-01-04T22:43:54Z

old aka dead -> close

sharidas self-assigned this Sep 3, 2019

sharidas added the deferred label Sep 3, 2019

sharidas added this to the development milestone Sep 3, 2019

sharidas mentioned this pull request Sep 4, 2019

Words for "Unable to create user due to exception" #36020

Open

karakayasemi force-pushed the move-createuser-toservice-v3 branch from f90ab66 to 0e8df13 Compare December 6, 2019 08:42

karakayasemi added 2 - Developing and removed deferred labels Dec 6, 2019

karakayasemi force-pushed the move-createuser-toservice-v3 branch from 0e8df13 to 90f8a45 Compare December 6, 2019 09:13

karakayasemi self-assigned this Dec 6, 2019

karakayasemi force-pushed the move-createuser-toservice-v3 branch 2 times, most recently from 51bbc2b to 7e0ff42 Compare December 6, 2019 11:29

karakayasemi requested a review from phil-davis December 7, 2019 10:25

karakayasemi added 3 - To Review and removed 2 - Developing labels Dec 8, 2019

phil-davis reviewed Dec 9, 2019

View reviewed changes

lib/private/User/Service/CreateUserService.php Show resolved Hide resolved

lib/private/User/Service/CreateUserService.php Outdated Show resolved Hide resolved

karakayasemi force-pushed the move-createuser-toservice-v3 branch 3 times, most recently from 010fbef to 7ce0a19 Compare December 9, 2019 17:52

sharidas force-pushed the move-createuser-toservice-v3 branch from 7ce0a19 to 78c508d Compare December 10, 2019 05:54

karakayasemi requested a review from micbar December 10, 2019 12:44

sharidas force-pushed the move-createuser-toservice-v3 branch from 78c508d to 0441002 Compare December 10, 2019 16:09

jvillafanez reviewed Dec 11, 2019

View reviewed changes

karakayasemi force-pushed the move-createuser-toservice-v3 branch from 0441002 to 5cc92c9 Compare December 12, 2019 18:44

karakayasemi force-pushed the move-createuser-toservice-v3 branch 2 times, most recently from 988427d to 19c6610 Compare December 12, 2019 20:11

karakayasemi changed the title ~~Move user creation to a separate service~~ [WIP] Move user creation to a separate service Dec 12, 2019

karakayasemi added 2 - Developing and removed 3 - To Review labels Dec 12, 2019

karakayasemi force-pushed the move-createuser-toservice-v3 branch 3 times, most recently from 6e73274 to 37b3e3a Compare December 13, 2019 14:21

karakayasemi reviewed Dec 18, 2019

View reviewed changes

core/Controller/UserController.php Show resolved Hide resolved

core/Controller/UserController.php Show resolved Hide resolved

core/Controller/UserController.php Show resolved Hide resolved

karakayasemi reviewed Dec 18, 2019

View reviewed changes

lib/private/User/Service/UserSendMailService.php Show resolved Hide resolved

karakayasemi force-pushed the move-createuser-toservice-v3 branch 2 times, most recently from 97eeafc to 0c923fa Compare December 18, 2019 19:09

karakayasemi requested a review from jvillafanez December 18, 2019 20:17

jvillafanez reviewed Dec 19, 2019

View reviewed changes

karakayasemi force-pushed the move-createuser-toservice-v3 branch from 0c923fa to 99aff11 Compare December 20, 2019 12:27

sharidas force-pushed the move-createuser-toservice-v3 branch from 99aff11 to 81f561d Compare January 2, 2020 14:26

Move user creation to a separate service

0dd2a2e

Move user creation to a separate service. Signed-off-by: Sujith H <[email protected]>

sharidas force-pushed the move-createuser-toservice-v3 branch from 81f561d to 2997476 Compare January 7, 2020 07:26

karakayasemi force-pushed the move-createuser-toservice-v3 branch from 2997476 to b32c813 Compare January 8, 2020 11:36

Do not change the old behavior of user creation

76c356c

karakayasemi force-pushed the move-createuser-toservice-v3 branch from b32c813 to 76c356c Compare January 8, 2020 11:48

karakayasemi added 3 - To Review and removed 2 - Developing labels Jan 17, 2020

DeepDiver1975 closed this Jan 4, 2023

DeepDiver1975 deleted the move-createuser-toservice-v3 branch January 4, 2023 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Move user creation to a separate service #36145

[WIP] Move user creation to a separate service #36145

sharidas commented Sep 3, 2019

sharidas commented Sep 3, 2019

codecov bot commented Dec 6, 2019 •

edited

Loading

karakayasemi commented Dec 7, 2019 •

edited

Loading

phil-davis left a comment

phil-davis commented Dec 9, 2019

karakayasemi commented Dec 9, 2019

sharidas commented Dec 10, 2019 •

edited

Loading

phil-davis commented Dec 10, 2019

karakayasemi commented Dec 18, 2019 •

edited

Loading

jvillafanez Dec 19, 2019

karakayasemi Dec 19, 2019

jvillafanez Dec 19, 2019

karakayasemi Dec 19, 2019

jvillafanez Dec 19, 2019

jvillafanez Dec 19, 2019

sharidas Jan 2, 2020

jvillafanez Dec 19, 2019

sharidas Jan 7, 2020

DeepDiver1975 commented Jan 4, 2023

[WIP] Move user creation to a separate service #36145

[WIP] Move user creation to a separate service #36145

Conversation

sharidas commented Sep 3, 2019

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

sharidas commented Sep 3, 2019

codecov bot commented Dec 6, 2019 • edited Loading

Codecov Report

karakayasemi commented Dec 7, 2019 • edited Loading

phil-davis left a comment

Choose a reason for hiding this comment

phil-davis commented Dec 9, 2019

karakayasemi commented Dec 9, 2019

sharidas commented Dec 10, 2019 • edited Loading

phil-davis commented Dec 10, 2019

karakayasemi commented Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepDiver1975 commented Jan 4, 2023

codecov bot commented Dec 6, 2019 •

edited

Loading

karakayasemi commented Dec 7, 2019 •

edited

Loading

sharidas commented Dec 10, 2019 •

edited

Loading

karakayasemi commented Dec 18, 2019 •

edited

Loading