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

[inkhey]

related to tracim/tracim#806

[coveralls]

Pull Request Test Coverage Report for Build 775

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 100 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.003%) to 90.967%

---

Files with Coverage Reduction	New Missed Lines	%
tracim_backend/models/context_models.py	6	98.76%
tracim_backend/views/core_api/workspace_controller.py	12	95.6%
tracim_backend/lib/core/content.py	82	83.31%

Totals
Change from base Build 768:	0.003%
Covered Lines:	15046
Relevant Lines:	16540

---

💛 - Coveralls

[lebouquetin]

@buxx @inkhey I suggest to implement a more genery way of sorting.

[lebouquetin]

what about using something like order_by_property with a default value to None and:

if order_by_property:
  resultset = resultset.order_by(order_by_property) ?

The usage would be get_all(..., order_by_property=Content.label)?

@buxx @inkhey what do you think?

[buxx]

I agree with that

[inkhey]

Done.

[tracim]

last change request

[tracim]

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

[inkhey]

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.

[lebouquetin]

@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?

[lebouquetin]

$ 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()) \

[inkhey]

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

[lebouquetin]

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.

[inkhey]

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

[lebouquetin]

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.

[lebouquetin]

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

[tracim]

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

[buxx]

There is some problems here:

• typing say List, default value is a tuple (it should be empty list)
• we can't give an list instance here, example:

>>> def foo(a=[]):
...   a.append(1)
...   print(a)
... 
>>> foo()
[1]
>>> foo()
[1, 1]
>>> foo()
[1, 1, 1]
>>> foo()
[1, 1, 1, 1]

Prefer:

def foo(a=None):
  a = a or []  # FDV

FDV = Forced Default Value