Add profile data listing and editing to occ command

[salmart-dev]

• Resolves: #49744

Summary

This PR adds the ability to list, edit or delete profile properties through the occ command.

Before:

After:

TODO

• [x] Update tests once the solution is approved
• [ ] Open new issues for the encountered problems in the command
• [x] Check if documentation and/or wiki needs update
• [x] Verify correctness of occ user:setting [uid] settings and decide whether to open a new ticket or fix in this context.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• [x] Tests (unit, integration, api and/or acceptance) are included
• [x] Screenshots before/after for front-end changes
• [x] Documentation (manuals or wiki) has been updated or is not required
• [ ] Backports requested where applicable (ex: critical bugfixes)

[salmart-dev]

Hi @provokateurin thank you for your review!

I'm sorry my first commit was hard to review. I wanted to keep the refactoring in one commit, but I see that it would've been better to split the changes more, at least for the sake of the review. If you have other suggestions for me about this, please let me know!

Regrding the fixes, do you prefer separate commits per item to be fixed, and then squash them with the relevant commits once the review is over, or something else?

Also, while reading and implementing my changes, I noticed a few oddities and I'm not sure whether those should be discussed in this PR or in another place:

• ~in the same file, inputting the argument --default-value doesn't seem to do anything but print it back, as can be seen in this line~
• passing a non-existing user and the ignore missing users option in combination with a display name value, allows writing the display name into the configuration database instead of using the alternative code that would store it into the user's account information instead. I wonder if the current implementation is correct or should be fixed here too.
• filtering for a specific app (e.g. occ user:setting admin login) always returns the settings properties if the display_name is set.

What do you think?

[provokateurin]

I'm sorry my first commit was hard to review. I wanted to keep the refactoring in one commit, but I see that it would've been better to split the changes more, at least for the sake of the review. If you have other suggestions for me about this, please let me know!

Like I said it's not that bad, especially given how bad the code was before. Sometimes rewriting is the only solution to make it clean.

Regrding the fixes, do you prefer separate commits per item to be fixed, and then squash them with the relevant commits once the review is over, or something else?

Ideally you only do fixup commits, so the reviewers can easily check what you changed, and then squash once at the end when everything was approved and is ready to be merged.

Also, while reading and implementing my changes, I noticed a few oddities and I'm not sure whether those should be discussed in this PR or in another place:

You could fix them here, but that might a bit too much. If you don't mind, please open some issues about your findings so we can fix them later.

[salmart-dev]

@provokateurin I finally managed to finish and push the updated/new tests 🎉

I decided to edit the get/list ones, since the test code would be not too complex, but added new versions for the set and delete ones since adding the new case to the old tests would make them a bit too complex and way harder to read.

Regarding my previous comment about the --default-value not working, I had a big 🤦 moment when I realised that it does exactly what it is supposed to and that I just completely missed its meaning (probably thanks to having fever).

[salmart-dev]

Hey @provokateurin thanks again for the loops of review!

I addressed everything up to your latest comments but I noticed that the fix to avoid returning the "settings" values is still in the branch. I want to confirm that I should remove it, correct? That would mean removing the if statement from the following code but keeping the assignment (Setting.php:302).

if (!$app || $app === 'settings') {
	// Only add the display name if the user exists
	$settings['settings']['display_name'] = $user->getDisplayName();
}

[provokateurin]

I noticed that the fix to avoid returning the "settings" values is still in the branch. I want to confirm that I should remove it, correct?

TBH I'm not knowledgeable about how it is supposed to work. I would think the check makes sense, but maybe you can elaborate a bit why you think it should be removed.

[salmart-dev]

TBH I'm not knowledgeable about how it is supposed to work. I would think the check makes sense, but maybe you can elaborate a bit why you think it should be removed.

I would love to keep the fix, since it leads to a more correct output for the occ function, but I don't want to skip the process of opening a new ticket and fixing it in that context if not doing that would be a problem in any way. And since you mentioned earlier that fixing the issues I reported in this branch would be too much, I was asking if I should remove the fix or not.

[github-actions[bot]]

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!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

[provokateurin]

Nice stuff! :tada:

[salmart-dev]

@provokateurin I adapted the getStoredValue, but I also had to add some handling to prevent reading, editing and deleting of properties that overlap between the "settings" and "profile" apps 🫠

[salmart-dev]

I fear that you’re gonna break ignore-missing-user option. You test in several places if the user exists despite the option value. Also in some places you use the user object from the userManager without testing its value first. And the test on $user is not always the same, it’s inconsistent, you have if (!$user) on one occasion and instanceof tests in the others.

It would be cleaner to get the user object early and pass it around, with a clear behavior when it’s null. I was not able to test the branch right now, but I’ll try to do that later.

Ok, I'll adapt the code to not fail when trying to edit a non-existing user and check how comes I was not testing for the user not being null.

[salmart-dev]

I fear that you’re gonna break ignore-missing-user option. You test in several places if the user exists despite the option value. Also in some places you use the user object from the userManager without testing its value first. And the test on $user is not always the same, it’s inconsistent, you have if (!$user) on one occasion and instanceof tests in the others.

It would be cleaner to get the user object early and pass it around, with a clear behavior when it’s null. I was not able to test the branch right now, but I’ll try to do that later.

What would be the expected behaviour when using --ignore-missing-user in the case of profile properties? Setting the email, display_name and any property that can be set on the profile view of a user needs a user account from my understanding, so they cannot be added or edited if the user doesn't exist.

For example, in the implementation that existed for email and display_name if the user doesn't exist, instead of using the custom handling, the code inserts data using the IConfig interface which from my limited understanding would be wrong behaviour.

[salmart-dev]

@provokateurin under suggestion from @come-nc, I moved the profile settings under occ user:profile for cleaner logic and separation of what is a config value and what is profile/account data.

The latest push has the following changes:

• rebased the branch on top of master
• undid the changes in Setting and SettingTest
• moved the new logic and tests to Profile and ProfileTest

The fixes for the issues with occ user:setting setting have also been undone. To avoid more churning of this PR I would avoid adding them back.

[salmart-dev]

Rebased the branch on master and squashed all commits that could be squashed.

[salmart-dev]

Merged in https://github.com/nextcloud/server/pull/53540