feat(package-actions): Refactor logic for getPackageActions

[hannasage]

Purpose

This PR bolsters our package actions logic for forthcoming development by separating the logic for each rule into a single ActionRule associated with the rule.

Type definition

export type ActionRule = {
  action: Action;
  check: (
    checker: ReturnType<typeof ActionAvailabilityCheck>,
    user: CognitoUserAttributes,
    /** Keep excess parameters to a minimum **/
    ...any: any[]
  ) => boolean;
};

Furthermore, as seen above in the line ReturnType<typeof ActionAvailabilityCheck>, we also have a new utility object for performing many checks/frequently used conditions. This takes in a package and produces properties and methods for use in ActionRules

export const ActionAvailabilityCheck = ({
  seatoolStatus,
  rais,
  raiWithdrawEnabled,
}: OsMainSourceItem);

Linked Issues to Close

https://qmacbis.atlassian.net/browse/OY2-26859
https://qmacbis.atlassian.net/browse/OY2-26851

Approach

Previously, we had used a single function with a switch statement to build the array of Actions and give it back to the user. Now, we're still entering thru that function, but we've refactored the switch out. Using a switch only really gave us a single dimension of control per nested layer, meaning as complexities emerged in when to show what actions, and to whom, our switch would grow increasingly deep.

We now pair a single Action with a check function that takes in the package, user attributes, and the latest RAI object for convenience, and returns a boolean. This is then used to filter down the array of all actions into only actions passing these checks. The type also takes any additional parameters you need, but please keep them to a minimum, or if they're common use, elevate them to a defined parameter in the type, such as latestRai

Assorted Notes/Considerations/Learning

This PR checks in the addition of a new utility for the Authority PlanType enum values allowing us to check them against a set of valid plan types.

export const PlanTypeCheck = (planType: PlanType | null) => ({
  isSpa: checkPlan(planType, [PlanType.MED_SPA, PlanType.CHIP_SPA]),
  isWaiver: checkPlan(planType, []),
  /** Keep excess methods to a minimum with `is` **/
  is: (validPlanTypes: PlanType[]) => checkPlan(planType, validPlanTypes),
});

[hannasage]

Can't test til #271 merges

[mdial89f]

I really really like how this is broken out. And come to think of it, this aligns a bit in theory to what I saw from Brian yesterday, where the rules for a given action were maintained by / bound to the action itself, rather than a single big switch.

I expect testing will be tedious, since all actions need to be checked, but this is great.

[pkim-gswell]

To kick off WW3, but Im not a fan of enums. I think enums should be reserved for higher level domain(ish) use cases. For a custom utility example, like here, I lean towards type unions.

[hannasage]

This looks new, but it's just a name change from actions.ts. I'm indifferent about how we type based on constant values, but I will always advocate for those constant values being imported instead of typed as individual strings. Could we infer value types from PlanType to keep a string union type in line with the enum? That could serve as a middle of the road pattern while we're working towards more modularized usage of constant string values.

The more we build utilities like PlanCheck, we'll likely end up reducing how often we use any given constant value elsewhere, and could probably eventually do away with the enum, but for now they're used largely to keep all our rogue strings around the codebase in check where needed.

[hannasage]

Something to chew on: which of the following implementations would be preferable and why?

ActionAvailabilityCheck(data?.seatoolStatus).isInRaiStatus // direct boolean

ActionAvailabilityCheck.isInRaiStatus(data?.seatoolStatus) // called method, returns boolean

[pkim-gswell]

1️⃣ - I think this approach opens alot of possiblities. We could extend the functionality with flexibility.

Off the top of my head:

AuthorityService(user).is("cms");
AuthorityService(user).can("withdraw-rai")

PackageService(data).actions.can("withdraw-rai")
PackageService(data).status.is(SEATOOL_STATUS.UNKNOWN)
PackageService(data.seatoolStatus).status.is(
  [ SEATOOL_STATUS.PENDING, SEATOOL_STATUS.PENDING_OFF_THE_CLOCK, SEATOOL_STATUS.PENDING_APPROVAL]
)

[hannasage]

I like the idea of having methods like is and can, but the idea is to balance use of the "catch-all" method like AuthorityService.is() with our most common checks or more complex checks so method params can be baked into them and tested while bugs are relegated to the ad-hoc uses of the catch-all.

Like, the array [ SEATOOL_STATUS.PENDING, SEATOOL_STATUS.PENDING_OFF_THE_CLOCK, SEATOOL_STATUS.PENDING_APPROVAL] is basically second clock statuses, and when paired with other intrinsic attributes and conditions, you can use something like PackageService(data).isInSecondClock as a boolean check.

One thing I have thought about, too, is chaining checks on a single object type, akin to PackageService(data).isInSecondClock().canWithdrawRaiResponse() but without needing to return the object from a method each time. That's not an immediate need as we can just do checks.isInSecondClock && checks.canWithdrawRai but compound conditions like that also get remade a lot without need for ad-hoc control. I know a common pattern is PackageService(data).all([...]) where you can give it some value and it'll run all checks.

[github-actions]

🎉 This PR is included in version 1.5.0-val.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