Use cache_accesor for organization permissions related methods

[luchotc]

This is minor, but as part of performance improvements, I think we should use cache_accessor more often.

In particular, student_granted_organizations is used in core features like immersive redirections and profile completion and it's totally cacheable. Also, I was tempted to replace that Mumukit::Platform::Organization.find_by_name! for a Organization.where as Organization is being used in another places in this file, but I wanted to hear your opinion @flbulgarelli.

[flbulgarelli]

Looks good! However, a long time ago I benchmarked the cache_accessor and I got that the whole metaprogramming thing was costlier than the chaced method if its code was fast enough. So I'd recommend benchmarking both versions in order to decide if it is actual an improvement or new performance issue.

[flbulgarelli]

Looks good! However, a long time ago I benchmarked the cache_accessor and I got that the whole metaprogramming thing was costlier than the chaced method if its code was fast enough. So I'd recommend benchmarking both versions in order to decide if it is actual an improvement or new performance issue.

PS: I see that both methods are actually hitting the DB, so it makes sense to cache them when possible, although the net time it will take may be a little more - because ActiveRecord will cache select organization where name = ... SQL queries. So my recommendation of doing a benchmark still applies, but maybe we could do the cache on the revamp itself, which involves less introspection magic:

 revamp_accessor :any_granted_organizations, :student_granted_organizations do |selector, _, result|
      ... cache logic here using the selector ...
      result.map { |org| Mumukit::Platform::Organization.find_by_name!(org) rescue nil }.compact
  end

See like in https://github.com/mumuki/mumukit-core/blob/2f822e552f4f284b8a3563283e4b0c00a409d840/lib/mumukit/core/module.rb#L88

[luchotc]

@flbulgarelli I've implemented your suggestion. Nevertheless I find a disturbing scenario. I've created a test for it.

Memoization of this methods may cause issues for uncontextualized users. When an user is instanced, both methods are called in order to define the current_context with the sole purpose of assigning a random avatar if it does not have one. To be honest, I don't see how this could cause issues, as there isn't any feature that instances users, changes permissions and then use that to do something, but I think it could give us some headaches in the future :confused:. I'm inclined just to merge "Use safe_current" commit which is OK but is completely unrelated.

[luchotc]

Ok, the test actually passed :stuck_out_tongue: but I suspect it's because they are run together and that changes something. If you run just the last test I've added it fails, and that makes sense. It's because of what I've explained just above.

[luchotc]

If we do end up merging this one despite that possible issue, let's remove the last test.

[flbulgarelli]

PS: I agree with this:

I'm inclined just to merge "Use safe_current" commit which is OK but is completely unrelated.

[flbulgarelli]

This is not the first time I hit this problem in the last month. We should actually have some in-memory organization-sensitive cache store for a lot of things like that. Such cache would avoid a lot of usages queries - we could build an in-memory, organization-specific map for content-tree nodes.

[luchotc]

Yep, I agree with that last statement. In fact along with @julian-berbel we've proposed that same thing. He actually implemented some rudimentary version of that in the migration script but of course the actual implementation would be much more difficult.

In this case I see it just a memoization pitfall though

[luchotc]

I've should have let it go like Frozen but I couldn't. This solution is a bit more cumbersome but I think is the right one.

I've renamed methods in auth to better reflect what they do. They don't actually retrieve organizations but they retrieve organizations names. That allowed me to do a more clever memoization here. I've just memoized student_granted_organizations as for the other method would be more expensive to do it rather than doing nothing. I've also removed an unused method here, please check it out @flbulgarelli :smile:

You will see that I'm caching only the DB search. I'm a bit concerned that maybe this caching isn't truly speeding things as Rails does cache DB calls. I'm confident that, even Rails does that, it's cheaper this way :smile:

[luchotc]

Also, I was tempted to replace that map to a plain Organization.where(name: names) but we will lose order. I think that we shouldn't rely on that order though, but I'm pretty sure that we are doing that right now somewhere