refactor Perm like TokenPermissionType

[nicolasfranck]

Instead of reloading the permissions on every request, these are loaded once at class loading time.
This can probably be done more efficiently in one query..

[briri]

Thanks @nicolasfranck. I think this would be a good improvement. I added a comment about how you could accomplish the same with a single query.

Once this has been merged I will add a ticket to re-evaluate the methods on the User model and a few other spots in the code. I think we can eventually get rid of those shortcut methods (e.g. def self.grant_permissions) and just call the constants directly (e.g. Perm::GRANT_PERMISSIONS) once they're in place

Also be sure to rebase/merge from the development branch and run rubocop app/models/perm.rb on the file

[briri]

You could do this in one query with something like:

  # Preload all of the records and assign them to a Constant
  Perm.all.each { |perm| const_set(perm.name.upcase, perm) }

You would then have to alter a few of the shortcut methods below since names like GRANT_API would actually be GRANT_API_TO_ORGS

[nicolasfranck]

Cool @briri , I wasn't aware I could query like this at package level. I tried to do this in a class method, but that failed ..

[xsrust]

Neat, this is really clean

[xsrust]

Updating this branch as well. See #2741 (comment) for an explanation

[briri]

please run Rubocop and fix up any issues it complains about (you can run rubocop -a to have it auto-correct most of them)

it also appears that there are some downstream effects that are causing errors. I suspect the switch over to making these constants has caused a timing/load issue. For example:

 describe ".add_orgs" do
    subject { Perm.add_orgs }

    context "when name is 'add_orgs'" do
      let!(:perm) { create(:perm, name: "add_organisations") }
      it { is_expected.to eql(perm) }
    end
    
    # ...
end

I suspect that the use of let! here may be causing some problems. Try using a local variable and invoking the method directly inside a before hook instead to isolate out the RSpec subject/let helpers:

describe "" do
  before(:each)
    @perm = create(:perm, name: "add_organisations")
  end
  context "when name is 'add_orgs'" do
    expect(Perm.add_orgs).to eql(@perm)
  end
end

[nicolasfranck]

@briri I ran rubocop, and saved the changes, but I'm not sure how to make rspec run successfully (it is complaining about "Uninitialized constant FactoryBot"). No problem if you update the branch

[briri]

try running bin/rspec there may be some conflict between the gems you have installed locally and the ones used by the application.

[nicolasfranck]

Right, I see.

I think I found the problem: rspec is destructive in nature. That line Perm.all returns an empty result, so no constants are being created..

I will look for another way

[nicolasfranck]

Another problem: that DatabaseCleaner removes all records before each test. Each test recreates a record, so each of them has id 1. When it tries to compare that to the subject it fails, because they both have that same id. Apparently subject is called first, then that database cleaner, then let!, and then finally the test itself.

Confusing..

[briri]

Interesting, and that makes sense. In my opinion it is good practice for the DB records to be cleaned out after each test to prevent the records from leaking into other tests and introducing confusing results. I have been using instance variables over RSpec's subject! and let! which are built/created before the test and destroyed afterward. Something like:

# Instance variable approach
describe "my test" do
  before(:each) do
    @org = build(:org)
  end

  it "should pass" do
    expect(@org.is_a?(Org)).to eql(true)
  end
end

# ----------------------------------
# Instead of RSpec's subject let
describe "my test" do
  let!(:org) { build(:org) }

  it "should pass" do
    expect(org.is_a?(Org)).to eql(true)
  end
end

The tests are a mix of different approaches and many of them are outdated (written pre 2015). Going over the latest documentation for the latest versions of RSpec, Factorybot and Capybara is one of our goals for the future.