4
\$\begingroup\$

I was looking for a way to markup parts of my app as restricted to only certain roles, on an element basis.

This works quite well, using an attribute directive, where I specify the allowed roles and disable/hide the corresponding element; e.g.

<input require-role="admin" />

In order to make the app more maintainable I thought about inheriting the the required role attribute, e.g.

<form>
    <div required-role="Admin">              <!-- Set:       'Admin' -->
        <input required-role/>               <!-- Inherit:   'Admin' -->
        <input required-role/>               <!-- Inherit:   'Admin' -->
        <input required-role/>               <!-- Inherit:   'Admin' -->
        <input required-role/>               <!-- Inherit:   'Admin' -->
        <input />                            <!-- No role checking  -->
        <div required-role="User">           <!-- Overwrite: 'User' -->
            <input required-role/>           <!-- Inherit:   'User' -->
        </div>
    </div>                                 
    <div required-role="Admin User">         <!-- Set:   'Admin User' -->         
        <input required-role/>               <!-- Inherit:   'Admin User' -->
        <input required-role/>               <!-- Inherit:   'Admin User' -->
        <input required-role/>               <!-- Inherit:   'Admin User' -->
        <input required-role="Admin"/>       <!-- Overwrite: 'Admin' -->
    </div>
    <button required-role="Admin">Save</button>
    <button required-role="User">Send to review</button>
</form>

What I came up with is the following directive, that recursively searches in the isolatedScope of the parent element until it eventually finds the requiredRole property:

var app = angular.module('plunker', []);

app.controller('MainCtrl', function($scope){
  $scope.name = 'World';
});
app.directive('requireRole', function(){
  var getRequiredRoleFromParent = function(element){
    var par = element.parent();
    if (!par) return;
    var is = par.isolateScope();
    if (is && is.requireRole) return is.requireRole;
    return getRequiredRoleFromParent(par);
  };
  return {
    restrict: 'A',
    scope   : {
      requireRole: '@'
    },
    link    : function(scope, element){
      var labelClass = "label-success";
      if (!scope.requireRole) {
        scope.requireRole = getRequiredRoleFromParent(element);
        labelClass = "label-info";
      }
      element.prepend('<span class="label ' + labelClass + ' ">' + scope.requireRole + '</span>');
    }
  }
});

I am happy with the solution I came up with, yet my experience developing directives is limited; maybe there are better ways to achieve the same behavior?

Plunkr can be found here

\$\endgroup\$
1
\$\begingroup\$

In your HTML

  <form>
      <div required-role="Admin">              <!-- Set:       'Admin' -->
          <input required-role/>               <!-- Inherit:   'Admin' -->
          <input required-role/>               <!-- Inherit:   'Admin' -->
          <input required-role/>               <!-- Inherit:   'Admin' -->
          <input required-role/>               <!-- Inherit:   'Admin' -->
          <input />                            <!-- No role checking  -->
          <div required-role="User">           <!-- Overwrite: 'User' -->
              <input required-role/>           <!-- Inherit:   'User' -->
          </div>
      </div>                                 
      <div required-role="Admin User">         <!-- Set:   'Admin User' -->         
          <input required-role/>               <!-- Inherit:   'Admin User' -->
          <input required-role/>               <!-- Inherit:   'Admin User' -->
          <input required-role/>               <!-- Inherit:   'Admin User' -->
          <input required-role="Admin"/>       <!-- Overwrite: 'Admin' -->
      </div>
      <button required-role="Admin">Save</button>
      <button required-role="User">Send to review</button>
  </form>

You create a div just to inherit from it, this is bad practice, unless there is more than one element inside that div that is going to inherit from the div

in the second group of code you actually do it the right way, not sure why you were inconsistent with the first block.

you should write it like this

<form>
    <div required-role="Admin">              <!-- Set:       'Admin' -->
        <input required-role/>               <!-- Inherit:   'Admin' -->
        <input required-role/>               <!-- Inherit:   'Admin' -->
        <input required-role/>               <!-- Inherit:   'Admin' -->
        <input required-role/>               <!-- Inherit:   'Admin' -->
        <input />                            <!-- No role checking  -->
        <input required-role="User"/>        <!-- Inherit:   'User' -->
    </div>                                 
    <div required-role="Admin User">         <!-- Set:   'Admin User' -->         
        <input required-role/>               <!-- Inherit:   'Admin User' -->
        <input required-role/>               <!-- Inherit:   'Admin User' -->
        <input required-role/>               <!-- Inherit:   'Admin User' -->
        <input required-role="Admin"/>       <!-- Overwrite: 'Admin' -->
    </div>
    <button required-role="Admin">Save</button>
    <button required-role="User">Send to review</button>
</form>

I also assume that this is Example HTML as you don't give any of those input tags ID's which makes them unusable.


In your Javascript

  var getRequiredRoleFromParent = function(element){
      var par = element.parent();
      if (!par) return;
      var is = par.isolateScope();
      if (is && is.requireRole) return is.requireRole;
      return getRequiredRoleFromParent(par);
  };

You could take the negative check out of there, it doesn't make sense that way, instead perform a positive check, if it's true then you can perform what is after that.

The "after that" stuff should be changed to a ternary statement, it looks cleaner to me and it is equivalent. It is slightly nested but it shows the flow of the code brightly.

var getRequiredRoleFromParent = function(element){
    var par = element.parent();
    if (par) {
        var is = par.isolateScope();
        return (is && is.requireRole)
            ? is.requireRole
            : getRequiredRoleFromParent(par);
    }
};

The Recursion of this function looks good, I don't see an issue with the recursive nature of this function.

\$\endgroup\$
  • \$\begingroup\$ Your answer seems not particuliary useful to me. The HTML is obviously a SSCCE, there is no point in criticizing this minor point, in the real HTML there will obviously be more elements. Also, copying the whole HTML chunk adds nothing as it is already visible in the question itself. \$\endgroup\$ – dirkk Oct 2 '14 at 14:19
  • \$\begingroup\$ The !par statement is even wrong to me, as exiting early and not using potentially nested ifs is good coding practice imho (see programmers.stackexchange.com/questions/118703/…). Also, the ternary statement is largely only a personal opinion, I don't think it is any easier to read. \$\endgroup\$ – dirkk Oct 2 '14 at 14:20
  • 1
    \$\begingroup\$ coding standards on the other hand say that you should write a clear path and not dance around the fire 3 times before doing what needs to be done. if you would like to discuss further we can take this to chat. 2nd Monitor \$\endgroup\$ – Malachi Oct 2 '14 at 14:32
  • 1
    \$\begingroup\$ @dirkk I would love if you'd take a tour and read our help center. Please be aware the latter explicitly mentions: "Reviewers may comment on any part of the code" Your criticism of this answer thus is IMO unjustified and slightly rude. Aside from that, further discussion should be taken to Code Review Chat or Code Review Meta ;) \$\endgroup\$ – Vogel612 Oct 2 '14 at 14:43
  • 1
    \$\begingroup\$ Thanks everyone for taking care of this question. I was indeed more interested in AngularJS specifics :-) But nevertheless, the issues you mentioned with my example code are valid, thank you. \$\endgroup\$ – michael Oct 6 '14 at 13:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.