Fix/661/additional attributes #729

nc-fkl · 2023-12-07T14:20:43Z

Added multiple attributes
Aligned mapping
Aligned frontend layout a littlebit so that its more categorized
Setting newly added properties of account via IAccountManager instead of IUser

julien-nc

Nice!
Unit and integration tests don't pass.
I'll make real life tests when they do.

julien-nc · 2023-12-07T15:19:38Z

lib/Service/ProvisioningService.php

 		$event = new AttributeMappedEvent(ProviderService::SETTING_MAPPING_UID, $idTokenPayload, $tokenUserId);
 		$this->eventDispatcher->dispatchTyped($event);

-		$backendUser = $this->userMapper->getOrCreate($providerId, $event->getValue());
+		$backendUser = $this->userMapper->getOrCreate($providerId, (string)$event->getValue());


Do you cast to get an empty string if the value is null? If so, it would require a comment and do it like:

Suggested change

$backendUser = $this->userMapper->getOrCreate($providerId, (string)$event->getValue());

$backendUser = $this->userMapper->getOrCreate($providerId, $event->getValue() ?? '');

Actualy my ide marked it as error that's not casted, that was the only reason for that :) took your solution now

julien-nc · 2023-12-07T15:25:18Z

lib/Service/ProvisioningService.php

+		// filter out empty values
+		$addressParts = array_filter($addressParts, function($value) {
+			return !empty($value);
+		});
+		// concatenate them back together into a string and remove unsused ', '
+		$address = preg_replace('/^, |, $/', '', str_replace('  ', ' ', implode(', ', $addressParts)));


I think if you have empty parts and you remove them, the result string will be in the wrong format. If the street is missing, then the postal code will be seen as the street. I think we need a fixed number of fields as they are identified by their position. No problem with empty ones.

Ah good point, makes sense, so lets keep the "ugly" string when a value was not provided, will also be a good hint for the user/admin itself that a value isn't set correctly.

julien-nc

Check the unit tests output. They need to be adjusted with the new setting values.

julien-nc · 2023-12-08T14:52:47Z

lib/Command/UpsertProvider.php

+		if ($mapping = $input->getOption('mapping-groups')) {
+			$this->providerService->setSetting($provider->getId(), ProviderService::SETTING_MAPPING_GROUPS, $mapping);
+		}


You forgot to add this option in the configure method. This will make the integration tests pass.

nc-fkl · 2023-12-11T14:38:37Z

I guess everything should be fixed and fine now, can you please review again @julien-nc ? Thx

julien-nc

Nice, i tried with a few attributes, works like a charm.

It would be nice to hide all those setting fields and make them appear/disappear if user clicks on a "Extra attributes mapping" button.

Also, now there are conflicts with main. Nothing weird, the new options in the UpSert command are added on the same line. Let me know if you need help to solve the conflicts.

Signed-off-by: Florian Klinger <[email protected]>

…justed Signed-off-by: Florian Klinger <[email protected]>

Signed-off-by: Florian Klinger <[email protected]>

nc-fkl · 2023-12-18T12:59:44Z

Nice, i tried with a few attributes, works like a charm.

It would be nice to hide all those setting fields and make them appear/disappear if user clicks on a "Extra attributes mapping" button.

Also, now there are conflicts with main. Nothing weird, the new options in the UpSert command are added on the same line. Let me know if you need help to solve the conflicts.

Rebased and added expandable/collapsable extra attributes. Would be nice if you can review again 🏓 , thank you @julien-nc :)

Signed-off-by: Julien Veyssier <[email protected]>

julien-nc

Woop woop

github-actions · 2024-04-24T02:06:23Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

julien-nc requested changes Dec 7, 2023

View reviewed changes

nc-fkl requested a review from julien-nc December 7, 2023 17:11

julien-nc requested changes Dec 8, 2023

View reviewed changes

nc-fkl force-pushed the fix/661/additional-attributes branch from d51d4b9 to fd0ef87 Compare December 11, 2023 09:09

