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

initial #2270

Closed
wants to merge 8 commits into from
Closed

initial #2270

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
8 changes: 4 additions & 4 deletions .ds.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@
"filename": ".github/workflows/checks.yml",
"hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8",
"is_verified": false,
"line_number": 68,
"line_number": 71,
"is_secret": false
}
],
Expand Down Expand Up @@ -555,15 +555,15 @@
"filename": "tests/app/main/views/test_register.py",
"hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8",
"is_verified": false,
"line_number": 200,
"line_number": 199,
"is_secret": false
},
{
"type": "Secret Keyword",
"filename": "tests/app/main/views/test_register.py",
"hashed_secret": "bb5b7caa27d005d38039e3797c3ddb9bcd22c3c8",
"is_verified": false,
"line_number": 273,
"line_number": 272,
"is_secret": false
}
],
Expand Down Expand Up @@ -684,5 +684,5 @@
}
]
},
"generated_at": "2025-01-13T20:16:58Z"
"generated_at": "2025-01-16T21:38:03Z"
}
7 changes: 5 additions & 2 deletions .github/workflows/checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,13 @@ jobs:
output: report-markdown
annotations: failed-tests
prnumber: ${{ steps.findPr.outputs.number }}
- name: Check imports alphabetized
run: poetry run isort ./app ./tests
- name: Check pep8
run: poetry run black ./app ./tests
- name: Run style checks
run: poetry run flake8 .
- name: Check imports alphabetized
run: poetry run isort --check-only ./app ./tests

- name: Check dead code
run: make dead-code
- name: Run js tests
Expand Down
12 changes: 12 additions & 0 deletions app/main/validators.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import csv
from io import StringIO
import re
from abc import ABC, abstractmethod

Expand Down Expand Up @@ -28,6 +30,16 @@ def __init__(self, message="Not a csv file"):
self.message = message

def __call__(self, form, field):

data = Spreadsheet.from_file_form(form).as_dict
data = data["data"]
if data is not None and data != "":
csv_file = StringIO(data)
reader = csv.reader(csv_file)
first_line = next(reader, None)
if first_line is None or all(cell.strip() == "" for cell in first_line):
raise ValidationError(f"No headers on row 1 in {field.data.filename}")

if not Spreadsheet.can_handle(field.data.filename):
raise ValidationError(
"{} is not a spreadsheet that Notify can read".format(
Expand Down
8 changes: 1 addition & 7 deletions app/main/views/index.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,7 @@ def check_feature_flags():
@main.route("/test/feature-flags")
def test_feature_flags():
return jsonify(
{
"FEATURE_ABOUT_PAGE_ENABLED": current_app.config[
"FEATURE_ABOUT_PAGE_ENABLED"
]
}
{"FEATURE_ABOUT_PAGE_ENABLED": current_app.config["FEATURE_ABOUT_PAGE_ENABLED"]}
)


Expand Down Expand Up @@ -235,7 +231,6 @@ def contact():
return render_template(
"views/contact.html",
navigation_links=about_notify_nav(),

)


Expand Down Expand Up @@ -268,7 +263,6 @@ def join_notify():
return render_template(
"views/join-notify.html",
navigation_links=about_notify_nav(),

)


Expand Down
1 change: 1 addition & 0 deletions app/main/views/send.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ def send_messages(service_id, template_id):
form = CsvUploadForm()
if form.validate_on_submit():
try:

upload_id = s3upload(
service_id,
Spreadsheet.from_file_form(form).as_dict,
Expand Down
3 changes: 2 additions & 1 deletion app/models/spreadsheet.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ def get_extension(filename):

@staticmethod
def normalise_newlines(file_content):
return "\r\n".join(file_content.read().decode("utf-8").splitlines())
rows = file_content.read().decode("utf-8").splitlines()
return "\r\n".join(rows)

@classmethod
def from_rows(cls, rows, filename=""):
Expand Down
4 changes: 1 addition & 3 deletions tests/app/main/views/test_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,7 @@ def test_static_pages(client_request, mock_get_organization_by_domain, view, moc
session["user_id"] = None
request(
_expected_status=302,
_expected_redirect="/sign-in?next={}".format(
url_for("main.{}".format(view))
),
_expected_redirect="/sign-in?next={}".format(url_for("main.{}".format(view))),
)


Expand Down
5 changes: 2 additions & 3 deletions tests/app/main/views/test_register.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,8 @@ def test_should_return_200_when_email_is_not_gov_uk(
_expected_status=200,
)

assert (
"Enter a public sector email address."
in normalize_spaces(page.select_one(".usa-error-message").text)
assert "Enter a public sector email address." in normalize_spaces(
page.select_one(".usa-error-message").text
)


Expand Down
61 changes: 61 additions & 0 deletions tests/app/main/views/test_send.py
Original file line number Diff line number Diff line change
Expand Up @@ -898,6 +898,67 @@ def test_upload_csv_invalid_extension(
assert "invalid.txt is not a spreadsheet that Notify can read" in page.text


@pytest.mark.parametrize(
("file_contents", "expected_error"),
[
(
"""

phone number, name
+12028675109, example
+12028675109,
+12028675109, example
""",
(
"There’s a problem with example.csv "
"You need to put the column headers in row 1."
),
),
],
)
def test_upload_csv_file_with_extra_blank_lines_is_flagged(
client_request,
mocker,
mock_get_service_template_with_placeholders,
mock_get_users_by_service,
mock_get_service_statistics,
mock_get_job_doesnt_exist,
mock_get_jobs,
service_one,
fake_uuid,
file_contents,
expected_error,
):

mocker.patch("app.main.views.send.set_metadata_on_csv_upload")

mocker.patch(
"app.main.views.send.get_csv_metadata",
return_value={"original_file_name": "example.csv"},
)
mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid())

mocker.patch("app.main.views.send.s3download", return_value=file_contents)

page = client_request.post(
"main.send_messages",
service_id=service_one["id"],
template_id=fake_uuid,
_data={"file": (BytesIO("".encode("utf-8")), "example.csv")},
_follow_redirects=True,
)

with client_request.session_transaction() as session:
assert "file_uploads" not in session

assert page.select_one("input[type=file]").has_attr("accept")
assert (
page.select_one("input[type=file]")["accept"]
== ".csv,.xlsx,.xls,.ods,.xlsm,.tsv"
)
assert normalize_spaces(page.select(".banner-dangerous")[0].text) == expected_error


def test_upload_csv_size_too_big(
client_request,
mock_login,
Expand Down
Loading