Skip to content

Task list should only show tasks one can administrate #4087

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 16, 2019
Merged

Conversation

youri-k
Copy link
Contributor

@youri-k youri-k commented May 9, 2019

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • open task list if you are a team manager in one team and a user in another one
  • task list should only show task from the team where you're team manager

Issues:


@youri-k youri-k requested review from fm3 and jstriebel May 9, 2019 12:17
@youri-k youri-k self-assigned this May 9, 2019
@youri-k
Copy link
Contributor Author

youri-k commented May 9, 2019

This is a pretty naive solution but it works and doesn't change the sql query, so I thought this would fix the problem 🙂

Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

I think this condition should rather be part of the SQL query. The current version probably breaks pagination, as this is handled on top of the SQL query, leading to possibly fewer results per page. Also this version is slower, probably not a bottleneck, though.

If this is urgent and/or the SQL variant too complex, we can also use this version. But I'd assume that in SQL this should just be a left join on the administrable task types.

@youri-k
Copy link
Contributor Author

youri-k commented May 9, 2019

The current version probably breaks pagination, as this is handled on top of the SQL query, leading to possibly fewer results per page.

Good catch! I forgot about that. I now added a listAccessQ that is similar to the deleteAccessQ because it adds the needed extra condition 🙂

@jstriebel
Copy link
Contributor

I now added a listAccessQ that is similar to the deleteAccessQ because it adds the needed extra condition

Nice! That's actually even simpler than before. Just rename the whole access query to administrateAccessQ, that fits both cases. The setup to test this is quite complex, so I'd skip this if you tested this thoroughly.

@youri-k
Copy link
Contributor Author

youri-k commented May 9, 2019

I don't think this works because the deleteAccessQ is a member of the base class and is used in deleteOne for instance. We could think about using the deleteAccessQ in the list query and adding a comment but I'm not sure if this is really better

Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

I agree, the current version is fine! Code LGTM, did not test it myself.

@youri-k youri-k merged commit 09bab0b into master May 16, 2019
@youri-k youri-k deleted the fix-task-list branch May 16, 2019 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Team Manager can't access tasks' annotations
2 participants