refactor allocation status lists #816
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The list of allocation statuses is long and rather confusing. Coldfront's behavior with respect to these statuses is inconsistent. It seems as though the list of statuses has grown over time, and some statuses were added without also being accounted for in various filters and conditionals. This isn't surprising because accounting for all statuses in all filters and conditionals is a lot of work. I think many of these statuses would be more appropriate as tags.
Here are some rules I came up with on my own about how I think statuses were meant to be used (please correct me if I'm wrong):
New
Denied
Approved
Pending
Payment Requested
Payment Pending
Unpaid
Paid
Active
Renewal Requested
Expired
Revoked
Assuming the above rules are followed, Coldfront's behavior is still surprising in many situations:
Active
to statusExpired
, theallocation_disable
andallocation_remove_user
signals will not be triggered, so plugins cannot respond. These signals trigger only when an allocation changes to statusesDenied
orRevoked
.Expired
orRevoked
.New
,Paid
,Payment Pending
, andPayment Requested
count towards the allocation limit, but allocations with statusesApproved
,Payment Declined
,Pending
, andUnpaid
do not.Active
, but it can be removed from an allocation with statusRenewal Requested
.Expired
, but it can be removed from an allocation with statusRevoked
.This PR does not change this behavior in any way, instead it organizes the behavior for all situations in one file, so it can be easily understood. This PR also makes this behavior configurable, which is a nice bonus.
The tests all pass. For reference, here are the tests that fail if I make all the allocation status lists into empty lists:
Future work:
FIXME
comments have been added, these should be fixed as soon as this PR goes throughNew
,Active
,Inactive
, with all other statuses becoming tagsALLOCATION_STATUSES_UNSET_START_DATE_END_DATE
andALLOCATION_STATUSES_DO_DISABLE
should be separate conditionals, rather than having one nested in the other