Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed allocation expiration task bug #401

Merged

Conversation

aestoltm
Copy link
Collaborator

@aestoltm aestoltm commented Jun 1, 2022

Resolved #378 : Fixed bug that would only update allocation statuses of 'active' to 'expired', other statuses would stay as their current status after expiration date was reached.

NOTE: Need redis running and possibly MailHog to have the scheduler work

Tested fix through the following:

  1. Created many allocations of different statuses (also tested some allocations that were locked/unlocked and some that allowed change requests)
  2. Put all created allocations start and end dates so that they should be expired
  3. Through ColdFront Administration page, forced the DJANGO Q 'Scheduled tasks' to perform a status update.
  4. Force status update by changing 'Date' to 'Now' and 'Time' to 'Now', then click 'SAVE'
  5. Status updates should occur shortly after. Then check allocations for the expected status update.

Tested bug through the following:

  1. Tested 'active' status to 'expired'
  2. Tested bug of other statuses to 'expired' (found they did not switch their status as described in bug report)
  3. Follow steps 3-5 above to force scheduler to update statuses

…ses of 'active' to 'expired', other statuses would stay as their current status after expiration date was reached.
@dsajdak
Copy link
Contributor

dsajdak commented Jun 1, 2022

This is going to break the Invoice report but it needs to be done. On the roadmap is a rework of how we handle "invoices" or payments. If an invoice isn't paid on time, the allocation should be set to expired and access to the resource should be blocked.

@aestoltm aestoltm changed the title Resolved #387: Fixed bug that would only update allocation statuses o… Fixed allocation expiration task bug Jun 1, 2022
@aebruno aebruno requested a review from dsajdak June 2, 2022 19:35
Copy link
Contributor

@dsajdak dsajdak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@aebruno
Copy link
Member

aebruno commented Jun 3, 2022

One side effect of this change is that all allocations with an end_date in the past will now be changed to expired every time this job is run. Here's a few cases where this may not be desired:

  • Expired allocations that already have their status set to expired will get updated unnecessarily.
  • If an admin wanted to explicitly set the status of an expired allocation to something other than expired, this will get overwritten next time this job runs. With this change there will be no way to override this.
  • Sites that have added custom statuses may have different business logic that conflicts with this change

Perhaps a better approach would be to add other statuses here in the filter instead of removing the "Active". So maybe we come up with the list of statuses an allocation must be along with an end_date in the past that will trigger this change of status to "expired". That will make this job more predictable and allow other sites to modify the behavior if desired. Thoughts?

@dsajdak
Copy link
Contributor

dsajdak commented Jun 3, 2022

Yes, that is definitely not ideal. We would not want already expired allocations to get 'expired' notices again. Based on what you're saying I think this would also run on allocations that have been denied and revoked too. We would want this to run on the following statuses: active, payment pending, payment requested, and unpaid. What we were seeing is the allocation end date would arrive and unless the status was 'active' the allocation status would not change to 'expired' The users would still get an email saying their allocation had expired but the status was still set to payment pending or payment requested. This would affect the plugins that sync looking for the 'expired' status and users on the allocation would still have access to the resource.

@dsajdak dsajdak added the needs review Waiting for review label Jun 8, 2022
@aebruno aebruno requested a review from dsajdak June 14, 2022 11:50
Copy link
Member

@aebruno aebruno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@dsajdak dsajdak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested functionality and LGTM

@aebruno aebruno merged commit f5245d3 into ubccr:master Jun 14, 2022
@dsajdak dsajdak removed the needs review Waiting for review label Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

Allocation: Update status when expiration date is reached
3 participants