1
\$\begingroup\$

Is there any DRYer way to write this method ? or is it better to keep it this way for better reader understanding ...

Each role has a list of authorised activities for a specific resource ( in this case Sheet) , a user may have multiple roles ( depending upon his company, his working group in the company, his function in the working group). This method is a sheet_policy, if among all roles , the current user can "index" sheets, the method returns true otherwise it returns false

   def index?
    # check if sheet#index is listed in current_user roles activities
    sheet_activities = []
    user.roles.each {|role| sheet_activities << role.activities["sheet"]}
    return false if sheet_activities.first.nil? # no authorized activities on Sheet
    sheet_activities.flatten.uniq!.include?("index")
  end

activities is a JSONB attribute of the Sheet record ...

create_table :roles do |t|
  t.string      :company, null: false, default: Settings.company.default.name
  t.string      :group
  t.string      :function
  t.jsonb       :activities, null: false, default: '{}'      
  t.timestamps  null: false

Seeding example :

   @role_accounting_employee = Role.create( group: "accounting", function: "employee")  # default company : "internal"
   @role_accounting_employee.activities.deep_merge!({sheet: %w(show index) })
\$\endgroup\$
  • \$\begingroup\$ Why do you check sheet_activities.first.nil?? Is there anything special about the user's first role? \$\endgroup\$ – 200_success Sep 19 '15 at 1:43
  • \$\begingroup\$ the :activities field is a JSONB structure which is defaulted to an empty hash. It can be filled dynamically later with authorised activities on different resources ( :sheet is one of them , but not necessarily present... ) so my first test in this case is to test if any allowed activities for the :sheet resource exist... :using .each I got a sheet.activities being [nil] Using .map should avoid it... \$\endgroup\$ – erwin Sep 19 '15 at 16:24
3
\$\begingroup\$

[Improved with flat_map thanks to @tokland.]

How about:

def index?
  user.roles.flat_map { |role| role.activities['sheet'] }.include?('index')
end

In Ruby, it is almost never necessary to set a temporary variable to empty and iterate over an object, given the power of methods like map, reduce, find, all?, etc. I don't see the need to use uniq! since it doesn't change whether index will be present or not. It's unclear why you're checking for nil on the first sheet_activities (since you can't guarantee what will be first without sorting), but if you just want to return false when sheet_activities are nil, the above will already do so.

Also note that Rubocop likes single quotes when you're not interpolating.

\$\endgroup\$
  • \$\begingroup\$ about first sheet_activities.nil? see my comment above... I also agree that .uniq! is not necessary , as I'll only check for the presence of a method (:index, ..:show..) so I don't care for duplicated methods... I juts thought it was "pretty' :-)) Thanks a lot ... \$\endgroup\$ – erwin Sep 19 '15 at 16:25
  • \$\begingroup\$ map + flatten(1) = flat_map \$\endgroup\$ – tokland Sep 20 '15 at 21:50

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.