-
Notifications
You must be signed in to change notification settings - Fork 1
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
2627 improve mock data #2636
2627 improve mock data #2636
Conversation
@@ -3,17 +3,14 @@ | |||
"model": "registration.operation", | |||
"pk": "e1300fd7-2dee-47d1-b655-2ad3fd10f052", | |||
"fields": { | |||
"point_of_contact": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got errors when I removed this, but I don't think it's supposed to be used in reg2, to investigate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can handle this in the contacts migration ticket: https://github.com/orgs/bcgov/projects/123/views/16?filterQuery=repo%3Abcgov%2Fcas-registration+-status%3Adone+contact&pane=issue&itemId=93316267&issue=bcgov%7Ccas-registration%7C2672. I've added an AC to update mock data
a14e0d2
to
220956b
Compare
71331c3
to
7c9a410
Compare
0c1835a
to
ea5c67a
Compare
"latitude_of_largest_emissions": 43.5, | ||
"longitude_of_largest_emissions": -123.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Facilities with "Single Facility" type don't need lat/long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean "Small Aggregate"? We won't be able to remove lat/long until #2653 is in (where they're made optional)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Sorry my bad!
I think that PR will be merged sooner than this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried removing this on my end but it failed when I ran make reset_reg_data, realized because I haven't gotten the code from develop yet. So I'll be removing the long/lat from Small Aggregate once the code's updated.
Rebased and the long/lat is already removed!
"model": "registration.facility", | ||
"pk": "67fd8288-422b-43d3-a9b1-e4ddc54e2139", | ||
"fields": { | ||
"created_by": "3fa85f64-5717-4562-b3fc-2c963f66afa6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This user has Pending
status in the user operator mocks; using another user or marking this user as approved might be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing this! I forgot to check that the users made sense within the business requirements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related, we should update the bceid_business_name
in cas-registration/bc_obps/registration/fixtures/mock/user.json
to match any changed operator names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BCerki, I saw the thread in Teams about whether bceid_business_name is related to operator legal name, and I had the same question. Is my understanding correct that it's not exactly the same (but technically they are?) and there can be subtle differences, but do you guys want me to just go ahead and update bceid_business_name to reflect the changed operator names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's make them match the operator names for the purposes of mock data
{ | ||
"model": "registration.facilitydesignatedoperationtimeline", | ||
"fields": { | ||
"created_by": "00000000-0000-0000-0000-000000000001", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This user is associated with operator 4a792f0f-cf9d-48c8-9a95-f504c5f84b12
, and this operator only has one operation with pk 62d5d8ea-b163-4a83-95a4-bfadbb6b21f7
. Therefore, this user could not create this record because the operation pk is not associated with the user's operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this by changing the operator, I put Bojangles' pk instead since that one's missing the facility. Please let me know if the data is still inconsistent 😄
"regulated_products": [], | ||
"status": "Not Started", | ||
"status": "Registered", | ||
"bc_obps_regulated_operation": "21-0014", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
24-0014
or 25-0014
since it was created in 2024.
"regulated_products": [], | ||
"status": "Not Started", | ||
"regulated_products": [3], | ||
"status": "Closed", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have Closed
status?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing more statuses that are not in the Operation data model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the statuses in the reg1 mock data (bc_obps/registration/fixtures/mock/v1/operation.json) can stay as is to pass CI, but for reg2 mocks, yes, we should only have statuses that are in the Operation model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I got some statuses that are not part of the model, I don't recall anymore sorry about that!
"end_date": "2024-06-28T23:21:01Z", | ||
"status": "Closed" | ||
"created_at": "2024-2-01T15:27:00.000Z", | ||
"created_by": "ba2ba62a-1218-42e0-942a-ab9e92ce8822", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a user with ba2ba62a-1218-42e0-942a-ab9e92ce8822
user_guid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to have created_at
and created_by
for all records in this fixture.
"documents": [], | ||
"name": "Operation 2", | ||
"name": "Banana LFO - Registered", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this AC: All LFO operations (except one) should have at least two facilities
LFOs without facilities shouldn't be registered (though I don't think we enforce this). Can we change the status and name of whichever one doesn't have facilities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 2 LFO operations in the file, this (Banana) and Apple. Apple has only 1 facility registered, Banana has 20 registered. Would you prefer to remove the facility registered in Apple so it doesn't have anything under it and I can change the Status and Name to reflect that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, or we can make a third LFO with no facilities, whatever's easiest
"name": "Facility 26", | ||
"type": "Single Facility", | ||
"swrs_facility_id": 1026, | ||
"bcghg_id": "13219990029", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove bcghg_id from some of the facilities? This will make it easier for testers to assign them. There's a note about having done this in the PR description but I think it may have been lost? I don't see it in the referenced commit
@@ -232,5 +232,185 @@ | |||
"operation": "1bd04128-d070-4d3a-940a-0874c4956181", | |||
"start_date": "2024-06-28T23:20:02Z" | |||
} | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We either need to update both timeline fixtures to match the transfer_event fixture, or change that to match this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest prioritizing the population of timeline tables first, as they are more critical. The transfer_event fixtures can be handled afterward as the final step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the thorough review! I realized that some of the data might've gotten incorrect because of the merge conflict I tried to fix last time. This commit addresses these:
While I was fixing the code, I did have some questions:
|
3b4f50b
to
c2f3d72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The first two digits of the BORO ID must correspond to the year it was issued. For example, an ID such as 21-0001
cannot be issued on 2023-10-13
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User 00000000-0000-0000-0000-000000000004
has a pending access request to operator 685d581b-5698-411f-ae00-de1d97334a71
so we can't use this user for created_by
fields in this fixture.
320a3f0
to
c4ed199
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some facilities were created by user 3fa85f64-5717-4562-b3fc-2c963f66afa6
, which is associated with the operator 438eff6c-d2e7-40ab-8220-29d3a86ef314
, but that operator doesn't own the operation this facility is associated with. For instance, facility f486f2fb-62ed-438d-bb3e-0819b51e3aeb
is associated with operation 002d5a9e-32a6-4191-938c-2c02bfec592d
, but this operation is linked to operator 4242ea9d-b917-4129-93c2-db00b7451051
, which is not the same as the user's operator.
a373521
to
5f9dec7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤩 Great work!
…_operation_timeline to reflect realistic user, fix facility bcghg_id distribution
5f9dec7
to
1415ccf
Compare
card: https://github.com/orgs/bcgov/projects/123/views/16?pane=issue&itemId=91540613&issue=bcgov%7Ccas-registration%7C2627
Notes:
develop
and replace with this