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

RAS-939 Survey Status Change #914

Merged
merged 19 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from 18 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
4 changes: 2 additions & 2 deletions _infra/helm/response-operations-ui/Chart.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ type: application

# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
version: 3.1.53
version: 3.1.54

# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application.
appVersion: 3.1.53
appVersion: 3.1.54
35 changes: 35 additions & 0 deletions response_operations_ui/contexts/case.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
from flask import url_for


def build_response_status_context(
ru_ref: str,
survey_short_name: str,
case_group_id: str,
ce_period: str,
allowed_transitions_for_case: dict,
survey_id: str,
has_reporting_unit_permission: bool,
) -> dict:
if has_reporting_unit_permission:
Copy link
Contributor

Choose a reason for hiding this comment

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

You have kind of doubled up here with the frontend and checked permissions twice ({% if isReportingUnitPermission %} in the template), so else is never reachable. However we shouldn't need to do isReportingUnitPermission in the template, as it's been done in the back end. Depends on what else uses it on how to pass it through, if it's just this then checking change_response_status is not null would work

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've removed this permissions check from the frontend and made it just check for it existing

change_response_status = {
"url": url_for(
"case_bp.update_response_status",
ru_ref=ru_ref,
survey=survey_short_name,
case_group_id=case_group_id,
period=ce_period,
),
"radios": _generate_radios(allowed_transitions_for_case),
"cancel_link": url_for("reporting_unit_bp.view_reporting_unit_survey", ru_ref=ru_ref, survey_id=survey_id),
}
else:
change_response_status = {}
return {"change_response_status": change_response_status}


def _generate_radios(allowed_transitions_for_case: dict):
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing the return type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added return type

radios = [
{"id": f"state-{index + 1}", "label": {"text": status}, "value": event}
for index, (event, status) in enumerate(allowed_transitions_for_case.items())
]
return radios
231 changes: 231 additions & 0 deletions response_operations_ui/contexts/reporting_units.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,231 @@
from flask import url_for

CE_STATUS_CLASS = {
"Not started": "ons-status--info",
"In progress": "ons-status--pending",
"Completed": "ons-status--success",
"Completed by phone": "ons-status--success",
"No longer required": "ons-status--dead",
}

ENROLMENT_ENABLED = "ENABLED"
ENROLMENT_PENDING = "PENDING"


