From 255894392ad09fa1e9b72f26f13ab7d96d43bc6e Mon Sep 17 00:00:00 2001 From: Adam Coldrick Date: Thu, 14 Mar 2019 09:08:05 +0000 Subject: [PATCH] Rationalise code for filtering private Boards and Worklists This commit replaces the confusing mess of aliased joins used when filtering private Worklists and Boards out of queries, with a clearer approach utilising the relationships defined in the SQLAlchemy model. This approach will be much easier to extend to support adding Teams to Board and Worklist permissions. Change-Id: I5a373dc91315a3f29f4442363da19105d4d18bfc --- storyboard/db/api/base.py | 105 +++++++++++++------------------------- storyboard/db/models.py | 2 + 2 files changed, 37 insertions(+), 70 deletions(-) diff --git a/storyboard/db/api/base.py b/storyboard/db/api/base.py index ca567251..d35cb6ac 100644 --- a/storyboard/db/api/base.py +++ b/storyboard/db/api/base.py @@ -446,64 +446,35 @@ def filter_private_worklists(query, current_user, hide_lanes=True): are lanes in a board. """ - # Alias all the things we're joining, in case they've been - # joined already. - board_worklists = aliased(models.BoardWorklist, - name="worklist_boardworklists") - boards = aliased(models.Board, name="worklist_boards") - board_permissions = aliased(models.board_permissions, - name="worklist_boardpermissions") - worklist_permissions = aliased(models.worklist_permissions, - name="worklist_worklistpermissions") - permissions = aliased(models.Permission, name="worklist_permissions") - user_permissions = aliased(models.user_permissions, - name="worklist_userpermissions") - users = aliased(models.User, name="worklist_users") - - # Worklists permissions must be inherited from the board which - # contains the list (if any). To handle this we split the query - # into the lists which are in boards (`lanes`) and those which - # aren't (`lists`). We then either hide the lanes entirely or - # unify the two queries. - lanes = query.join( - (board_worklists, models.Worklist.id == board_worklists.list_id)) - lanes = (lanes - .outerjoin((boards, boards.id == board_worklists.board_id)) - .outerjoin((board_permissions, - boards.id == board_permissions.c.board_id)) - .outerjoin((permissions, - permissions.id == board_permissions.c.permission_id)) - .outerjoin((user_permissions, - permissions.id == user_permissions.c.permission_id)) - .outerjoin((users, user_permissions.c.user_id == users.id))) - - lists = query.outerjoin( - (board_worklists, models.Worklist.id == board_worklists.list_id)) - lists = (lists.filter(board_worklists.board_id.is_(None)) - .outerjoin((worklist_permissions, - models.Worklist.id == worklist_permissions.c.worklist_id)) - .outerjoin((permissions, - permissions.id == worklist_permissions.c.permission_id)) - .outerjoin((user_permissions, - user_permissions.c.permission_id == permissions.id)) - .outerjoin((users, user_permissions.c.user_id == users.id))) + # Lanes inherit board permissions and ignore their own, so need to be + # handled separately. If we're not hiding all lanes from the result, + # these two queries are combined at the end. + lanes = query.filter(models.Worklist.boards.any()) + lists = query.filter(~models.Worklist.boards.any()) if current_user: if not hide_lanes: + # This removes any worklists that are related only to boards + # that the user doesn't have permission to see. lanes = lanes.filter( - or_( - and_( - users.id == current_user, - boards.private == true() - ), - boards.private == false() + models.Worklist.boards.any( + or_( + models.Board.permissions.any( + models.Permission.users.any( + models.User.id == current_user + ) + ), + models.Board.private == false(), + models.Board.id.is_(None) + ) ) ) lists = lists.filter( or_( - and_( - users.id == current_user, - models.Worklist.private == true() + models.Worklist.permissions.any( + models.Permission.users.any( + models.User.id == current_user + ) ), models.Worklist.private == false(), models.Worklist.id.is_(None) @@ -511,7 +482,16 @@ def filter_private_worklists(query, current_user, hide_lanes=True): ) else: if not hide_lanes: - lanes = lanes.filter(boards.private == false()) + # This removes any worklists that are related to only private + # boards + lanes = lanes.filter( + models.Worklist.boards.any( + or_( + models.Board.private == false(), + models.Board.id.is_(None) + ) + ) + ) lists = lists.filter( or_( models.Worklist.private == false(), @@ -531,28 +511,13 @@ def filter_private_boards(query, current_user): :param current_user: The ID of the user requesting the result. """ - board_permissions = aliased(models.board_permissions, - name="board_boardpermissions") - permissions = aliased(models.Permission, name="board_permissions") - user_permissions = aliased(models.user_permissions, - name="board_userpermissions") - users = aliased(models.User, name="board_users") - - query = (query - .outerjoin((board_permissions, - models.Board.id == board_permissions.c.board_id)) - .outerjoin((permissions, - board_permissions.c.permission_id == permissions.id)) - .outerjoin((user_permissions, - permissions.id == user_permissions.c.permission_id)) - .outerjoin((users, user_permissions.c.user_id == users.id))) - if current_user: query = query.filter( or_( - and_( - users.id == current_user, - models.Board.private == true() + models.Board.permissions.any( + models.Permission.users.any( + models.User.id == current_user + ) ), models.Board.private == false(), models.Board.id.is_(None) diff --git a/storyboard/db/models.py b/storyboard/db/models.py index 0432fffd..44b2db63 100644 --- a/storyboard/db/models.py +++ b/storyboard/db/models.py @@ -606,6 +606,8 @@ class Worklist(FullText, ModelBuilder, Base): automatic = Column(Boolean, default=False) filters = relationship(WorklistFilter) permissions = relationship("Permission", secondary="worklist_permissions") + boards = relationship("Board", secondary="board_worklists", + backref="worklists") _public_fields = ["id", "title", "creator_id", "project_id", "permission_id", "private", "archived", "automatic"]