Refactor ApplicationPolicy #same_org?

thejonroberts commented 2 months ago

What github issue is this PR for, if any?

No issue. ApplicationPolicy#same_org? was just a little too wild, and allowed misuse of authorize.

What changed, and why?

ApplicationPolicy#same_org? has gotten out of hand... It should not worry about the class of the record it's getting, only what the record's associated CasaOrg is.

Any variation from that can be left to the sub-policies that deal with specific models. For example, I redefined same_org? for CasaOrgPolicy because the record is the casa_org itself, so record.casa_org is impossible. Between that and most of the Models involved not having a casa_org association defined, I can see how it got to where it is slowly over time. Changes in this PR:

Use model associations for same_org? check.
Override in Policy classes as needed (CasaOrg).
Change index actions that went through same_org? (ContactTopicPolicy)
Change member actions which authorized a class instead of the available @record. (ContactTopicController)
Fix broken specs (only 2)
Make an instructive comment in same_org? to discourage future abuse.

How is this tested? (please write tests!) 💖💪

Current tests only, as no behavior changes. If there is something that may be under tested, I can dig in. ~~I was seeing some failing tests locally but they happened again when I removed the changes. Curious to see what happens on CI.~~

elasticspoon commented 2 months ago

@thejonroberts oh but this does need a small lint fix

thejonroberts commented 2 months ago

Makes sense. Honestly, part of me wonders if we should even have the same org in the application policy? Maybe each individual policy should just be responsible for implementing the method.

Especially with the way you had to do the testing allow(record).to receive(:casa_org).and_return(casa_org)

Well, I obviously think otherwise. 😉 It's just going to look the same for all of the classes (14 and counting). Unless they don't have a proper association defined. But if the model doesn't have a relation to an org, what is it doing here in same_org??

I could have used a factory to create a record with an org association, like create :contact_type, casa_org: casa_org/other_casa_org, but which model to use? At one point, I thought about making an array of factories models and picking one at random! That is a good indication that it really doesn't matter!

So same_org? doesn't care what type of record it is, only if it returns the correct casa_org -- so we just make a record double and stub the .casa_org value it returns.

One thing I didn't test is when there is no casa_org association. I could add that case (would raise a NoMethodError), but seems like that should only come up during development, and would lead the dev to the comment?

elasticspoon commented 2 months ago

Makes sense. Honestly, part of me wonders if we should even have the same org in the application policy? Maybe each individual policy should just be responsible for implementing the method. Especially with the way you had to do the testing allow(record).to receive(:casa_org).and_return(casa_org)

Well, I obviously think otherwise. 😉 It's just going to look the same for all of the classes (14 and counting). Unless they don't have a proper association defined. But if the model doesn't have a relation to an org, what is it doing here in same_org??

I could have used a factory to create a record with an org association, like create :contact_type, casa_org: casa_org/other_casa_org, but which model to use? At one point, I thought about making an array of factories models and picking one at random! That is a good indication that it really doesn't matter!

So same_org? doesn't care what type of record it is, only if it returns the correct casa_org -- so we just make a record double and stub the .casa_org value it returns.

One thing I didn't test is when there is no casa_org association. I could add that case (would raise a NoMethodError), but seems like that should only come up during development, and would lead the dev to the comment?

Yea I see your point, I'm not totally sure how else you could approach testing it. It makes sense, it does a much better job expressing that you are basically duck typing that method.

thejonroberts commented 2 months ago

Added user.casa_org = nil & user = nil contexts to specs.

Also, after thinking about the discussion above, added some clarity to the record double setup, differentiating between org_record and other_org_record. Should make it easier to understand for future devs. 🤞🏻

rubyforgood / casa