Add search to @sharing #316

jaroel · 2017-04-18T14:21:11Z

No description provided.

coveralls · 2017-04-18T14:33:29Z

Coverage decreased (-0.02%) to 96.231% when pulling 2239d0c on sharing into 3b2c9e9 on master.

tisto · 2017-04-18T16:25:38Z

@jaroel so the idea would be that you can query @sharing on the portal root instead of the @Principals endpoint, correct?

FYI: Even though we are still in alpha, I would prefer to deprecate a method first, before finally removing it at a later version. People are already using plone.restapi in production. Unlike Google/Angular, we take breaking changes seriously. ;)

tisto · 2017-04-18T16:27:37Z

docs/source/sharing.rst

@@ -35,6 +35,9 @@ The sharing information of a content object can also be directly accessed by app
 .. literalinclude:: _json/sharing_folder_get.resp
   :language: http

+.. note::
+    Searching for users and/or groups who do not yet have a sharing entry can be done by appending the argument `search_term` to the query string. ie search_term=admin


@jaroel I would prefer to use "search" instead of "search_term" to be consistent with the other endpoints.

tisto · 2017-04-18T16:30:22Z

@jaroel is it possible that you forgot to add the "@Principals" search implementation? Or would that be another pr?

jaroel · 2017-04-19T06:11:24Z

@tisto you'll use @sharing on context, so the results will include the correct acquired and inherited flags. @Principals never supported this and never will, unless it is called on context.
Yeah, @Principals should be deprecated first.
I'll make it so that we can use "search" instead of "search_term".

coveralls · 2017-04-19T09:56:25Z

Coverage increased (+0.2%) to 96.482% when pulling 25a6c60 on sharing into 3b2c9e9 on master.

coveralls · 2017-04-19T10:07:28Z

Coverage increased (+0.2%) to 96.482% when pulling 25a6c60 on sharing into 3b2c9e9 on master.

coveralls · 2017-04-19T10:23:38Z

Coverage increased (+0.2%) to 96.482% when pulling 80fb01c on sharing into 3b2c9e9 on master.

jaroel · 2017-04-19T11:39:39Z

@tiso I've reverted removing @Principals. Could you reviews this again?

tisto · 2017-04-19T20:44:29Z

@sneridagh if you have time I would like to hear your opinion on that pull request.

tisto

PR looks good to me and the feature is desirable. We need to discuss the issues I raised in the review though.

tisto · 2017-04-20T18:05:55Z

docs/source/sharing.rst

@@ -35,6 +35,9 @@ The sharing information of a content object can also be directly accessed by app
 .. literalinclude:: _json/sharing_folder_get.resp
   :language: http

+.. note::
+    Users and/or groups without a sharing entry can be found by appending the argument `search` to the query string. ie search=admin. It will also flag or global roles.


@jaroel what does "It will also flag or global roles" mean?

Right, that's just stupid.

tisto · 2017-04-20T18:07:16Z

src/plone/restapi/serializer/local_roles.py

        sharing_view = getMultiAdapter((self.context, self.request),
                                       name='sharing')
-        local_roles = sharing_view.existing_role_settings()
+        local_roles = sharing_view.role_settings()


@jaroel this changes the structure of what we return on the sharing view, correct? If this is the case, this is a breaking change.

Nope, it's just that the search results are also in 'entries'

Ok. Fine with me then.

tisto · 2017-04-20T18:07:46Z

src/plone/restapi/tests/test_content_local_roles.py

+        search_entries = response.json()['entries']
+
+        # Did we find anything?
+        self.assertNotEqual(len(non_search_entries), len(search_entries))


@jaroel could we add some more tests here to make it visible what the sharing search actually returns?

Sure, no problem!

@jaroel Can we have a test that verifies that a POST to @sharing can be fed using the result from a previous GET to the endpoint?

@sneridagh That would be the same a normal post, wouldn't it?

Yes it would. The idea is just to make sure that you can GET first, slightly manipulate the result and POST it back.

@tisto / @sneridagh I've added a test for this

coveralls · 2017-04-22T08:35:19Z

Coverage increased (+0.2%) to 96.482% when pulling 190503d on sharing into 3b2c9e9 on master.

Allows the frontend to search for other users and directly get their roles including the acquired and inherited flags.

This reverts commit 4024327.

This reverts commit 2239d0c.

coveralls · 2017-04-22T09:49:35Z

Coverage increased (+0.001%) to 96.482% when pulling 767dfac on sharing into a94169f on master.

sneridagh · 2017-04-24T13:48:42Z

docs/source/sharing.rst

@@ -35,6 +35,10 @@ The sharing information of a content object can also be directly accessed by app
 .. literalinclude:: _json/sharing_folder_get.resp
   :language: http

+.. note::
+    Users and/or groups without a sharing entry can be found by appending the argument `search` to the query string. ie search=admin.


@jaroel it would be nice to include a documentation example of the usage of the search term to test_documentation.py . I think it will be common enough use case to have it's own example.

@tisto / @sneridagh I've added a test for this

sneridagh