def build_reporting_units_context(
collection_exercises: list, ru: dict, survey: dict, respondents: list, case, unused_iac: str, permissions: dict
) -> dict:
collection_exercise_section = _build_collection_exercise_section(
collection_exercises, ru["sampleUnitRef"], survey["shortName"], permissions["reporting_unit_edit"]
)
respondents_section = _build_respondents_section(
respondents,
case["id"],
collection_exercises[0]["id"],
collection_exercises[0]["tradingAs"],
ru["name"],
ru["sampleUnitRef"],
ru["id"],
survey["surveyRef"],
survey["shortName"],
survey["id"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry my fault, I wasn't clear. If it's 1 or 2 elements in a list we should pass the value, more than that pass the list. It's a balance act between passing what it needs and how many vars for the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yeah that makes more sense - updated

unused_iac,
permissions,
)

return {"collection_exercise_section": collection_exercise_section, "respondents_section": respondents_section}


def _build_collection_exercise_section(
collection_exercises: list, sample_unit_ref: str, survey_short_name: str, reporting_unit_permission: bool
) -> list:
table = [
{
"status_class": (
CE_STATUS_CLASS[ce["responseStatus"]]
if ce["responseStatus"] in CE_STATUS_CLASS.keys()
else "ons-status--error"
),
"hyperlink": url_for(
"case_bp.get_response_statuses",
ru_ref=sample_unit_ref,
survey=survey_short_name,
period=ce["exerciseRef"],
),
"hyperlink_text": "Change" if reporting_unit_permission else "View",
"period": ce["exerciseRef"],
"reporting_unit_name": ce["companyName"],
"trading_as": ce["tradingAs"],
"region": ce["companyRegion"],
"response_status": ce["responseStatus"],
"status": _build_ce_status(
sample_unit_ref,
survey_short_name,
ce["exerciseRef"],
ce["responseStatus"],
reporting_unit_permission,
),
}
for ce in collection_exercises
]
return table


def _build_ce_status(ru_ref: str, survey: str, period: str, response_status: str, ru_permission: bool) -> dict:
ce_status = {
"hyperlink": url_for(
"case_bp.get_response_statuses",
ru_ref=ru_ref,
survey=survey,
period=period,
),
"hyperlink_text": "Change" if ru_permission else "View",
"status_class": (
CE_STATUS_CLASS[response_status] if response_status in CE_STATUS_CLASS.keys() else "ons-status--error"
Copy link
Contributor

Choose a reason for hiding this comment

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

A get with default might be better here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to get with a default

),
"response_status": response_status,
}
return ce_status


def _build_respondents_section(
respondents: list,
case_id: str,
collection_exercise_id: str,
trading_as: str,
ru_name: str,
sample_unit_ref: str,
ru_id: str,
survey_ref: str,
survey_short_name: str,
survey_id: str,
unused_iac: str,
permissions: dict,
) -> list:
table = []
for respondent in respondents:
row = {}
if permissions["reporting_unit_edit"]:
if unused_iac:
Copy link
Contributor

Choose a reason for hiding this comment

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

I see it, the logic is a bit unusual as the frontend is also checking off the iac key being passed to it and diverting it around. We should try not to do that as it kind of invalidates any unit tests in that area as there is the same logic both in the frontend and back.

row["enrolment_code"] = unused_iac
Copy link
Contributor

Choose a reason for hiding this comment

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

Is enrolment_code not assigned if it has permissions and unused

Copy link
Contributor

Choose a reason for hiding this comment

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

This also looks like a walrus operator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah in the front end it checks the permissions again and doesn't try to display the enrolment code at all - also not sure how to put in a walrus operator here

else:
row["enrolment_code_hyperlink"] = url_for(
"reporting_unit_bp.generate_new_enrolment_code",
case_id=case_id,
collection_exercise_id=collection_exercise_id,
ru_name=ru_name,
ru_ref=sample_unit_ref,
trading_as=trading_as,
survey_ref=survey_ref,
survey_name=survey_short_name,
)
row["enrolment_code_hyperlink_text"] = "Generate new enrollment code"
else:
row["enrolment_code"] = ""
row["contact_details"] = {
"name": f"{respondent['firstName']} {respondent['lastName']}",
"email": respondent["emailAddress"],
"tel": respondent["telephone"],
}
row["account_status_class"] = (
"ons-status--error" if respondent["status"] == "SUSPENDED" else "ons-status--success"
)
row["account_status"] = respondent["status"].capitalize()
row["enrolment_status"] = respondent["enrolmentStatus"].capitalize()
(
row["enrolment_status_hyperlink"],
row["enrolment_status_hyperlink_text"],
row["enrolment_status_class"],
) = _build_enrollment_status_hyperlink(
respondent["enrolmentStatus"],
respondent["id"],
respondent["firstName"],
respondent["lastName"],
sample_unit_ref,
ru_name,
ru_id,
trading_as,
survey_id,
survey_short_name,
permissions["reporting_unit_edit"],
)
if permissions["messages_edit"]:
row["message"] = [
{"name": "ru_ref", "value": sample_unit_ref},
{"name": "business_id", "value": ru_id},
{"name": "business", "value": ru_name},
{"name": "survey", "value": survey_short_name},
{"name": "survey_id", "value": survey_id},
{"name": "msg_to_name", "value": row["contact_details"]["name"]},
{"name": "msg_to", "value": respondent["id"]},
]
table.append(row)

return table


def _build_enrollment_status_hyperlink(
enrolment_status: str,
respondent_id: str,
respondent_first_name: str,
respondent_last_name: str,
sample_unit_ref: str,
ru_name: str,
ru_id: str,
trading_as: str,
survey_id: str,
survey_short_name: str,
ru_permission: bool,
) -> tuple[str | None, str, str]:
if enrolment_status == ENROLMENT_ENABLED:
status_class = "ons-status--success"
hyperlink = (
url_for(
"reporting_unit_bp.confirm_change_enrolment_status",
ru_ref=sample_unit_ref,
ru_name=ru_name,
survey_id=survey_id,
survey_name=survey_short_name,
respondent_id=respondent_id,
respondent_first_name=respondent_first_name,
respondent_last_name=respondent_last_name,
business_id=ru_id,
trading_as=trading_as,
change_flag="DISABLED",
tab="reporting_units",
)
if ru_permission
else None
)
hyperlink_text = "Disable"
elif enrolment_status == ENROLMENT_PENDING:
status_class = "ons-status--info"
hyperlink = (
url_for("reporting_unit_bp.view_resend_verification", ru_ref=sample_unit_ref, party_id=respondent_id)
if ru_permission
else None
)
hyperlink_text = "Re-send verification email"
else:
status_class = "ons-status--dead"
hyperlink = (
url_for(
"reporting_unit_bp.confirm_change_enrolment_status",
ru_ref=sample_unit_ref,
ru_name=ru_name,
survey_id=survey_id,
survey_name=survey_short_name,
respondent_id=respondent_id,
respondent_first_name=respondent_first_name,
respondent_last_name=respondent_last_name,
business_id=ru_id,
trading_as=trading_as,
change_flag="ENABLED",
tab="reporting_units",
)
if ru_permission
else None
)
hyperlink_text = "Re-enable"

return hyperlink, hyperlink_text, status_class
Loading
Loading