Skip to content
This repository has been archived by the owner on Feb 21, 2019. It is now read-only.

Feature/806 add label filter and sort content by label in contents endpoint #45

Merged
merged 15 commits into from Sep 27, 2018
Merged

Feature/806 add label filter and sort content by label in contents endpoint #45

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
94bd1b5
add filter by label in workspace_content endpoint
inkhey Sep 7, 2018
4b3c97c
sort content by label in content endpoint
inkhey Sep 7, 2018
3a0c516
Do not allow empty label value directly from schema
inkhey Sep 10, 2018
6db446b
order_by_property refactor
inkhey Sep 11, 2018
845f8ba
allow sort by list of properties instead of only one property
inkhey Sep 21, 2018
5a0f42b
Merge branch 'develop' of github.com:tracim/tracim_v2 into feature/80…
inkhey Sep 21, 2018
abfe0ca
better typing
inkhey Sep 27, 2018
3f3f498
Merge branch 'develop' of github.com:tracim/tracim_v2 into feature/80…
inkhey Sep 27, 2018
14bc527
better typing
inkhey Sep 27, 2018
6d869f8
better docstring
inkhey Sep 27, 2018
f520524
better docstring, more explicit about order_by_properties + get_all h…
inkhey Sep 27, 2018
a2d0bf5
better default value for order by properties
inkhey Sep 27, 2018
64ea79f
Merge branch 'develop' into feature/806_add_label_filter_and_sort_con…
inkhey Sep 27, 2018
ab228b3
fix broken schema
inkhey Sep 27, 2018
21b5bc8
fix broken schema
inkhey Sep 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 17 additions & 3 deletions backend/tracim_backend/lib/core/content.py
View file
Open in desktop
Expand Up @@ -1002,7 +1002,9 @@ def _get_all_query(
self,
parent_id: int = None,
content_type_slug: str = CONTENT_TYPES.Any_SLUG,
workspace: Workspace = None
workspace: Workspace = None,
label:str = None,
order_by_properties: typing.List[str] = (),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type hint is wrong. It should by typing.List[Column], doesnt it?

Copy link
Contributor Author

@inkhey inkhey Sep 26, 2018 •
edited

Choose a reason for hiding this comment

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

Currently no, we currently pass strings to "order_by" function of sqlalchemy.
We can also pass Column, complex sql subquery and probably others thing to this function, but as we have string in entry from endpoint and that string work for ordering, i do not think about casting this to Column.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@inkhey the part of code I looked at was something like .order_by(Content.label). Do you really pass strings? If so, what is the string content?

Copy link
Collaborator

Choose a reason for hiding this comment

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

$ grep -r order_by *
backend/tracim_backend/lib/core/group.py:        return self._base_query().order_by(Group.group_id).all()
backend/tracim_backend/lib/core/workspace.py:            workspaces = self._base_query().order_by(Workspace.label).all()
backend/tracim_backend/lib/core/workspace.py:                .order_by(Workspace.label) \
backend/tracim_backend/lib/core/user.py:        return self._session.query(User).order_by(User.display_name)
backend/tracim_backend/lib/core/userworkspace.py:    # def get_all_for_user_order_by_workspace(
backend/tracim_backend/lib/core/userworkspace.py:    #         .join(UserRoleInWorkspace.workspace).order_by(Workspace.label).all()
backend/tracim_backend/lib/core/content.py:                    .order_by(ContentRevisionRO.revision_id.desc())
backend/tracim_backend/lib/core/content.py:            .order_by(
backend/tracim_backend/lib/core/content.py:                .order_by(Content.revision_id.desc()) \
backend/tracim_backend/lib/core/content.py:        resultset = resultset.order_by(desc(Content.updated))
backend/tracim_backend/lib/core/content.py:    #         .order_by(desc(Content.updated))
backend/tracim_backend/models/data.py:                             order_by="ContentRevisionRO.revision_id")
backend/tracim_backend/tests/models/test_content.py:            .order_by(ContentRevisionRO.revision_id.desc())\
backend/tracim_backend/tests/library/test_webdav.py:            .order_by(Content.revision_id.desc()) \
backend/tracim_backend/tests/library/test_webdav.py:            .order_by(Content.revision_id.desc()) \
backend/tracim_backend/tests/library/test_webdav.py:            .order_by(Content.revision_id.desc()) \
backend/tracim_backend/tests/library/test_webdav.py:            .order_by(Content.revision_id.desc()) \
backend/tracim_backend/tests/library/test_webdav.py:            .order_by(ContentRevisionRO.revision_id.desc()) \
backend/tracim_backend/tests/library/test_webdav.py:            .order_by(ContentRevisionRO.revision_id.desc()) \
backend/tracim_backend/tests/library/test_webdav.py:            .order_by(ContentRevisionRO.revision_id.desc()) \
backend/tracim_backend/tests/library/test_webdav.py:            .order_by(ContentRevisionRO.revision_id.desc()) \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, in fact, i pass "Content.label" which is "'hybrid_propertyProxy" .... i just tried with passing "label" and it work also.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, the problem is that typing must help developer to understand the code. If the typing is defined as "string", the developer has no information about what to give as a parameter. I see 2 possibilities :

  • improve typing (the best solution)
  • add docstring

Maybe both must be implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

problem i see is that typing who should allowed here is clearly unclear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok. so I suggest you set the typing to "str or hybrid_propertyProxy", and you add docstring explaining that the recommanded way to use it is to pass Model.attribute.

Copy link
Collaborator

Choose a reason for hiding this comment

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

as soos as you fix this, tell me (or @buxx) to merge the MR

) -> Query:
"""
Extended filter for better "get all data" query
Expand All @@ -1028,11 +1030,23 @@ def _get_all_query(
resultset = resultset.filter(Content.parent_id==parent_id)
if parent_id == 0 or parent_id is False:
resultset = resultset.filter(Content.parent_id == None)
if label:
resultset = resultset.filter(Content.label.ilike('%{}%'.format(label))) # nopep8

for _property in order_by_properties:
resultset = resultset.order_by(_property)

return resultset

def get_all(self, parent_id: int=None, content_type: str=CONTENT_TYPES.Any_SLUG, workspace: Workspace=None) -> typing.List[Content]:
return self._get_all_query(parent_id, content_type, workspace).all()
def get_all(
self,
parent_id: int=None,
content_type: str=CONTENT_TYPES.Any_SLUG,
workspace: Workspace=None,
label: str=None,
order_by_properties: typing.List[str] = ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type hint is wrong. It should by typing.List[Column], doesnt it?

) -> typing.List[Content]:
return self._get_all_query(parent_id, content_type, workspace, label, order_by_properties).all()

# TODO - G.M - 2018-07-17 - [Cleanup] Drop this method if unneeded
# def get_children(self, parent_id: int, content_types: list, workspace: Workspace=None) -> typing.List[Content]:
Expand Down
2 changes: 2 additions & 0 deletions backend/tracim_backend/models/context_models.py
View file
Open in desktop
Expand Up @@ -280,6 +280,7 @@ def __init__(
show_deleted: int = 0,
show_active: int = 1,
content_type: str = None,
label: str = None,
offset: int = None,
limit: int = None,
) -> None:
Expand All @@ -290,6 +291,7 @@ def __init__(
self.show_active = bool(show_active)
self.limit = limit
self.offset = offset
self.label = label
self.content_type = content_type


Expand Down
132 changes: 85 additions & 47 deletions backend/tracim_backend/tests/functional/test_workspaces.py
View file
Open in desktop
Expand Up @@ -1744,18 +1744,16 @@ def test_api__get_workspace_content__ok_200__get_default(self):
# TODO - G.M - 30-05-2018 - Check this test
assert len(res) == 3
content = res[0]
assert content['content_id'] == 1
assert content['content_type'] == 'folder'
assert content['content_id'] == 11
assert content['content_type'] == 'html-document'
assert content['is_archived'] is False
assert content['is_deleted'] is False
assert content['label'] == 'Tools'
assert content['parent_id'] is None
assert content['label'] == 'Current Menu'
assert content['parent_id'] == 2
assert content['show_in_ui'] is True
assert content['slug'] == 'tools'
assert content['slug'] == 'current-menu'
assert content['status'] == 'open'
assert len(content['sub_content_types']) > 1
assert 'comment' in content['sub_content_types']
assert 'folder' in content['sub_content_types']
assert set(content['sub_content_types']) == {'comment'}
assert content['workspace_id'] == 1
content = res[1]
assert content['content_id'] == 2
Expand All @@ -1772,16 +1770,18 @@ def test_api__get_workspace_content__ok_200__get_default(self):
assert 'folder' in content['sub_content_types']
assert content['workspace_id'] == 1
content = res[2]
assert content['content_id'] == 11
assert content['content_type'] == 'html-document'
assert content['content_id'] == 1
assert content['content_type'] == 'folder'
assert content['is_archived'] is False
assert content['is_deleted'] is False
assert content['label'] == 'Current Menu'
assert content['parent_id'] == 2
assert content['label'] == 'Tools'
assert content['parent_id'] is None
assert content['show_in_ui'] is True
assert content['slug'] == 'current-menu'
assert content['slug'] == 'tools'
assert content['status'] == 'open'
assert set(content['sub_content_types']) == {'comment'}
assert len(content['sub_content_types']) > 1
assert 'comment' in content['sub_content_types']
assert 'folder' in content['sub_content_types']
assert content['workspace_id'] == 1

def test_api__get_workspace_content__ok_200__get_default_html_documents(self):
Expand Down Expand Up @@ -1840,20 +1840,20 @@ def test_api__get_workspace_content__ok_200__get_all_root_content__legacy_html_s
).json_body # nopep8
# TODO - G.M - 30-05-2018 - Check this test
assert len(res) == 4
content = res[1]
content = res[0]
assert content['content_type'] == 'html-document'
assert content['content_id'] == 15
assert content['content_id'] == 17
assert content['is_archived'] is False
assert content['is_deleted'] is False
assert content['label'] == 'New Fruit Salad'
assert content['is_deleted'] is True
assert content['label'].startswith('Bad Fruit Salad')
assert content['parent_id'] is None
assert content['show_in_ui'] is True
assert content['slug'] == 'new-fruit-salad'
assert content['slug'].startswith('bad-fruit-salad')
assert content['status'] == 'open'
assert set(content['sub_content_types']) == {'comment'}
assert content['workspace_id'] == 3

content = res[2]
content = res[1]
assert content['content_type'] == 'html-document'
assert content['content_id'] == 16
assert content['is_archived'] is True
Expand All @@ -1868,13 +1868,13 @@ def test_api__get_workspace_content__ok_200__get_all_root_content__legacy_html_s

content = res[3]
assert content['content_type'] == 'html-document'
assert content['content_id'] == 17
assert content['content_id'] == 15
assert content['is_archived'] is False
assert content['is_deleted'] is True
assert content['label'].startswith('Bad Fruit Salad')
assert content['is_deleted'] is False
assert content['label'] == 'New Fruit Salad'
assert content['parent_id'] is None
assert content['show_in_ui'] is True
assert content['slug'].startswith('bad-fruit-salad')
assert content['slug'] == 'new-fruit-salad'
assert content['status'] == 'open'
assert set(content['sub_content_types']) == {'comment'}
assert content['workspace_id'] == 3
Expand Down Expand Up @@ -1903,20 +1903,20 @@ def test_api__get_workspace_content__ok_200__get_all_root_content(self):
).json_body # nopep8
# TODO - G.M - 30-05-2018 - Check this test
assert len(res) == 4
content = res[1]
content = res[0]
assert content['content_type'] == 'html-document'
assert content['content_id'] == 15
assert content['content_id'] == 17
assert content['is_archived'] is False
assert content['is_deleted'] is False
assert content['label'] == 'New Fruit Salad'
assert content['is_deleted'] is True
assert content['label'].startswith('Bad Fruit Salad')
assert content['parent_id'] is None
assert content['show_in_ui'] is True
assert content['slug'] == 'new-fruit-salad'
assert content['slug'].startswith('bad-fruit-salad')
assert content['status'] == 'open'
assert set(content['sub_content_types']) == {'comment'}
assert content['workspace_id'] == 3

content = res[2]
content = res[1]
assert content['content_type'] == 'html-document'
assert content['content_id'] == 16
assert content['is_archived'] is True
Expand All @@ -1931,13 +1931,51 @@ def test_api__get_workspace_content__ok_200__get_all_root_content(self):

content = res[3]
assert content['content_type'] == 'html-document'
assert content['content_id'] == 17
assert content['content_id'] == 15
assert content['is_archived'] is False
assert content['is_deleted'] is True
assert content['label'].startswith('Bad Fruit Salad')
assert content['is_deleted'] is False
assert content['label'] == 'New Fruit Salad'
assert content['parent_id'] is None
assert content['show_in_ui'] is True
assert content['slug'].startswith('bad-fruit-salad')
assert content['slug'] == 'new-fruit-salad'
assert content['status'] == 'open'
assert set(content['sub_content_types']) == {'comment'}
assert content['workspace_id'] == 3

def test_api__get_workspace_content__ok_200__get_all_root_content_filter_by_label(self): # nopep8
"""
Check obtain workspace all root contents
"""
params = {
'parent_id': 0,
'show_archived': 1,
'show_deleted': 1,
'show_active': 1,
'label': 'ew'
}
self.testapp.authorization = (
'Basic',
(
'bob@fsf.local',
'foobarbaz'
)
)
res = self.testapp.get(
'/api/v2/workspaces/3/contents',
status=200,
params=params,
).json_body # nopep8
# TODO - G.M - 30-05-2018 - Check this test
assert len(res) == 1
content = res[0]
assert content['content_type'] == 'html-document'
assert content['content_id'] == 15
assert content['is_archived'] is False
assert content['is_deleted'] is False
assert content['label'] == 'New Fruit Salad'
assert content['parent_id'] is None
assert content['show_in_ui'] is True
assert content['slug'] == 'new-fruit-salad'
assert content['status'] == 'open'
assert set(content['sub_content_types']) == {'comment'}
assert content['workspace_id'] == 3
Expand Down Expand Up @@ -2284,26 +2322,26 @@ def test_api__get_workspace_content__ok_200__get_all_filter_content_html_and_leg
assert content['content_id']
assert content['is_archived'] is False
assert content['is_deleted'] is False
assert content['label'] == 'test_page'
assert content['label'] == 'test_html_page'
assert content['parent_id'] == 1
assert content['show_in_ui'] is True
assert content['slug'] == 'test-page'
assert content['slug'] == 'test-html-page'
assert content['status'] == 'open'
assert set(content['sub_content_types']) == {'comment'} # nopep8
assert content['workspace_id'] == 1
assert res[0]['content_id'] != res[1]['content_id']
content = res[1]
assert content['content_type'] == 'html-document'
assert content['content_id']
assert content['is_archived'] is False
assert content['is_deleted'] is False
assert content['label'] == 'test_html_page'
assert content['label'] == 'test_page'
assert content['parent_id'] == 1
assert content['show_in_ui'] is True
assert content['slug'] == 'test-html-page'
assert content['slug'] == 'test-page'
assert content['status'] == 'open'
assert set(content['sub_content_types']) == {'comment'} # nopep8
assert content['workspace_id'] == 1
assert res[0]['content_id'] != res[1]['content_id']

def test_api__get_workspace_content__ok_200__get_all_folder_content(self):
"""
Expand Down Expand Up @@ -2331,13 +2369,13 @@ def test_api__get_workspace_content__ok_200__get_all_folder_content(self):
assert len(res) == 3
content = res[0]
assert content['content_type'] == 'html-document'
assert content['content_id'] == 12
assert content['content_id'] == 14
assert content['is_archived'] is False
assert content['is_deleted'] is False
assert content['label'] == 'New Fruit Salad'
assert content['is_deleted'] is True
assert content['label'].startswith('Bad Fruit Salad')
assert content['parent_id'] == 10
assert content['show_in_ui'] is True
assert content['slug'] == 'new-fruit-salad'
assert content['slug'].startswith('bad-fruit-salad')
assert content['status'] == 'open'
assert set(content['sub_content_types']) == {'comment'} # nopep8
assert content['workspace_id'] == 2
Expand All @@ -2357,13 +2395,13 @@ def test_api__get_workspace_content__ok_200__get_all_folder_content(self):

