Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Always set displayname and display-name for user data #36665

Merged
merged 1 commit into from
Mar 22, 2023
Merged

Always set displayname and display-name for user data #36665

merged 1 commit into from
Mar 22, 2023

Conversation

provokateurin
Copy link
Member

Summary

Always set both fields to make the returned data the same regardless of the endpoint.

Checklist

@provokateurin
Always set displayname and display-name for user data
77f7da9 
Signed-off-by: jld3103 <jld3103yt@gmail.com>
@provokateurin provokateurin mentioned this pull request Feb 10, 2023
Closed
8 tasks
@szaimen szaimen added this to the Nextcloud 26 milestone Feb 11, 2023
@szaimen szaimen added the 3. to review Waiting for reviews label Feb 11, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
Merged
@blizzz blizzz mentioned this pull request Mar 7, 2023
Merged
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 7, 2023
ChristophWurst
ChristophWurst approved these changes Mar 17, 2023
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Needs a doc update at https://docs.nextcloud.com/server/latest/developer_manual/client_apis/OCS/ocs-api-overview.html#user-metadata

@ChristophWurst ChristophWurst added integration pending documentation This pull request needs an associated documentation update labels Mar 17, 2023
@provokateurin provokateurin mentioned this pull request Mar 21, 2023
Merged
ShGKme added a commit to nextcloud/talk-desktop that referenced this pull request Mar 22, 2023
@ShGKme
fix: fix wrong display-name property from provisioning getUser response
ba2a2f9 
- Fix: #70
- Related: nextcloud/server#36665

Signed-off-by: Grigorii Shartsev <grigorii.shartsev@nextcloud.com>
@ShGKme ShGKme mentioned this pull request Mar 22, 2023
Merged
1 task
ShGKme
ShGKme approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Tested on my server, works as expected 🐸

@julien-nc
Copy link
Member

Drone failures are unrelated.

@julien-nc julien-nc merged commit c838bdb into master Mar 22, 2023
@julien-nc julien-nc deleted the fix/36539 branch March 22, 2023 14:21
@nickvergessen
Copy link
Member

grafik

@nickvergessen
Copy link
Member

Drone failures ARE TOTALLY related. Please fix asap to restore green CI

julien-nc added a commit that referenced this pull request Mar 23, 2023
@julien-nc
adjust tests after #36665
f70aa9f 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
julien-nc added a commit that referenced this pull request Mar 23, 2023
@julien-nc
adjust tests after #36665
e97abb7 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
julien-nc added a commit that referenced this pull request Mar 23, 2023
@julien-nc
adjust tests after #36665
69a1fe2 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc mentioned this pull request Mar 23, 2023
Merged
nickvergessen added a commit that referenced this pull request Mar 23, 2023
@nickvergessen
Merge pull request #37361 from nextcloud/fix/noid/display-name-users-…
553b5a5 
…controller-test

Adjust provisioning API tests after #36665
nickvergessen
nickvergessen reviewed Mar 26, 2023
View reviewed changes
lib/public/Accounts/IAccountManager.php
@@ -106,6 +107,7 @@ interface IAccountManager {

public const PROPERTY_AVATAR = 'avatar';
public const PROPERTY_DISPLAYNAME = 'displayname';
public const PROPERTY_DISPLAYNAME_LEGACY = 'display-name';
Copy link
Member

Choose a reason for hiding this comment

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

Please document in #37039

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a non-breaking backwards compatible change, but it should still be documented?

Copy link
Member

Choose a reason for hiding this comment

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

Basically anything touching OCP should be documented, so people are aware it now exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you tell me how I should write it down? I'm not sure exactly what I should write

Copy link
Member

Choose a reason for hiding this comment

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

In the Backend section just write a new bullet point:

  • OCP\Accounts\IAccounManager::PROPERTY_DISPLAYNAME_LEGACY was added to be used for …

And then leave a comment with your changes, so that people that are subscribed to the ticket know that the list was modified.

@provokateurin provokateurin mentioned this pull request Mar 27, 2023
Closed
@DaphneMuller
Copy link

Hello!
Thank you for your work on this! I noticed that your PR still has the label 'documentation'. I'm not sure if you already had a look at the documentation for your PR, but at Nextcloud we strive to document changes that affect other app developers or other admins before the release takes place, and I'm pinging you with the hope to get this done.

Are you familiar with our documentation process already?
Changes that affect app developers should be documented here:
https://docs.nextcloud.com/server/27/developer_manual/app_publishing_maintenance/app_upgrade_guide/upgrade_to_27.html

Changes that affect administrators should be documented here:
https://docs.nextcloud.com/server/latest/admin_manual/release_notes/upgrade_to_27.html

While I understand sometimes it's handy to merge fast to get your changes in, it would be great if next time you could try to add the documentation before merging. You can find more information on that here:
https://docs.nextcloud.com/server/27/developer_manual/prologue/compatibility_app_ecosystem.html#documentation-procedures-of-changes-that-affect-app-developers

If all your documentation efforts are done, please remove the label 'documentation' and check the checkbox in your opening note and I'll stop bugging you :)

Many thanks in advance and thanks again for your work!

@provokateurin provokateurin removed the pending documentation This pull request needs an associated documentation update label May 30, 2023
@provokateurin
Copy link
Member Author

Documentation was done, label was wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@ShGKme ShGKme ShGKme approved these changes

@julien-nc julien-nc Awaiting requested review from julien-nc

Assignees
No one assigned
Labels
3. to review Waiting for reviews integration
Projects
None yet
Milestone
Nextcloud 27
Development

Successfully merging this pull request may close these issues.

[Bug]: provisioning_api current user display name field
8 participants
@provokateurin @julien-nc @nickvergessen @DaphneMuller @ChristophWurst @ShGKme @blizzz @szaimen