Closed Alchez closed 3 months ago
Alchez
commented
4 months ago @agritheory Should an approval rule's condition(s) affect if the user should be able to view the document? Or having a rule for a user or user role is good enough?
Also, I think this change is causing the tests to fail. What do you suggest?
agritheory
commented
4 months ago This override strategy is too specific - requiring a class override per doctype that requires approval is not feasible. We really only want to use the has_permission hook.
agritheory
commented
4 months ago @Alchez New error:
Alchez
commented
4 months ago @agritheory Okay, so I tested it out locally and I spoke with Revant as well about the double APIs, and here's what I've found:
has_permission hooks do actually override the controller class method as well, so it's more of an entrypoint than a separate API.
fproldan
commented
4 months ago @agritheory
About the "Page" issue.
I've found where the root cause might be: router.js
The DocType has to be in frappe.boot.user.can_read for the router to display it as FormView
Even adding manually in the code for testing purposes I got stuck here
00:17:50 web.1 | Traceback (most recent call last):
00:17:50 web.1 | File "apps/frappe/frappe/app.py", line 95, in application
00:17:50 web.1 | response = frappe.api.handle()
00:17:50 web.1 | ^^^^^^^^^^^^^^^^^^^
00:17:50 web.1 | File "apps/frappe/frappe/api.py", line 55, in handle
00:17:50 web.1 | return frappe.handler.handle()
00:17:50 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^
00:17:50 web.1 | File "apps/frappe/frappe/handler.py", line 48, in handle
00:17:50 web.1 | data = execute_cmd(cmd)
00:17:50 web.1 | ^^^^^^^^^^^^^^^^
00:17:50 web.1 | File "apps/frappe/frappe/handler.py", line 86, in execute_cmd
00:17:50 web.1 | return frappe.call(method, **frappe.form_dict)
00:17:50 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
00:17:50 web.1 | File "apps/frappe/frappe/__init__.py", line 1611, in call
00:17:50 web.1 | return fn(*args, **newargs)
00:17:50 web.1 | ^^^^^^^^^^^^^^^^^^^^
00:17:50 web.1 | File "apps/frappe/frappe/desk/form/load.py", line 43, in getdoc
00:17:50 web.1 | raise frappe.PermissionError(("read", doctype, name))
00:17:50 web.1 | frappe.exceptions.PermissionError: ('read', 'Purchase Invoice', 'ACC-PINV-2024-00011')@agritheory @fproldan
I can confirm the error with the changes that Francisco suggested.
It seems like the permission hook is simply a filter in the calculation of permissions. Having our hook return truthy doesn't imply read-access; it's more of an additional check. The actual permission matrix is still generated based on the set permissions in Frappe.
I think we have (atleast) two possibilities here:
frappe.local.role_permissions property.
PS: In both the above solutions, I got permission errors for different doctypes while trying to load a Purchase Invoice (frappe.exceptions.PermissionError: No permission for Buying Settings). Although the actual document did eventually load up for me.
Example use of the second approach:
cache_key = ("Purchase Invoice", "sam@cfc.com", False) # the last element is for whether the user created the document
frappe.local.role_permissions[cache_key] = {
"read": True,
"write": True,
"create": False,
"delete": False,
"submit": False,
"cancel": False,
"amend": False,
}@agritheory I've added the docshare feature for approvals in two places:
assign_users API is called for a document (currently this isn't being used anywhere, but I've added it just in case)Is there another scenario that I might be missing?
Might be related, but the tests seem to be failing while checking for ToDos. I'm not sure what could be causing it and I think that also means the share feature might not be working for new documents. Can you have a look?
PS: I've removed the previous has_permission controller hook implementation since the docshare feature seems sufficient for this use-case.
Closes #21.
Anytime an approver is manually added, the system will also share that document with the approver to ensure they can view/edit the document.