nc-fkl requested a review from julien-nc December 11, 2023 14:38

julien-nc requested changes Dec 13, 2023

View reviewed changes

nc-fkl added 11 commits December 18, 2023 10:17

Added posibility to add additional attributes

1a0db0a

Signed-off-by: Florian Klinger <[email protected]>

mini fix - duplicated element id

bb5e03f

Signed-off-by: Florian Klinger <[email protected]>

Added attribute mappings, aligned fronted, added multiple attributes

9219ab3

Signed-off-by: Florian Klinger <[email protected]>

Lint fixes, review fixes, locality mapping typo fix, frontend text ad…

af26fae

…justed Signed-off-by: Florian Klinger <[email protected]>

Casting to empty value - lint fix

a520460

Signed-off-by: Florian Klinger <[email protected]>

lint fix for php7.4

12ef673

Signed-off-by: Florian Klinger <[email protected]>

Added missing mapping-group option

d5edd9c

Signed-off-by: Florian Klinger <[email protected]>

Added missing values for unittests

05228d2

Signed-off-by: Florian Klinger <[email protected]>

Added missing defaults values for unittest

4c784d4

Signed-off-by: Florian Klinger <[email protected]>

Fix lint - indents

7677036

Signed-off-by: Florian Klinger <[email protected]>

Rebase main and expandable/collapsable extra attributes in the frontend

9ea902d

Signed-off-by: Florian Klinger <[email protected]>

nc-fkl force-pushed the fix/661/additional-attributes branch from c6191fb to 9ea902d Compare December 18, 2023 12:52

nc-fkl requested a review from julien-nc December 18, 2023 12:59

use chevron icons for expandable extra attribute list

b7cc45f

Signed-off-by: Julien Veyssier <[email protected]>

julien-nc approved these changes Dec 18, 2023

View reviewed changes

julien-nc merged commit 9a5e996 into main Dec 18, 2023
41 checks passed

julien-nc deleted the fix/661/additional-attributes branch December 18, 2023 13:11

nc-fkl linked an issue Jan 3, 2024 that may be closed by this pull request

Add commandline argument to set mapping for groups #744

Closed

julien-nc mentioned this pull request Jan 29, 2024

Prepare v1.3.6 #777

Merged

github-actions bot added the feedback-requested label Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/661/additional attributes #729

Fix/661/additional attributes #729

nc-fkl commented Dec 7, 2023

julien-nc left a comment

julien-nc Dec 7, 2023

nc-fkl Dec 7, 2023

julien-nc Dec 7, 2023

nc-fkl Dec 7, 2023

julien-nc left a comment

julien-nc Dec 8, 2023

nc-fkl commented Dec 11, 2023

julien-nc left a comment

nc-fkl commented Dec 18, 2023 •

edited

Loading

julien-nc left a comment

github-actions bot commented Apr 24, 2024

	$backendUser = $this->userMapper->getOrCreate($providerId, (string)$event->getValue());
	$backendUser = $this->userMapper->getOrCreate($providerId, $event->getValue() ?? '');

Fix/661/additional attributes #729

Fix/661/additional attributes #729

Conversation

nc-fkl commented Dec 7, 2023

julien-nc left a comment

Choose a reason for hiding this comment

julien-nc Dec 7, 2023

Choose a reason for hiding this comment

nc-fkl Dec 7, 2023

Choose a reason for hiding this comment

julien-nc Dec 7, 2023

Choose a reason for hiding this comment

nc-fkl Dec 7, 2023

Choose a reason for hiding this comment

julien-nc left a comment

Choose a reason for hiding this comment

julien-nc Dec 8, 2023

Choose a reason for hiding this comment

nc-fkl commented Dec 11, 2023

julien-nc left a comment

Choose a reason for hiding this comment

nc-fkl commented Dec 18, 2023 • edited Loading

julien-nc left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 24, 2024

nc-fkl commented Dec 18, 2023 •

edited

Loading