I would also not deprecate the @Principals endpoint, it could be handy for some other use cases.

tisto · 2017-05-07T17:48:29Z

Apart from the minor issues @sneridagh and I raised this PR looks good to me and can be merged.

coveralls · 2017-05-08T15:31:46Z

Coverage increased (+0.001%) to 96.384% when pulling 6e9a743 on sharing into 2d37bc7 on master.

tisto · 2017-05-19T04:43:34Z

@sneridagh could you please have a second look at this pr and let me know if you are happy with it now?

jaroel added the in progress label Apr 18, 2017

tisto requested a review from sneridagh April 18, 2017 16:20

tisto reviewed Apr 18, 2017

View reviewed changes

jaroel changed the title ~~Replace @principals with @@sharing?search_term support~~ Replace @principals with @@sharing?search support Apr 19, 2017

jaroel changed the title ~~Replace @principals with @@sharing?search support~~ Add search to @sharing Apr 19, 2017

tisto requested changes Apr 20, 2017

View reviewed changes

jaroel added 11 commits April 22, 2017 11:34

Support the 'search_term' parameter in the @@sharing endpoint.

0eff48e

Allows the frontend to search for other users and directly get their roles including the acquired and inherited flags.

Remove @Principals endpoint as there is currently no need.

9d5a593

remove more + changelog

516598a

Revert "Remove @Principals endpoint as there is currently no need."

6fa7136

This reverts commit 4024327.

Revert "remove more + changelog"

96a0e72

This reverts commit 2239d0c.

Use 'search' not 'search_term' in @sharing

395a157

Use 'search' not 'search_term' in @sharing

a4ff695

changelog

4022cc3

Update docs

6d54bc9

wording

3b66d7c

Add tests for inherited and global roles

767dfac

jaroel force-pushed the sharing branch from dbe9c7b to 767dfac Compare April 22, 2017 09:37

sneridagh reviewed Apr 24, 2017

View reviewed changes

sneridagh requested changes Apr 24, 2017

View reviewed changes

jaroel added 3 commits May 8, 2017 12:40

Add search example for sharing with inherited and global role

3cdacc6

Add sharing search/post roundtrip test

0504b37

Merge branch 'master' into sharing

6e9a743

tisto approved these changes May 22, 2017

View reviewed changes

sneridagh approved these changes May 23, 2017

View reviewed changes

tisto merged commit 5b35970 into master May 23, 2017

tisto deleted the sharing branch May 23, 2017 07:25

tisto removed the in progress label May 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add search to @sharing #316

Add search to @sharing #316

jaroel commented Apr 18, 2017

coveralls commented Apr 18, 2017 •

edited

tisto commented Apr 18, 2017

tisto Apr 18, 2017

tisto commented Apr 18, 2017

jaroel commented Apr 19, 2017 •

edited

coveralls commented Apr 19, 2017 •

edited

coveralls commented Apr 19, 2017 •

edited

coveralls commented Apr 19, 2017 •

edited

jaroel commented Apr 19, 2017

tisto commented Apr 19, 2017

tisto left a comment

tisto Apr 20, 2017

jaroel Apr 21, 2017

tisto Apr 20, 2017

jaroel Apr 21, 2017

tisto May 7, 2017

tisto Apr 20, 2017

jaroel Apr 21, 2017

jaroel Apr 22, 2017

sneridagh Apr 24, 2017

jaroel Apr 24, 2017 •

edited

tisto May 7, 2017

jaroel May 8, 2017

coveralls commented Apr 22, 2017 •

edited

coveralls commented Apr 22, 2017 •

edited

sneridagh Apr 24, 2017

tisto May 7, 2017

jaroel May 8, 2017

sneridagh left a comment

tisto commented May 7, 2017

coveralls commented May 8, 2017 •

edited

tisto commented May 19, 2017

Add search to @sharing #316

Add search to @sharing #316

Conversation

jaroel commented Apr 18, 2017

coveralls commented Apr 18, 2017 • edited

tisto commented Apr 18, 2017

Choose a reason for hiding this comment

tisto commented Apr 18, 2017

jaroel commented Apr 19, 2017 • edited

coveralls commented Apr 19, 2017 • edited

coveralls commented Apr 19, 2017 • edited

coveralls commented Apr 19, 2017 • edited

jaroel commented Apr 19, 2017

tisto commented Apr 19, 2017

tisto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaroel Apr 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 22, 2017 • edited

coveralls commented Apr 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sneridagh left a comment

Choose a reason for hiding this comment

tisto commented May 7, 2017

coveralls commented May 8, 2017 • edited

tisto commented May 19, 2017

coveralls commented Apr 18, 2017 •

edited

jaroel commented Apr 19, 2017 •

edited

coveralls commented Apr 19, 2017 •

edited

coveralls commented Apr 19, 2017 •

edited

coveralls commented Apr 19, 2017 •

edited

jaroel Apr 24, 2017 •

edited

coveralls commented Apr 22, 2017 •

edited

coveralls commented Apr 22, 2017 •

edited

coveralls commented May 8, 2017 •

edited