Soft delete too

[gkostin1966]

Added a spec for location soft delete … it is a bit rough since there is only a grant repo to work with … feel free to refactor it before merging.

[botimer]

For tests with database type, we include a shared_context that makes rom and relations available to shorten up these kinds of expressions.

As far as the choice between modifying the created rows vs getting the factory to set them up as we want in the first place, I don't have a strong opinion. The location/obj table itself is a bit messy with its composite key, so I would use a where on the collection rather than using by_pk.

The immediate reassignment also seems tricky to me -- I'd probably use a different name for the first/bad one or do the update in a tap with a temporary.

[malakai97]

standardrb will complain about using tap, fyi

[botimer]

I think the triple enclosing contexts obscure the purpose/specificity of these tests. It seems to me that the most important context hierarchy is when the entities are marked deleted.

This feels like a situation where conventional RSpec contexts/subjects/examples lead to a structure that is harder to approach without enough payoff. Please don't take this as a criticism of your work, though. I'm detailing a concern I have with a common pattern and a possible alternative.

The subject and implicit example stuff is supposed to make the tests easier to write and read more like a "specification", but I think the typical community suggestions just plain miss the mark by being too implementation- and DRY-focused. A concrete example from just below:

      context "when user soft deleted" do
        let!(:user) { Factory[:user, userid: "lauth-allowed", dlpsDeleted: "t"] }

        it { expect(grants).to eq [] }
      end

There is so much mental work up and down the file/tree to know what this example is supposed to be specifying and what's overridden, how. Pun alert: it's missing too much context for me. I also think it sort of lost its charm when the "should" syntax was removed. The implicit subject and is_expected.to is too cute for its own good, in my opinion. The concise form using the built-in matchers doesn't often make very good documentation either -- I really do not want see "eq []" in runner output that's supposed to be a domain-oriented explanation how something behaves. Basically, I think the motivation to make a DSL that reads more like a spec was great, but there is too much weird ruby stuff you have to do in between that it loses value -- you're caught switching between language intent, perspective levels, and enclosing scopes far too often.

Anyway, I'm wondering about a structure that is expressly about "when this is deleted", then specifying some behavior under there. Something like this, maybe:

• when a collection is deleted
  • grants on it do not apply
• when a location is deleted
  • grants on its collection do not apply
• when a grant is deleted
  • it does not apply
• when a user is deleted
  • individual grants for them do not apply
• when an institution is deleted
  • grants for named members do not apply
  • visitors from its networks do not receive its grants
• when an institutional network is deleted
  • when there is an enclosing, active network
    • visitors from the inner network receive the institution's grants
  • when there is no enclosing, active network
    • visitors from the inner network do not receive the institution's grants
• when an institution membership is deleted
  • grants for that member do not apply
• when a group is deleted
  • grants for members do not apply
• when a group membership is deleted
  • grants for that member do not apply

Each of the "when" pieces would be a context and the next level would be it/specify. There would be no subject at the top. There would be similar setup in each context, but it's subtly different for each. The 3-8 lines of setup are all pertinent for the full scope for the behavior specified therein. I think the locality and isolation are probably more valuable when considering that scenario than reducing the apparent duplication of text.

What do you think?

[malakai97]

I agree regarding the it without a string name for the test. I only like seeing that when quickly doing a bunch of very similar tests, like cartesian product stuff when writing a + method. Usually that's to get them all on the screen at once.

Super nitpick that goes against how both of you write your tests: whitespace between lets and the tests they apply to decreases readability for me. Similarly subject and let because subject is just an alias of let

[gkostin1966]

Lol. It was a quick and dirty one off because we don’t have repos for locations and collections. I still am no fan of traits.

[gkostin1966]

Redo