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.

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

Adding group display name support in group backends #26750

Merged
merged 3 commits into from Dec 9, 2016

Conversation

Projects
None yet
4 participants
@PVince81
Member

PVince81 commented Nov 30, 2016

A step forward for #7918

TODOs

  • Adjust ShareesController to use the display name
    • requires adjusting the search methods to also return display names
    • adjust search to also search in display name
  • Unit tests

@jvillafanez @DeepDiver1975

@PVince81 PVince81 added this to the 9.2 milestone Nov 30, 2016

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Nov 30, 2016

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @bartv2 and @macjohnny to be potential reviewers.

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @bartv2 and @macjohnny to be potential reviewers.

@PVince81 PVince81 changed the title from Adding group display name support to Adding group display name support in group backends Nov 30, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 30, 2016

Member

Hmm, I'm surprised that it already works so far. That was easy.

However adjusting all other pieces of the UI to accomodate for the display name will be more work.

In this PR I'll focus mostly on backend support + share display name.

Member

PVince81 commented Nov 30, 2016

Hmm, I'm surprised that it already works so far. That was easy.

However adjusting all other pieces of the UI to accomodate for the display name will be more work.

In this PR I'll focus mostly on backend support + share display name.

@PVince81 PVince81 referenced this pull request Dec 1, 2016

Merged

Implement group backend #1

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 1, 2016

Member

Now I see that there is actually another mechanism to add new actions, the implementsAction method.

Do we want to keep doing this ? If yes I'd get rid of IGroupBackend and simply add an action getGroup to the list. I personally would prefer if in the future we'd simply have several interfaces and action availability could be checked with instanceof.

  • clarify if we want to use implementsAction for getGroup instead of a new interface IGroupBackend
Member

PVince81 commented Dec 1, 2016

Now I see that there is actually another mechanism to add new actions, the implementsAction method.

Do we want to keep doing this ? If yes I'd get rid of IGroupBackend and simply add an action getGroup to the list. I personally would prefer if in the future we'd simply have several interfaces and action availability could be checked with instanceof.

  • clarify if we want to use implementsAction for getGroup instead of a new interface IGroupBackend
@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Dec 2, 2016

Member

I guess that depends on the number of conditional actions, or how the UI would look like.

