Skip to content
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

Add permission manager #158

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add permission manager #158

wants to merge 8 commits into from

Conversation

sidkurella
Copy link
Contributor

Allows editing office and user permissions without having to manually add rows to the database.

This was supposed to be a nice intro project for working on the website but then I just sorta stopped working on it.

First part of permission manager system.
Add page to view current office permission and current user permissions.
Also shows current offices with no permissions and current users with no
permissions.

TODO: have each entry link to an edit page for that user
Clicking the name of the office or user will lead to a page to edit
permissions. TODO: implement edit page.
import sqlalchemy
from ruddock.resources import Permissions

def fetch_office_permissions():
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the function to do this? Can't we just have it return the permissions of all offices, and also the is_active column?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean I suppose lol, does it matter?

Copy link
Contributor

@HenrySwanson HenrySwanson Apr 13, 2018

Choose a reason for hiding this comment

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

ahh, i tried it out with more thorough test data, and now i see it. you wanna show every office that's got permissions, even if they're not active, in case you need to remove them. likewise, you want to show all active offices, even if they have no permissions, in case you want to manage them.

same with users.

if that's the case, lemme suggest the following (you gotta have the extra any_permissions column because there's a bug where you can't sort on expressions involving aliases: https://bugs.mysql.com/bug.php?id=80802)

SELECT GROUP_CONCAT(permission_id) AS permissions,
  permission_id IS NOT NULL AS any_permissions,
  office_name, office_id, office_order, is_active
FROM offices
  NATURAL LEFT JOIN office_permissions
GROUP BY office_id
HAVING is_active OR any_permissions
ORDER BY
  any_permissions DESC, -- puts nulls after non-nulls
  permissions,
  office_order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it worked fine on my machine (using MariaDB, maybe they fixed it?). I checked and the Ruddock website is using MariaDB also so this might not be needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also using mariadb; maybe I'm just on an older version. In any case, I think that approach makes it much clearer what you're intending to fetch there.


def delete_office_permission(office_id, perm_id):
"""Deletes the specified permission from the office."""
if type(perm_id) is str or type(perm_id) is unicode:
Copy link
Contributor

Choose a reason for hiding this comment

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

man, py3 makes this much nicer; maybe we should consider that someday

far in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

later later

return flask.g.db.execute(query, o_id=office_id, p_id=perm_id)


def fetch_user_permissions():
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before, why does this return a table and also a subset of it?

+ str(type(string)))
return [get_perm_name(int(x)) for x in string.split(sep)]

def decode_perm_string_with_id(string, sep=","):
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@HenrySwanson HenrySwanson left a comment

Choose a reason for hiding this comment

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

Looks great! 😄

UX suggestion: having a "back to permission manager" button might be good

Also, I managed to remove admin powers from everyone and every office. Since that can always be fixed by hand in the DB, IDK if we care that much, but worth bringing up.

Which reminds me, someday we should implement logging on all these actions. Might be a good project for a newbie, so they can get acquainted with the whole site.

import sqlalchemy
from ruddock.resources import Permissions

def fetch_office_permissions():
Copy link
Contributor

@HenrySwanson HenrySwanson Apr 13, 2018

Choose a reason for hiding this comment

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

ahh, i tried it out with more thorough test data, and now i see it. you wanna show every office that's got permissions, even if they're not active, in case you need to remove them. likewise, you want to show all active offices, even if they have no permissions, in case you want to manage them.

same with users.

if that's the case, lemme suggest the following (you gotta have the extra any_permissions column because there's a bug where you can't sort on expressions involving aliases: https://bugs.mysql.com/bug.php?id=80802)

SELECT GROUP_CONCAT(permission_id) AS permissions,
  permission_id IS NOT NULL AS any_permissions,
  office_name, office_id, office_order, is_active
FROM offices
  NATURAL LEFT JOIN office_permissions
GROUP BY office_id
HAVING is_active OR any_permissions
ORDER BY
  any_permissions DESC, -- puts nulls after non-nulls
  permissions,
  office_order

@asharma0703 asharma0703 removed their request for review March 22, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants