Skip to content

Public API to get user acocunt data #11740

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
Merged
rullzer merged 4 commits into from
Oct 15, 2018
Merged

Public API to get user acocunt data #11740

rullzer merged 4 commits into from
Oct 15, 2018

Conversation

@juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Oct 10, 2018 •
edited
Loading

This adds a readonly API to public namespace to allow apps to access the user account data.

Todo:

  • Add tests

@juliusknorr juliusknorr added this to the Nextcloud 15 milestone Oct 10, 2018
@juliusknorr juliusknorr mentioned this pull request Oct 10, 2018
Closed
@juliusknorr juliusknorr force-pushed the feature/noid/account-api branch from 0055170 to 7982943 Compare October 10, 2018 14:40
ChristophWurst
ChristophWurst reviewed Oct 10, 2018
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.

I know, I know, this is not labeled to be reviewed yet, but I couldn't resist to give feedback 😉

lib/private/Accounts/Account.php Outdated
use OCP\Accounts\IAccountProperty;
use OCP\IUser;

class Account implements IAccount, \JsonSerializable {
Copy link
Member

@ChristophWurst ChristophWurst Oct 10, 2018

Choose a reason for hiding this comment

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

FYI: if the JSON serialization should also be available for apps, the public interface should extend JsonSerializable

Copy link
Member Author

@juliusknorr juliusknorr Oct 10, 2018

Choose a reason for hiding this comment

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

Yes, makes sense to have it in the interface of course.

@juliusknorr juliusknorr force-pushed the feature/noid/account-api branch from 7982943 to d395235 Compare October 10, 2018 17:25
@juliusknorr
Copy link
Member Author

juliusknorr commented Oct 10, 2018

I know, I know, this is not labeled to be reviewed yet, but I couldn't resist to give feedback

Totally fine for me 😉

rullzer
rullzer previously requested changes Oct 10, 2018
View reviewed changes
@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 11, 2018
@juliusknorr
Copy link
Member Author

juliusknorr commented Oct 11, 2018

Ready for review.

rullzer
rullzer reviewed Oct 11, 2018
View reviewed changes
ChristophWurst
ChristophWurst requested changes Oct 11, 2018
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.

Looks good otherwise :)

lib/private/Accounts/Account.php Outdated
}

public function getFilteredProperties(string $scope = null, string $verified = null): array {
return \array_filter($this->properties, function($obj) use ($scope, $verified){
Copy link
Member

@ChristophWurst ChristophWurst Oct 11, 2018

Choose a reason for hiding this comment

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

you can typehint the $obj type here

lib/private/Accounts/AccountProperty.php Outdated

public function jsonSerialize() {
return [
'value' => $this->value,
Copy link
Member

@ChristophWurst ChristophWurst Oct 11, 2018

Choose a reason for hiding this comment

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

name missing. was that intentional?

Copy link
Member Author

@juliusknorr juliusknorr Oct 11, 2018

Choose a reason for hiding this comment

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

The name is always available from the data given by the Account class, but yes, it makes sense to have it in the serialized array as well.

@juliusknorr juliusknorr force-pushed the feature/noid/account-api branch from a664817 to 63ae96c Compare October 11, 2018 13:41
rullzer
rullzer approved these changes Oct 11, 2018
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Cool stuff!

ChristophWurst
ChristophWurst approved these changes Oct 12, 2018
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.

Looks great 👍

I'd appreciate if you could squash your commits :)

@juliusknorr juliusknorr force-pushed the feature/noid/account-api branch from 63ae96c to aacec01 Compare October 12, 2018 07:31
@juliusknorr
Copy link
Member Author

juliusknorr commented Oct 12, 2018

@ChristophWurst Squashed them 😉

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 12, 2018
@MorrisJobke

This comment has been minimized.

Sign in to view
@juliusknorr juliusknorr force-pushed the feature/noid/account-api branch from aacec01 to ea75eab Compare October 12, 2018 09:17
nickvergessen
nickvergessen reviewed Oct 12, 2018
View reviewed changes
tests/lib/Accounts/AccountPropertyTest.php Outdated

public function testConstructor() {
$accountProperty = new AccountProperty(
AccountManager::PROPERTY_WEBSITE,
Copy link
Member

@nickvergessen nickvergessen Oct 12, 2018

Choose a reason for hiding this comment

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

should use them from IAccountManager to make sure they are not removed there?

Copy link
Member

@MorrisJobke MorrisJobke Oct 15, 2018

Choose a reason for hiding this comment

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

@juliushaertl Mind to look into this final comment and then we can merge this. 😉

Copy link
Member Author

@juliusknorr juliusknorr Oct 15, 2018

Choose a reason for hiding this comment

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

@nickvergessen Done.

@juliusknorr
Add \OCP\Account public API
d05080f 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Add tests for new account api classes
9381e68 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Update autoloader
90cdd0a 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Use IAccountManager constants
b9a87a6 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the feature/noid/account-api branch from ea75eab to b9a87a6 Compare October 15, 2018 11:30
@juliusknorr
Copy link
Member Author

juliusknorr commented Oct 15, 2018

Failing tests unrelated:

sh: 1: kill: No such process

@rullzer rullzer merged commit 5aaa8a8 into master Oct 15, 2018
@rullzer rullzer deleted the feature/noid/account-api branch October 15, 2018 19:46
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

@MorrisJobke MorrisJobke MorrisJobke left review comments

@rullzer rullzer rullzer approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@schiessle schiessle Awaiting requested review from schiessle

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement

Projects

None yet

Milestone

Nextcloud 15

Development

Successfully merging this pull request may close these issues.

6 participants

@juliusknorr @MorrisJobke @rullzer @nickvergessen @ChristophWurst