Add util for closing and reminting stale opportunities

[martha]

Merging this PR

• use the squash-merge strategy for PRs targeting a release-X branch

Description

• Add Hmis::Ce::OpportunityRefresher service class and associated rake task
• Refactor build_opportunity_for_unit into a helper on the unit class

How to test:

• Create a unit and mark it as accepting referrals.
• Add or modify a match rule that applies to that unit.
• Trigger reprocessing: Hmis::Ce::Match::CandidatePoolBuilder.call(force_reprocessing: true), Hmis::Ce::ProcessPoolsJob.perform_now
• In the UI you can now see that this unit's "eligibility rules" are out of date
• Run the ce_opportunity_refresh rake task; the out-of-date unit should be updated.

Type of change

New feature

Checklist before requesting review

• I have performed a self-review of my code
• I have run the code that is being changed under ideal conditions, and it doesn't fail
• If adding a new endpoint / exposing data in a new way, I have:
  • ensured the API can't leak data from other data sources
  • ensured this does not introduce N+1s
  • ensured permissions and visibility checks are performed in the right places
• Any major architectural changes are supported by an approved ADR (Architectural Decision Record)
• I have updated the documentation (or not applicable)
• I have added spec tests (or not applicable)
• I have provided testing instructions in this PR or the related issue (or not applicable)

//: # NOTE: system tests may fail if there is no branch on the hmis-frontend that matches the Source or Target branch of this PR. This is expected

[gigxz]

Might we want to just stick to refreshing open opportunities, rather than including locked opportunities here and then filtering them out below on line 25?

My understanding of the opportunity statuses is:
open = no active referral, "accepting referrals"
locked = has active referral
closed = closed and not accepting referrals

Maybe I'm missing some reasoning about wanting to have logging to identify the stale locked opportunities?

[martha]

Sure, that seems reasonable! And your understanding matches mine.

I think I've run into this question before at some point -- whether to trust that open status always means no active referral while locked means there is a referral, or to actually check the active_referral association and confirm. Maybe this points to a code smell? But I think in this case you're right that we can just scope to open opportunities.

Regarding the reason to log skipped opportunities: I was just doing this out of interest, since (I believe) if a referral is in progress but gets declined, that opportunity is now open again but is still stale. That might lead to some confusion with us (or eventually users) thinking "oh, I thought I refreshed all the opportunities, it didn't work?" However, probably a better way to track this would be to add some verbiage/warning alert on the Unit (opportunity) page noting that the requirements have gotten out of date. edit: something like this greenriver/hmis-frontend#1320

[gigxz]

Good point, and displaying the stale flag seems like a good solution.

Agreed about opportunity status, we don't have validations ensuring that the status is in line with the active_referral presence. It looks like opportunity status is updated from the engine. Currently there isn't a way to delete a referral (I don't think? though depending on what is proposed in #6000 we may introduce it), but that is one scenario where this bug could appear (eg. Referral is deleted and opportunity is not closed). I can try to address that additional validation in the PR

[gigxz]

Looks great! Just a few minor comments

[gigxz]

I went on a bit of a rabbit hole trying to remember how this works, so it might be worth adding a comment stating that the UnitGroup may or may not have a candidate_pool_id. Unit groups in projects supporting waitlist-based referrals typically have an associated candidate pool (once processing has run), and unit groups in projects only supporting direct referrals typically do not have an associated candidate pool (but they may have one left over if they used to support waitlists).

I opened https://github.com/open-path/Green-River/issues/8555 to clarify that behavior a bit and see if we need to add any more constraints or routines for when those configs change.

[martha]

Great point and thanks for the ticket. I did add a comment along the lines of what you're suggesting, with a todo linking it to that ticket so we remember to update the comment if the behavior changes. 1d8f591

[gigxz]

Is it ever expected that the number of opportunities closed and the number of opportunities created doesn't match? When would that come up?

If it's not expected, I think we should raise and rollback if that happens.

Again, I am not sure of the answer of my question, but if the numbers are expected to match maybe the logged object could be simplified:

        raise if closed_opportunity_unit_ids.uniq.size != created_ids.size
        {
          num_refreshed_units: closed_opportunity_unit_ids.size,
          refreshed_unit_ids: closed_opportunity_unit_ids,
          created_opportunity_ids: created_ids,
        }

[martha]

Oh, yep you are right it's unexpected! This extra bulky logging object was a relic of AI that I didn't critically cut down ✂️ . I'll make the change you're recommending.

• if an opportunity was recreated in between the close and create steps - no, this won't happen since it's inside of a transaction
• if there were somehow multiple stale opportunities per unit? - no, this shouldn't happen since there should only be one open opportunity per unit, which we validate on the opportunity

[gigxz]

maybe add a check that the candidate pools exist? I could see accidentally passing different ids (like project ids or unit group ids) to this routine, which would have unexpected behavior.

edit: i suppose that check could go in OpportunityRefresher? my concern is really with the rake task since the arg isn't named

[martha]

Good plan, I added this (1a48abb)

While testing that out, I also noticed that apparently passing a comma-separated list of args to a rake task is not as straightforward as I thought 🤦‍♀️ . Updated the usage comment 3ed4d94

[gigxz]

I think no rules = no candidate pool is not really true. Here's an example that I tested locally on this branch:

• add a match rule at the data source level
• go to project that only accepts direct referrals
• mark units as available. it will create opportunities that have assignment_rules but no candidate pool id.

Another example of this could be that the unit group was recently marked as supporting waitlists but the processing hasn't completed yet; so it has rules but is not yet associated to a candidate pool.

This is why I opened https://github.com/open-path/Green-River/issues/8555 because it feels like there is not clarity on what the expectations are. That said I think its probably fine (and good?) that opportunities get stored with assignment rules in all cases. It might make sense to just split this test out to test each case separately (unit group with no candidate pool; unit group with no match rules).

[martha]

I see, that's a good point this comment is inaccurate. I removed it and separated out the tests as you recommended.

I haven't added a test for the case where the unit group has a candidate pool, but no match rules. I couldn't think of a scenario for this other than when all rules were deleted and the candidate pool builder hasn't finished processing yet, which doesn't seem like it warrants its own test case (?) but lmk what you think! 3c3dd5f