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

2427 internal users can transfer a facility or operation #2557

Merged
merged 53 commits into from
Dec 14, 2024

Conversation

Sepehr-Sobhani
Copy link
Contributor

@Sepehr-Sobhani Sepehr-Sobhani commented Dec 6, 2024

ISSUE 1949
&&
ISSUE 2427

🗒️ NOTES:

  • Added cas_analyst as the new user role
  • Excluded all from backend error util if error was a generic backend error (We don't want to see All:<rest of the error> in the UI)
  • Removed unused jsonSchema
  • Fixed allowedRoles prop on dashboard tile links
  • Updated some fixtures
  • We still need to add the cronjob (Reg\Transfer Entity\Cronjob to apply transfers on effective dates #2040) to be able to transfer entities in the future

🧪 TEST:

  • Log in as cas_analyst
  • Click the Transfer link
  • Click Make a Transfer button
  • See transfer form

@Sepehr-Sobhani Sepehr-Sobhani force-pushed the 2427-internal-user-can-transfer-a-facility branch from 1f49c5b to 0fd9214 Compare December 6, 2024 23:44
@Sepehr-Sobhani Sepehr-Sobhani changed the title 2427 internal user can transfer a facility 2427 internal users can transfer a facility or operation Dec 6, 2024
@Sepehr-Sobhani Sepehr-Sobhani force-pushed the 2427-internal-user-can-transfer-a-facility branch 2 times, most recently from e3ac1c1 to 294eb31 Compare December 12, 2024 00:59
Copy link
Contributor

@BCerki BCerki left a comment

Choose a reason for hiding this comment

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

I'm making some notes here, not finished testing yet so I might rescind them as I get further along.

Is the transfers dashboard meant to have another tile? (If it is I suggest separate card and PR)
image

This might just be a dev data thing, but I don't think I'm getting the full list of operations. Existing Operator 2 has lots of operations but I'm only getting Operation 7:
Screenshot from 2024-12-12 09-14-05

If I transfer something with a date in the past, I get a confirmation that says it's already been transferred, and then I see the transferred status in the table instead of complete. Is that correct?
image

Copy link
Contributor

@BCerki BCerki left a comment

Choose a reason for hiding this comment

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

Made some comments, anything that has a w in it means I want to talk about it in the walkthrough

@@ -39,3 +40,25 @@ def list_timeline_by_operation_id(
sort_by = f"{sort_direction}{sort_field}"
base_qs = cls.get_timeline_by_operation_id(user, operation_id)
return filters.filter(base_qs).order_by(sort_by)

@classmethod
def get_current_timeline(
Copy link
Contributor

Choose a reason for hiding this comment

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

At first I wasn't sure what this service is meant to do based on the name and return value. Are we trying to get the info for whomever the facility currently belongs to? If so, get_current_designated_operation might be clearer.

If I've understood the PR correctly (I made some notes in the docs), then I suggest adding a comment to explain why there's only going to be one record without an end_date

- Create a new timeline record using the
`FacilityDesignatedOperationTimelineDataAccessService.create_facility_designated_operation_timeline` service. This
new record links the facility to the new operation, sets the start date to the transfer's effective date, and sets
the status to `ACTIVE`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest adding what happens in the TransferEvent model at this point

3. Create a new timeline record using the
`OperationDesignatedOperatorTimelineDataAccessService.create_operation_designated_operator_timeline` service. This
new record links the operation to the new operator, sets the start date to the transfer's effective date, and sets
the status to `ACTIVE`.
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

docs/backend/events/transfer.md Show resolved Hide resolved
to a new operator. These events can be initiated for a specific date in the future, or they can be set to take effect
immediately.

## How do Transfer Events Work?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest adding something about how the models, statuses, and dates all interact throughout this process. That was the most confusing part for me. I'll type up how I ~think it works for looking at the code, feel free to use some of this in the docs if it's helpful.

When an internal user creates a transfer, it goes into the TransferEvent model with a status of TO_BE_TRANSFERRED and the effective date from the form. (Provided there aren't any existing transfers that it overlaps with.) If the effective date is in the past, the transfers is immediately processed and a record is created in the timeline table with a status of ACTIVE and an empty end date. If there was already a timeline record, that one gets an end date set and the status set to TRANSFERRED. The TransferEvent model then gets the status set to COMPLETE? (When would we ever use the TRANSFERRED status in this model?)

If the effective date is in the future, nothing happens to the timeline tables. Once the date passes, the chronjob adds to the timeline tables as described above.

As a result of all this, we can always trust that the record in the timeline table that doesn't have an end_date is the active one. (We never check or filter for ACTIVE status, but checking for a null end date is the same thing.)

Comment on lines +242 to +249
fromOperatorId={formState.from_operator}
toOperatorId={formState.to_operator}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we pass the legal_name instead of the id? (Is that stored in the form state anywhere, or only in the schema?) Then we wouldn't need to additionally pass operators

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not storing the legal_name here, we just store IDs, that is why we need to pass the whole operators down.

@Sepehr-Sobhani
Copy link
Contributor Author

I'm making some notes here, not finished testing yet so I might rescind them as I get further along.

Is the transfers dashboard meant to have another tile? (If it is I suggest separate card and PR) image

I'm not aware, so for now, I'm going to remove that sub-dashboard step because it doesn't make sense.

…nd added end_date as a new filter

Signed-off-by: SeSo <[email protected]>
…ed by the same facility uuid for different transfers

Signed-off-by: SeSo <[email protected]>
Signed-off-by: SeSo <[email protected]>
@Sepehr-Sobhani Sepehr-Sobhani force-pushed the 2427-internal-user-can-transfer-a-facility branch from 97cdab7 to eabb344 Compare December 13, 2024 22:39
2. **Timeline Update:**
- The operation's current timeline (linking it to its current operator) is updated similarly to facilities.
- A new timeline record is created for the new operator.
3. **TransferEvent Status:** Updated to `TRANSFERRED`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great, Sepehr, thanks for including so much detail and the examples!

Update the operator for the operation
At the time of implementation, this is only used for transferring operations between operators and,
is only available to cas_analyst users
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the docs

Copy link
Contributor

@BCerki BCerki left a comment

Choose a reason for hiding this comment

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

Great work on this!!

Signed-off-by: SeSo <[email protected]>
@Sepehr-Sobhani Sepehr-Sobhani merged commit 1b89c66 into develop Dec 14, 2024
42 checks passed
@Sepehr-Sobhani Sepehr-Sobhani deleted the 2427-internal-user-can-transfer-a-facility branch December 14, 2024 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants