I am currently struggling with privilege checks in the radiology module. As far as I know (feel free to correct me) there are 2 tags, which are able to check privileges on JSP level:
RequireTag
openmrs:require --> this throws an error if a privilege is not set
PrivilegeTag
openmrs:hasPrivilege --> this just “hides” the fields it contains
require provides the “allPrivileges” section, but hasPrivilege has no option for this. I was wondering if there is another way to hide code if one or more of several required privileges are missing. The following is ugly&no option for me:
PrivilegeTag already supports a list of comma-separated privileges, but the comma is an OR not an AND.
How are you going to indicate this without breaking compatibility ?
What about adding a boolean attribute like “require all” which is false by default (behavior equivalent to now) and if true all privileges in the comma separated list are required?
It’s a feasible option. Actually the “inverse” attribute means require none (all must be false). So the inverse of require all would be at least one false
As @lluismf mentioned, the privilege attribute of the hasPrivilege tag can be a comma delimited list of several privileges that behaves just like RequireTag.allPrivileges
@lluismf pointed out that the privilege attribute “supports a list of comma-separated privileges, but the comma is an OR not an AND” so you just need to have one of the privileges in the list for it to be true, thus it does not behave like RequireTag.allPrivileges. it should rather be called hasAnyPrivilege
Changing the attribute name will break the Jsp using the tag unless they are refactored.
Related to this, how will the new angular UI deal with this (conditional rendering of dom elements based on privileges) ? Server side rendering in this case seems easier.
opened a feature request https://issues.openmrs.org/browse/LUI-94
am happy to work on it if you think its ok
PS: to convince you PrivilegeTag.java has no test, this new feature will come with a great set of tests
@bgevam & @teleivo, this sounds like a great addition. I commented on the ticket that the attribute name “hasAll” is probably better than “requireAll” (and to make sure we support false values – e.g., “false”, “0”, “no” – for using the default behavior (to support, for example, someone conditionally setting this attribute).
@burke am on it
also commented back.
attributes that are considered boolean are never checked against “no”, “0”. so I suggest we dont start now, yes?