samvera / hydra-head

Samvera Repository Rails Engine
Other
98 stars 41 forks source link

Clarify Embargo/Lease Terminology #511

Open no-reply opened 4 years ago

no-reply commented 4 years ago

Descriptive summary

Embargoes and Leases use unclear and inconsistent terminology to describe their current and future states. Some important states are completely unnamed and are checked in client code via presence of *_release_date (which it might actually be nice to keep around).

We have:

I might try to draw a state diagram for this later.

In the meanwhile there's an important, unnamed state when the release date has passed but has not been deactivated (under present terminology (maybe) the embargo, is "inactive", but not "deactivated"). The only way to tell whether this is the case is to inspect embargo/lease_release_date and infer that, because it is present the embargo is still... ummmm... not-deactivated.

Rationale

The current state of affairs makes things confusing, even for experienced Samvera devs (:wave:)

Related work

https://github.com/samvera/hyrax/issues/4299#issuecomment-624950545

no-reply commented 4 years ago

there's also Embargoable#under_embargo? and Embargoable#active_lease? which further muddy the naming waters.

https://github.com/samvera/hydra-head/blob/4ffa873d713a418e03d81688d18948595951ae1d/hydra-access-controls/app/models/concerns/hydra/access_controls/embargoable.rb#L51-L57

jeremyf commented 4 years ago

At one point, I was considering a "visibility schedule" abstraction. Fundamentally, we have an embargo. From beginning of time until the embargo date. And during that time frame there are "magic" viewer rights. Perhaps institution? Perhaps only explicitly assigned people. Similarly we have lease. From the begining of time until the lease date, it's open-ish. Then collapses into a dark state.

The logic is leaky AND if an institution gets it wrong there can be punitive consequences. As such, I believe we should spend some energy to design a more clearly articulated implementation.

no-reply commented 4 years ago

And during that time frame there are "magic" viewer rights.

i don't think this is true. Embargo/Lease function as data objects that are used to manage editing ACLs (recall that "visibility" is just a less granular version of ACLs; see also: https://github.com/samvera/hyrax/wiki/Hyrax-Valkyrie-Usage-Guide#permissions). there aren't/shouldn't be any magic permissions, and AFAICT it is totally possible for an embargo to be "active" but for the intended permissions to not be applied.


i might suggest a cleaned up version of what exists:

all names can probably be refined.

under this model, we have the following states

current? active? appropriate action notes
true true :ok: this might be summarized as #enforced?
false true #release this is the state currently represented in the UI using #present? checks, maybe #needs_release?
true false #apply danger zone. objects in this state likely have ACLs in violation of policy
false false :ok:

note that there's very little difference in implementation between the embargo and lease classes as-is. both are more-or-less a set of permissions "during", a set of permissions "after", and a release date, and there's nothing about the data structure that stops an embargo's "during" permissions from being looser than its "after" permissions; i.e. an embargo can be effectively a lease, and vice versa. simplifying to one implementation for these two concepts seems good.