Skip to content

Conversation

@ArtificialOwl
Copy link
Member

@ArtificialOwl ArtificialOwl commented Jun 7, 2017

Add displayName and Avatatar to IShare.

@mention-bot
Copy link

@daita, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rullzer, @blizzz and @schiessle to be potential reviewers.

@ArtificialOwl
Copy link
Member Author

selection_006

This PR fix nextcloud/circles#78 and allow Circles to display its own avatar

@ArtificialOwl ArtificialOwl added the 3. to review Waiting for reviews label Jun 7, 2017
@ArtificialOwl ArtificialOwl modified the milestone: Nextcloud 12.0.1 Jun 7, 2017
Copy link
Member

@LukasReschke LukasReschke left a comment

Choose a reason for hiding this comment

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

Can I haz some unit tests on the new functions in Share.php? 😇

* Set the display name of the receiver of this share.
*
* @param string $displayName
* @return \OCP\Share\IShare The modified object
Copy link
Member

Choose a reason for hiding this comment

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

13 everywhere

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 I go 12.0.1 ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to backport this 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is wrong on having this on 12.0.1 ?

Copy link
Member

Choose a reason for hiding this comment

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

We only backport serious bug fixes usually 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

Copy link
Member Author

Choose a reason for hiding this comment

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

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 13, 2017
@ArtificialOwl
Copy link
Member Author

you know have few tests @LukasReschke

public function setSharedWithAvatar($src);

/**
* Get the display name of the receiver of this share.
Copy link
Member

Choose a reason for hiding this comment

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

I guess you mean avatar.

@MorrisJobke
Copy link
Member

@daita Should we try to get this in 13 or is 14 fine?

@MorrisJobke MorrisJobke closed this Dec 8, 2017
@MorrisJobke MorrisJobke reopened this Dec 8, 2017
@ArtificialOwl
Copy link
Member Author

14 is fine

@MorrisJobke MorrisJobke modified the milestones: Nextcloud 13, Nextcloud 14 Dec 8, 2017
@MorrisJobke MorrisJobke force-pushed the shared-with-display-name branch from a5568fe to a5b1357 Compare March 7, 2018 14:35
@MorrisJobke
Copy link
Member

Rebased on current master, squashed some commits and updated the @since PHPDoc tags.

@MorrisJobke MorrisJobke dismissed stale reviews from nickvergessen and LukasReschke March 7, 2018 14:35

Fixed

@nextcloud nextcloud deleted a comment from codecov bot Mar 7, 2018
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

The avatars work nicely 👍

@MorrisJobke
Copy link
Member

cc @nextcloud/sharing

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 7, 2018
@MorrisJobke MorrisJobke requested a review from rullzer March 7, 2018 14:53
@schiessle
Copy link
Member

I just tried it... but no avatars. Maybe something got lost during the rebase?

@MorrisJobke
Copy link
Member

@daita Also there is another merge conflict :(

@MorrisJobke
Copy link
Member

Merge conflict caused by 2c073dc - maybe @danxuliu could help to rebase this.

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 3, 2018
@danxuliu danxuliu force-pushed the shared-with-display-name branch from a5b1357 to 836ac8e Compare April 17, 2018 11:27
@codecov
Copy link

codecov bot commented Apr 17, 2018

Codecov Report

Merging #5280 into master will increase coverage by 0.02%.
The diff coverage is 87.87%.

@@             Coverage Diff              @@
##             master    #5280      +/-   ##
============================================
+ Coverage     51.69%   51.71%   +0.02%     
- Complexity    25744    25751       +7     
============================================
  Files          1643     1643              
  Lines         96506    96536      +30     
  Branches       1393     1396       +3     
============================================
+ Hits          49888    49928      +40     
+ Misses        46618    46608      -10
Impacted Files Coverage Δ Complexity Δ
core/js/sharedialogshareelistview.js 49.48% <100%> (+1.06%) 0 <0> (ø) ⬇️
...iles_sharing/lib/Controller/ShareAPIController.php 69.95% <100%> (+1.92%) 161 <0> (+1) ⬆️
core/js/sharedialogresharerinfoview.js 91.42% <50%> (-2.52%) 0 <0> (ø)
core/js/shareitemmodel.js 88.02% <80%> (-0.13%) 0 <0> (ø)
lib/private/Share20/Share.php 93.79% <85.71%> (-0.87%) 63 <6> (+6)
apps/files_trashbin/lib/Trashbin.php 72.7% <0%> (+0.24%) 136% <0%> (ø) ⬇️
core/js/js.js 65.96% <0%> (+0.55%) 0% <0%> (ø) ⬇️

@danxuliu
Copy link
Member

Merge conflict caused by 2c073dc - maybe @danxuliu could help to rebase this.

I have rebased again and fixed the conflict; now the display name provided by the circle share is used and, if none is given, then the old code to extract it from share_with is used.

I just tried it... but no avatars. Maybe something got lost during the rebase?

I have fixed that too; avatars were not shown in the list due to ed4b445, as since that commit modSeed was false when using circle avatars, and thus the imageplaceholderseed CSS class was not set.

Note, however, that the default avatar is still shown in the share dialog; showing the circle avatar there would require adding it in the Circles app and then using it from the dialog.

@danxuliu
Copy link
Member

Oops, tests are failing :-) I will fix them.

Signed-off-by: Maxence Lange <maxence@nextcloud.com>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Maxence Lange <maxence@nextcloud.com>
Signed-off-by: Maxence Lange <maxence@nextcloud.com>
@danxuliu danxuliu force-pushed the shared-with-display-name branch from 836ac8e to 4f5814c Compare May 20, 2018 23:36
@danxuliu danxuliu added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 20, 2018
@danxuliu
Copy link
Member

Oops, tests are failing :-) I will fix them.

I forgot to push... 🤦

Anyway, I made a good use of my slip by adding more unit tests ;-) I have also rebased the commits onto current master.

This pull request should be ready for review again!

@MorrisJobke MorrisJobke added the stale Ticket or PR with no recent activity label Jun 19, 2018
@MorrisJobke
Copy link
Member

@rullzer @danxuliu Mind to give a final review? Or should we push it to 15?

@nextcloud-bot nextcloud-bot removed the stale Ticket or PR with no recent activity label Jun 29, 2018
@MorrisJobke
Copy link
Member

@rullzer @danxuliu Mind to give a final review? Or should we push it to 15?

Ping 🏓

@MorrisJobke
Copy link
Member

@skjnldsv @danxuliu Mind to +1?

@MorrisJobke MorrisJobke merged commit 82021b2 into master Jul 13, 2018
@MorrisJobke MorrisJobke deleted the shared-with-display-name branch July 13, 2018 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants