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

510: is_application_lead_external 'no' error #534

Merged
merged 6 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 27 additions & 24 deletions bc_obps/registration/api/operation.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,18 +207,21 @@ def update_operation(request, operation_id: int, submit: str, payload: Operation
application_lead = Contact.objects.get(id=application_lead_id)
application_lead_address_id = application_lead.address.id if application_lead.address else None

is_application_lead_external = payload.is_application_lead_external
if is_application_lead_external is True: # the user has assigned a contact
address, _ = Address.objects.update_or_create(
id=application_lead_address_id,
defaults={
"street_address": payload.street_address,
"municipality": payload.municipality,
"province": payload.province,
"postal_code": payload.postal_code,
},
)
eal, _ = Contact.objects.update_or_create(
is_user_application_lead = payload.is_user_application_lead

# create or update the address for either user or external application lead
address, _ = Address.objects.update_or_create(
id=application_lead_address_id,
defaults={
"street_address": payload.street_address,
"municipality": payload.municipality,
"province": payload.province,
"postal_code": payload.postal_code,
},
)

if is_user_application_lead is True: # the application lead is the user
al, _ = Contact.objects.update_or_create(
id=application_lead_id,
defaults={
"first_name": payload.first_name,
Expand All @@ -230,24 +233,24 @@ def update_operation(request, operation_id: int, submit: str, payload: Operation
"address": address,
},
)
eal.set_create_or_update(modifier=user)
operation.application_lead = eal
al.set_create_or_update(modifier=user)
operation.application_lead = al

if is_application_lead_external is False: # the lead is the user
al, _ = Contact.objects.update_or_create(
if is_user_application_lead is False: # the application lead is an external user
eal, _ = Contact.objects.update_or_create(
id=application_lead_id,
defaults={
"first_name": user.first_name,
"last_name": user.last_name,
"position_title": user.position_title,
"email": user.email,
"phone_number": user.phone_number,
"first_name": payload.external_lead_first_name,
"last_name": payload.external_lead_last_name,
"position_title": payload.external_lead_position_title,
"email": payload.external_lead_email,
"phone_number": payload.external_lead_phone_number,
"business_role": BusinessRole.objects.get(role_name="Operation Registration Lead"),
"address": user.address,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

address doesn't exist in the user model and was causing the error.

"address": address,
},
)
al.set_create_or_update(modifier=user)
operation.application_lead = al
eal.set_create_or_update(modifier=user)
operation.application_lead = eal

# updating only a subset of fields (using all fields would overwrite the existing ones)
payload_dict: dict = payload.dict(
Expand Down
14 changes: 10 additions & 4 deletions bc_obps/registration/schema/operation.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,20 @@ class OperationUpdateIn(ModelSchema):
first_name: Optional[str] = None
last_name: Optional[str] = None
position_title: Optional[str] = None
email: Optional[str] = None
phone_number: Optional[str] = None
# external application lead details
external_lead_first_name: Optional[str] = None
external_lead_last_name: Optional[str] = None
external_lead_position_title: Optional[str] = None
external_lead_email: Optional[str] = None
external_lead_phone_number: Optional[str] = None
# shared application lead details
street_address: Optional[str] = None
municipality: Optional[str] = None
province: Optional[str] = None
postal_code: Optional[str] = None
email: Optional[str] = None
phone_number: Optional[str] = None
is_application_lead_external: Optional[bool] = None
is_user_application_lead: Optional[bool] = None
operation_has_multiple_operators: Optional[bool] = False
multiple_operators_array: Optional[list] = None

Expand All @@ -66,7 +73,6 @@ class OperationOut(ModelSchema):
bcghg_id: Optional[str] = None
opt_in: Optional[bool] = None
verified_at: Optional[date] = None
is_application_lead_external: Optional[bool] = None
application_lead: Optional[ContactSchema]
operation_has_multiple_operators: Optional[bool] = Field(False, alias="operation_has_multiple_operators")
multiple_operators_array: Optional["List[MultipleOperatorOut]"] = Field(None, alias="multiple_operator")
Expand Down
14 changes: 12 additions & 2 deletions bc_obps/registration/tests/endpoints/test_operations_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
UserOperator,
RegulatedProduct,
BusinessStructure,
BusinessRole,
)
from registration.schema import OperationCreateIn, OperationUpdateIn
from registration.tests.utils.helpers import CommonTestSetup, TestUtils
Expand Down Expand Up @@ -81,7 +82,6 @@ def test_unauthorized_roles_cannot_post(self):
assert post_response.status_code == 401

def test_unauthorized_roles_cannot_put_operations(self):

operation = baker.make(Operation)
mock_operation = TestUtils.mock_OperationUpdateIn()
# IRC users can't put
Expand Down Expand Up @@ -151,7 +151,6 @@ def test_get_method_with_mock_data(self):

# POST
def test_authorized_roles_can_post_new_operation(self):

operator = baker.make(Operator)
mock_operation = TestUtils.mock_OperationCreateIn(operator)
post_response = TestUtils.mock_post_with_auth_role(
Expand Down Expand Up @@ -346,6 +345,7 @@ def test_put_operation_update_status_invalid_data(self):
def test_put_operation_without_submit(self):
operation = baker.make(Operation)
mock_operation = TestUtils.mock_OperationUpdateIn()

# approve the user
baker.make(
UserOperator,
Expand Down Expand Up @@ -421,6 +421,16 @@ def test_put_operation_with_application_lead(self):
documents=[],
application_lead=contact2.id,
operator_id=operator.id,
is_user_application_lead=True,
street_address='19 Evergreen Terrace',
municipality='Springfield',
province='BC',
postal_code='V1V 1V1',
first_name='Homer',
last_name='Simpson',
email="homer@email.com",
position_title='Nuclear Safety Inspector',
phone_number='+17787777777',
)
TestUtils.authorize_current_user_as_operator_user(self, operator)
put_response = TestUtils.mock_put_with_auth_role(
Expand Down
15 changes: 15 additions & 0 deletions bc_obps/registration/tests/utils/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ def mock_OperationCreateIn(operator: Operator = None):
documents=[document.id],
application_lead=application_lead.id,
operator=operator.id,
is_user_application_lead=True,
street_address='19 Evergreen Terrace',
municipality='Springfield',
province='BC',
postal_code='V1V 1V1',
)

@staticmethod
Expand Down Expand Up @@ -105,6 +110,16 @@ def mock_OperationUpdateIn():
documents=[document.id],
application_lead=application_lead.id,
operator_id=operator.id,
is_user_application_lead=True,
first_name="Homer",
last_name="Simpson",
email="homer@email.com",
phone_number="+17787777777",
street_address="19 Evergreen Terrace",
position_title="Nuclear Safety Inspector",
municipality="Springfield",
province="BC",
postal_code="V1V 1V1",
)

@staticmethod
Expand Down
29 changes: 1 addition & 28 deletions client/app/components/form/OperationsForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ interface Props {

export default function OperationsForm({ formData, schema }: Readonly<Props>) {
const { data: session } = useSession();
const userEmail = session?.user?.email;

const [operationName, setOperationName] = useState("");
const [error, setError] = useState(undefined);
Expand All @@ -31,32 +30,6 @@ export default function OperationsForm({ formData, schema }: Readonly<Props>) {
const operationId = params?.operation;
const isCreate = params?.operation === "create";

const isApplicationLeadExternal =
userEmail !== formData?.application_lead?.email;

// empty array is not a valid value for multiple_operators_array as empty default should be [{}]
// to avoid buggy behaviour opening
const isMultipleOperatorsArray =
formData &&
Array.isArray(formData?.multiple_operators_array) &&
formData.multiple_operators_array.length > 0;

// We need to convert some of the information received from django into types RJSF can read.
const transformedFormData = {
...formData,
// we only add the application lead data to the formData (ie, show it in the form) if the application lead is external (ie, someone other than the user)
...(isApplicationLeadExternal && formData?.application_lead),
// If you spread anything and it has the same keys as operation (e.g. id, created_by), watch out for accidentally overwriting things. In this case it's safe to spread because the address schema excludes fields
...formData?.application_lead?.address,
"Did you submit a GHG emissions report for reporting year 2022?":
formData?.previous_year_attributable_emissions ? true : false,
is_application_lead_external: isApplicationLeadExternal,
// fix for null values not opening the multiple operators form if loading a previously saved form
multiple_operators_array: isMultipleOperatorsArray
? formData?.multiple_operators_array
: [{}],
};

const formSectionList = Object.keys(schema.properties as any);
const isNotFinalStep = formSection !== formSectionList.length;
const isFinalStep = formSection === formSectionList.length;
Expand Down Expand Up @@ -87,7 +60,7 @@ export default function OperationsForm({ formData, schema }: Readonly<Props>) {
<MultiStepFormBase
baseUrl={`/dashboard/operations/${operationId}`}
cancelUrl="/dashboard/operations"
formData={transformedFormData}
formData={formData}
setErrorReset={setError}
disabled={
session?.user.app_role?.includes("cas") ||
Expand Down
58 changes: 57 additions & 1 deletion client/app/components/routes/operations/form/Operation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import OperationsForm, {
} from "@/app/components/form/OperationsForm";
import { operationSchema } from "@/app/utils/jsonSchema/operations";
import { BusinessStructure } from "@/app/components/routes/select-operator/form/types";
import { UserProfileFormData } from "@/app/components/form/formDataTypes";
import { RJSFSchema } from "@rjsf/utils";
import { actionHandler } from "@/app/utils/actions";
import OperationReview from "./OperationReview";
Expand All @@ -12,6 +13,13 @@ import { Fade } from "@mui/material";
import { Status } from "@/app/utils/enums";
import { Operation as OperationInt } from "@/app/components/routes/operations/types";

// 🚀 API call: GET user's data
async function getUserFormData(): Promise<
UserProfileFormData | { error: string }
> {
return actionHandler(`registration/user-profile`, "GET", "");
}

// 🛠️ Function to fetch NAICS codes
async function getNaicsCodes() {
try {
Expand Down Expand Up @@ -194,6 +202,54 @@ export default async function Operation({ numRow }: { numRow?: number }) {
operation &&
[Status.REJECTED, Status.APPROVED].includes(operation?.status as Status);

let userProfileFormData: UserProfileFormData | { error: string } =
await getUserFormData();

const formData = {
...userProfileFormData,
...operation,
};

const userEmail = (userProfileFormData as UserProfileFormData)?.email;
const applicationLeadEmail = formData?.application_lead?.email;
// If the current user is the application lead, we want to show the application lead fields
const isUserApplicationLead =
userEmail === applicationLeadEmail && applicationLeadEmail !== undefined;

Comment on lines +216 to +218
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't entirely sure if this was enough for the check. I figured that if they didn't click the button but entered the same email there was just a misunderstanding so we would check this off.

Also I tried to do this with a function similar to the def user_is_senior_officer in the UserOperator model and then a resolver in the schema but since we don't have access to User unlike in that table I decided to take care of this on the front end.

Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation is making me think a bit here. I don't see any particular problem with this check, but it just feels a bit weird to me. Let's chat quickly so I can fully understand what's happening here

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally works for now, we probably want to review this implementation later to see if there is a best practice or standard that we can follow for future conditionals. I've added a tech debt card #602 to review this and start a conversation.

// empty array is not a valid value for multiple_operators_array as empty default should be [{}]
// to avoid buggy behaviour opening
const isMultipleOperatorsArray =
formData &&
Array.isArray(formData?.multiple_operators_array) &&
formData.multiple_operators_array.length > 0;

// We need to convert some of the information received from django into types RJSF can read.
const transformedFormData = {
...formData,
// Add the correct application lead data
...(isUserApplicationLead
? {
...formData?.application_lead,
}
: {
external_lead_first_name: formData?.application_lead?.first_name,
external_lead_last_name: formData?.application_lead?.last_name,
external_lead_email: formData?.application_lead?.email,
external_lead_phone_number: formData?.application_lead?.phone_number,
external_lead_position_title:
formData?.application_lead?.position_title,
}),
// If you spread anything and it has the same keys as operation (e.g. id, created_by), watch out for accidentally overwriting things. In this case it's safe to spread because the address schema excludes fields
...formData?.application_lead?.address,
"Did you submit a GHG emissions report for reporting year 2022?":
formData?.previous_year_attributable_emissions ? true : false,
is_user_application_lead: isUserApplicationLead,
// fix for null values not opening the multiple operators form if loading a previously saved form
multiple_operators_array: isMultipleOperatorsArray
? formData?.multiple_operators_array
: [{}],
};

Comment on lines +219 to +252
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this one file up to keep OperationsForm cleaner as we do the same for the UserOperatorMultistepForm. I think it makes sense to fetch and transform all data in the server components when possible, then pass it do the client components.

// Render the OperationsForm component with schema and formData if the operation already exists
return (
<>
Expand All @@ -213,7 +269,7 @@ export default async function Operation({ numRow }: { numRow?: number }) {
activities,
businessStructuresList,
)}
formData={operation as OperationsFormData}
formData={transformedFormData as OperationsFormData}
/>
</>
);
Expand Down
2 changes: 1 addition & 1 deletion client/app/components/routes/operations/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@ export interface Operation {
reporting_activities: Array<any>; // Change this once we have the ReportingActivity type
operator_id: number;
naics_code_id: number;
is_application_lead_external?: boolean;
is_user_application_lead?: boolean;
multiple_operators_array?: Array<any>; // Change this once we have the MultipleOperator type
}
Loading
Loading