PR#791: [frontend] fix for api3 pagination ordering - copr/copr

copr / copr

#791 [frontend] fix for api3 pagination ordering

Merged 4 years ago by praiskup. Opened 4 years ago by praiskup.

Unknown source paginator-order into master

[frontend] fix for api3 pagination ordering

Pavel Raiskup • 4 years ago

51681b4

frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py

file modified

+16 -6

		`@@ -117,7 +117,6 @@`
		`LIMIT = None`
		`OFFSET = 0`
		`ORDER = "id"`
		`- ORDER_TYPE = "ASC"`

		`def __init__(self, query, model, limit=None, offset=None, order=None, order_type=None, **kwargs):`
		`self.query = query`
		`@@ -125,15 +124,26 @@`
		`self.limit = limit or self.LIMIT`
		`self.offset = offset or self.OFFSET`
		`self.order = order or self.ORDER`
		`- self.order_type = order_type or self.ORDER_TYPE`
		`+ self.order_type = order_type`
		`+ if not self.order_type:`
		`+ # desc/asc unspecified, use some guessed defaults`
		`+ if self.order == 'id':`
		`+ self.order_type = 'DESC'`
		`+ if self.order == 'name':`
frostyx commented 4 years ago What about having just `else` here? The only exception that comes to my mind, that we want also have `DESC`, are dates.
praiskup commented 4 years ago Maybe, I don't have strong preference (I intentionally tried to let the default unspecified, since it looked like more natural; and the default is ASC anyways).
		`+ self.order_type = 'ASC'`
		`+`

		`def get(self):`
		`if not hasattr(self.model, self.order):`
		`raise CoprHttpException("Can order by: {}".format(self.order))`
		`- order_col = self.order`
		`- order_fun = sqlalchemy.desc if self.order_type == "DESC" else sqlalchemy.asc`
		`- return self.query.order_by(None).limit(None).offset(None)\`
		`- .order_by(order_fun(order_col)).limit(self.limit).offset(self.offset)`
		`+`
		`+ order_fun = (lambda x: x)`
		`+ if self.order_type == 'ASC':`
		`+ order_fun = sqlalchemy.asc`
		`+ elif self.order_type == 'DESC':`
		`+ order_fun = sqlalchemy.desc`
		`+`
		`+ return self.query.order_by(order_fun(self.order)).limit(self.limit).offset(self.offset)`

		`@property`
		`def meta(self):`

praiskup commented 4 years ago

Make sure that we don't override explicit ORDER BY request from
Paginator caller (reverts 3e73f63), and that we
use sane defaults:
- by default order by field: model.id
- if ORDER BY id => use DESC by default
- if ORDER BY name => use ASC by default
- otherwise keep system default ASC/DESC

Fixes: rhbz#1717506
Fixes: PR#791

rebased onto f1ef2f7a1f177080a1852bbafd7049efd873e9b6

4 years ago

praiskup commented 4 years ago

[copr-build] mirror sync issue

frostyx commented on line 19 of frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py 4 years ago

What about having just else here? The only exception that comes to my mind, that we want also have DESC, are dates.

frostyx commented 4 years ago

Looks good, thank you. +1

praiskup commented on line 19 of frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py 4 years ago

Maybe, I don't have strong preference (I intentionally tried to let the default unspecified, since it looked like more natural; and the default is ASC anyways).

praiskup commented 4 years ago

No strictly required changes, so I'm merging. We can tweak the defaults anytime later.

rebased onto 51681b4

4 years ago

Pull-Request has been merged by praiskup

4 years ago