isInRole, fix #56

[StorytellerCZ]

Adds isInRole function (and its async equivalent) as per #56

TODO: tests

[StorytellerCZ]

Just need to add the tests and then we are good to go.

[jankapunkt]

Generally approve, since it technically works. My comments a mostly nitpicking.
One thing, though: the majority of the functions could be extracted into a single fn (DRY):

const getIsInRole = (role, scope, fn) => {
  const userId = Meteor.userId()
  const comma = (role || '').indexOf(',')
  let roles

  if (!userId) return false
  if (!Match.test(role, String)) return false

  if (comma !== -1) {
    roles = role.split(',').reduce(function (memo, r) {
      if (!r) {
        return memo
      }
      memo.push(r)
      return memo
    }, [])
  } else {
    roles = [role]
  }

return Match.test(scope, String)
  ? fn(userId, roles, scope)
  : fn(userId, roles)
}
 

// ...

isInRole: function (role, scope) {
  return getIsInRole(role, scope, this.userIsInRole)
}

isInRoleAsync: function (role, scope) {
  return getIsInRole(role, scope, this.userIsInRoleAsync)
}

Just a potential saving of lines and potential bugs etc.

what do you think? Am I overengineering?

[jankapunkt]

This could be simplified:

if (r) {
  memo.push(r)
}
return memo

[jankapunkt]

This could be a conditional return:

return Match.test(scope, String)
  ? this.userIsInRole(userId, roles, scope)
  : this.userIsInRole(userId, roles)

[jankapunkt]

Technically you don't need to await before returning and also don't need to make the function async, because it will return a promise anyway.

If you resolve the promise before returning, there will be one more microtask execution than if returning the promise directly:

isInRoleAsync: function (role, scope) {
  // ...

  return this.userIsInRoleAsync(userId, roles) // always returns Promise
}

does not affect anything by itself but might be measurable when requests / operations do in the millions haha

[StorytellerCZ]

Yes, I agree with all the simplifications and improvements. The original code is pretty old.

[jankapunkt]

@StorytellerCZ is this still active or superseded by release 4.0.4

[StorytellerCZ]

Still, active. Have to find the time to check this again and get it merged, but I think that will be into the official package instead.