content = res[2]
assert content['content_type'] == 'html-document'
assert content['content_id'] == 14
assert content['content_id'] == 12
assert content['is_archived'] is False
assert content['is_deleted'] is True
assert content['label'].startswith('Bad Fruit Salad')
assert content['is_deleted'] is False
assert content['label'] == 'New Fruit Salad'
assert content['parent_id'] == 10
assert content['show_in_ui'] is True
assert content['slug'].startswith('bad-fruit-salad')
assert content['slug'] == 'new-fruit-salad'
assert content['status'] == 'open'
assert set(content['sub_content_types']) == {'comment'} # nopep8
assert content['workspace_id'] == 2
Expand Down
17 changes: 14 additions & 3 deletions backend/tracim_backend/views/core_api/schemas.py
View file
Open in desktop
Expand Up @@ -440,6 +440,12 @@ class FilterContentQuerySchema(marshmallow.Schema):
default=CONTENT_TYPES.Any_SLUG,
validate=all_content_types_validator
)
label = marshmallow.fields.String(
example='myfilename',
default=None,
allow_none=True,
description='Filter by content label'
)

@post_load
def make_content_filter(self, data):
Expand Down Expand Up @@ -796,11 +802,14 @@ def make_move_params(self, data):
class ContentCreationSchema(marshmallow.Schema):
label = marshmallow.fields.String(
example='contract for client XXX',
description='Title of the content to create'
description='Title of the content to create',
validate=Length(min=1),
required=True
)
content_type = marshmallow.fields.String(
example='html-document',
validate=all_content_types_validator,
required=True,
)
parent_id = marshmallow.fields.Integer(
example=35,
Expand Down Expand Up @@ -982,7 +991,8 @@ class CommentSchema(marshmallow.Schema):

class SetCommentSchema(marshmallow.Schema):
raw_content = marshmallow.fields.String(
example='<p>This is just an html comment !</p>'
example='<p>This is just an html comment !</p>',
validate=Length(min=1)
)

@post_load()
Expand All @@ -994,7 +1004,8 @@ class ContentModifyAbstractSchema(marshmallow.Schema):
label = marshmallow.fields.String(
required=True,
example='contract for client XXX',
description='New title of the content'
description='New title of the content',
validate=Length(min=1)
)


Expand Down
3 changes: 3 additions & 0 deletions backend/tracim_backend/views/core_api/workspace_controller.py
View file
Open in desktop
Expand Up @@ -33,6 +33,7 @@
from tracim_backend.models.context_models import ContentInContext
from tracim_backend.models.context_models import UserRoleWorkspaceInContext
from tracim_backend.models.data import ActionDescription
from tracim_backend.models.data import Content
from tracim_backend.models.data import UserRoleInWorkspace
from tracim_backend.models.revision_protection import new_revision
from tracim_backend.models.roles import WorkspaceRoles
Expand Down Expand Up @@ -395,6 +396,8 @@ def workspace_content(
parent_id=content_filter.parent_id,
workspace=request.current_workspace,
content_type=content_filter.content_type or CONTENT_TYPES.Any_SLUG,
label=content_filter.label,
order_by_properties=[Content.label]
)
contents = [
api.get_content_in_context(content) for content in contents
Expand Down