Taking the files as an example, each file might show a different set of actions: delete the file, rename the file, share the file... the UI would need to know what actions are available for the file before executing that action to show the corresponding button (if the delete action isn't supported, why would we need to show the button?)
Although this can be implemented by using interfaces, the number of interfaces that we'd need to implement is very high, so this doesn't seem a good approach in this case.

On the other hand, for this case we could have 2 types of group backends: read-only (such as LDAP) and read-write (the default one). If this is the case, using interfaces might be an option, but we MUST consider if all the possible backends would fit in any of those groups.
Notice that with this approach there won't be a backend that might implement only a small action set; it would be all or nothing.

Member

jvillafanez commented Dec 2, 2016

I guess that depends on the number of conditional actions, or how the UI would look like.

Taking the files as an example, each file might show a different set of actions: delete the file, rename the file, share the file... the UI would need to know what actions are available for the file before executing that action to show the corresponding button (if the delete action isn't supported, why would we need to show the button?)
Although this can be implemented by using interfaces, the number of interfaces that we'd need to implement is very high, so this doesn't seem a good approach in this case.

On the other hand, for this case we could have 2 types of group backends: read-only (such as LDAP) and read-write (the default one). If this is the case, using interfaces might be an option, but we MUST consider if all the possible backends would fit in any of those groups.
Notice that with this approach there won't be a backend that might implement only a small action set; it would be all or nothing.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 3, 2016

Member

Yeah, I know... Ok, I guess I'll just create a new action "has function to get group details (including display name" instead of a brand new interface.

The trouble with "implementsAction" is that there is no interface to enforce method signatures, and also no way to document these functions.

Member

PVince81 commented Dec 3, 2016

Yeah, I know... Ok, I guess I'll just create a new action "has function to get group details (including display name" instead of a brand new interface.

The trouble with "implementsAction" is that there is no interface to enforce method signatures, and also no way to document these functions.

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Dec 5, 2016

Member

I wonder if a "command pattern" would fit here (for reference: https://en.wikipedia.org/wiki/Command_pattern). The conditional methods would return a Command object (according to the pattern, not a Symfony Command object) to be run by the manager, or null if it doesn't implement the method.

So, instead of "backend, run this method" would be "backend, I want to run this, get me the method to do so".

Member

jvillafanez commented Dec 5, 2016

I wonder if a "command pattern" would fit here (for reference: https://en.wikipedia.org/wiki/Command_pattern). The conditional methods would return a Command object (according to the pattern, not a Symfony Command object) to be run by the manager, or null if it doesn't implement the method.

So, instead of "backend, run this method" would be "backend, I want to run this, get me the method to do so".

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 5, 2016

Member

Hmm not sure, command pattern might sound like overkill. Command pattern is usually better for stuff like real actions, file actions, etc. But here most of the time it's not really an action, it's just reading data from the DB.

Member

PVince81 commented Dec 5, 2016

Hmm not sure, command pattern might sound like overkill. Command pattern is usually better for stuff like real actions, file actions, etc. But here most of the time it's not really an action, it's just reading data from the DB.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 5, 2016

Member

I guess it's probably better to not invent new ways right now and have half the code use the new way and the half use the old way. I'll use the old way here with "implementsAction".

Member

PVince81 commented Dec 5, 2016

I guess it's probably better to not invent new ways right now and have half the code use the new way and the half use the old way. I'll use the old way here with "implementsAction".

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 5, 2016

Member

Argh... now I ported everything to implementsAction and I find that it's not possible to mock the new method without having a real class or interface implementing it...

Member

PVince81 commented Dec 5, 2016

Argh... now I ported everything to implementsAction and I find that it's not possible to mock the new method without having a real class or interface implementing it...

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 5, 2016

Member

Fixed, added unit tests.

I think this is ready for review @jvillafanez @DeepDiver1975

Member

PVince81 commented Dec 5, 2016

Fixed, added unit tests.

I think this is ready for review @jvillafanez @DeepDiver1975

Show outdated Hide outdated apps/files_sharing/lib/Controller/ShareesController.php
if (!in_array($gid, $groupIds)) {
continue;
}
if (strtolower($gid) === strtolower($search) || strtolower($group->getDisplayName()) === strtolower($search)) {

This comment has been minimized.

@jvillafanez

jvillafanez Dec 5, 2016

Member

convert $search to lowercase outside the loop

@jvillafanez

jvillafanez Dec 5, 2016

Member

convert $search to lowercase outside the loop

This comment has been minimized.

@PVince81

PVince81 Dec 5, 2016

Member

when I touched this line I KNEW you would say that 😉

@PVince81

PVince81 Dec 5, 2016

Member

when I touched this line I KNEW you would say that 😉

This comment has been minimized.

@jvillafanez

jvillafanez Dec 7, 2016

Member

looks like you've changed it in another place but not here 😄

@jvillafanez

jvillafanez Dec 7, 2016

Member

looks like you've changed it in another place but not here 😄

This comment has been minimized.

@PVince81

PVince81 Dec 7, 2016

Member

Oaaargghhhhhhhhhhhhhhh

@PVince81

PVince81 Dec 7, 2016

Member

Oaaargghhhhhhhhhhhhhhh

This comment has been minimized.

@PVince81

PVince81 Dec 7, 2016

Member

Indeed, I was so lazy that I searched for strtolower and landed somewhere else... I'll fix all occurences then

@PVince81

PVince81 Dec 7, 2016

Member

Indeed, I was so lazy that I searched for strtolower and landed somewhere else... I'll fix all occurences then

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 5, 2016

Member

@jvillafanez fixed now.

Member

PVince81 commented Dec 5, 2016

@jvillafanez fixed now.

PVince81 and others added some commits Nov 30, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 7, 2016

Member

Fixed all the strtolower now, rebased and squashed for a clean history

Member

PVince81 commented Dec 7, 2016

Fixed all the strtolower now, rebased and squashed for a clean history

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Dec 7, 2016

Member

👍

Member

jvillafanez commented Dec 7, 2016

👍

@PVince81 PVince81 merged commit 08da9a8 into master Dec 9, 2016

4 checks passed

Scrutinizer 9 new issues, 3 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the group-displaynames branch Dec 9, 2016

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