feat: respect user roles in dashboard APIs #36
Conversation
shadinaif
force-pushed
the
shadinaif/dashboard-user-roles
branch
5 times, most recently
from
43e29ae to
528d9cc
Compare
shadinaif
changed the title
shadinaif
force-pushed
the
shadinaif/dashboard-user-roles
branch
7 times, most recently
from
eeab1ce to
7a1c893
Compare
shadinaif
force-pushed
the
shadinaif/dashboard-user-roles
branch
from
7a1c893 to
809c621
Compare
shadinaif
force-pushed
the
shadinaif/dashboard-user-roles
branch
from
febf1a2 to
287c599
Compare
shadinaif
force-pushed
the
shadinaif/dashboard-user-roles
branch
from
287c599 to
773b02d
Compare
| from simple_history.models import HistoricalRecords | ||
|
|
||
|
|
||
| class ViewAllowedRoles(models.Model): |
There was a problem hiding this comment.
@shadinaif Isn't that a little bit too dynamic to our needs?
Would you mind sharing the details why do you think we cannot hard-code the allowed roles in every view?
There was a problem hiding this comment.
Maybe it's too dynamic right now. But I believe it'll make a lot more sense when FutureX customers start to use it. I actually guess they might ask for roles per tenant in the future!
shadinaif
force-pushed
the
shadinaif/dashboard-user-roles
branch
2 times, most recently
from
e6e065b to
71b59cd
Compare
shadinaif
force-pushed
the
shadinaif/dashboard-user-roles
branch
from
71b59cd to
aa1df58
Compare
| @cache_dict(timeout='FX_CACHE_TIMEOUT_COURSE_ACCESS_ROLES', key_generator_or_name=cs.CACHE_NAME_ALL_COURSE_ACCESS_ROLES) | ||
| def get_all_course_access_roles() -> dict: |
There was a problem hiding this comment.
Note for Omar on performance: This is is probably an anti-pattern that slows down the request.
#perf
| class FXViewRoleInfoMetaClass(type): | ||
| """Metaclass to provide role information to the view.""" |
There was a problem hiding this comment.
Why a Metaclass is needed?
There was a problem hiding this comment.
Two reasons:
- To have schema check on the class definition for any new view in the future.
- To collect the needed information to apply data-migration (not DB-migration) automatically. If we add a view, it'll have its allowed roles saved in
ViewAllowedRolestable automatically
There was a problem hiding this comment.
@shadinaif the code is fine and working and I couldn't fine huge issues in the code.
However, I think the course access roles cache is actually slowing down the site. I don't have data to show that, the code just doesn't look right and it has all sort of problems:
- Cache the results, which results in delayed permission granting and frustrating the users.
- The cache will store all users in a single object, while we need to check the permission of a single user (dashboard user) on every request.
I think it can be merged, but the cache needs to be tested to ensure it's not slowing requests down.
|
Thank you @OmarIthawi , can you please approve it? I have a plan for performance issues, I'll consider your note about the cache too for sure |
|
I thought I did approve it. Sorry. |
Respect user roles in dashboard APIs