Make row in URL match displayed row

Spreadsheets start at row 1 (the header row), and the values don’t start
until row 2. The row numbers in our URLs start at 0, which is a concept
that only makes sense to programmers.

It’s more predictable and consistent to make the number in the URL match
the row number displayed on the page when previewing the spreadsheet.
This commit is contained in:
Chris Hill-Scott
2018-01-15 14:45:38 +00:00
parent b95a7403b4
commit 072f8e0f87
3 changed files with 20 additions and 14 deletions
+8 -5
View File
@@ -523,9 +523,12 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a
count_of_recipients = len(list(recipients.rows)) count_of_recipients = len(list(recipients.rows))
if preview_row < count_of_recipients: if preview_row < 2:
template.values = recipients[preview_row] abort(404)
elif preview_row > 0:
if preview_row < count_of_recipients + 2:
template.values = recipients[preview_row - 2]
elif preview_row > 2:
abort(404) abort(404)
session['upload_data']['notification_count'] = count_of_recipients session['upload_data']['notification_count'] = count_of_recipients
@@ -562,7 +565,7 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a
@main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>", methods=['GET']) @main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>", methods=['GET'])
@login_required @login_required
@user_has_permissions('send_texts', 'send_emails', 'send_letters') @user_has_permissions('send_texts', 'send_emails', 'send_letters')
def check_messages(service_id, template_type, upload_id, row_index=0): def check_messages(service_id, template_type, upload_id, row_index=2):
data = _check_messages(service_id, template_type, upload_id, row_index) data = _check_messages(service_id, template_type, upload_id, row_index)
@@ -590,7 +593,7 @@ def check_messages(service_id, template_type, upload_id, row_index=0):
@main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>.<filetype>", methods=['GET']) @main.route("/services/<service_id>/<template_type>/check/<upload_id>/row-<int:row_index>.<filetype>", methods=['GET'])
@login_required @login_required
@user_has_permissions('send_texts', 'send_emails', 'send_letters') @user_has_permissions('send_texts', 'send_emails', 'send_letters')
def check_messages_preview(service_id, template_type, upload_id, filetype, row_index=0): def check_messages_preview(service_id, template_type, upload_id, filetype, row_index=2):
if filetype not in ('pdf', 'png'): if filetype not in ('pdf', 'png'):
abort(404) abort(404)
+2 -2
View File
@@ -60,10 +60,10 @@
) %} ) %}
{% call index_field() %} {% call index_field() %}
<span> <span>
{% if item.index == preview_row %} {% if (item.index + 2) == preview_row %}
{{ item.index + 2 }} {{ item.index + 2 }}
{% else %} {% else %}
<a href="{{ url_for('.check_messages', service_id=current_service.id, template_type=template.template_type, upload_id=upload_id, row_index=(item.index)) }}">{{ item.index + 2 }}</a> <a href="{{ url_for('.check_messages', service_id=current_service.id, template_type=template.template_type, upload_id=upload_id, row_index=(item.index + 2)) }}">{{ item.index + 2 }}</a>
{% endif %} {% endif %}
</span> </span>
{% endcall %} {% endcall %}
+10 -7
View File
@@ -472,13 +472,13 @@ def test_upload_valid_csv_redirects_to_check_page(
'Test Service: A, Template <em>content</em> with & entity', 'Test Service: A, Template <em>content</em> with & entity',
), ),
( (
{'row_index': 0}, {'row_index': 2},
None, None,
'To: 07700900001', 'To: 07700900001',
'Test Service: A, Template <em>content</em> with & entity', 'Test Service: A, Template <em>content</em> with & entity',
), ),
( (
{'row_index': 2}, {'row_index': 4},
True, True,
'To: 07700900003', 'To: 07700900003',
'Test Service: C, Template <em>content</em> with & entity', 'Test Service: C, Template <em>content</em> with & entity',
@@ -524,7 +524,7 @@ def test_upload_valid_csv_shows_preview_and_table(
if expected_link_in_first_row: if expected_link_in_first_row:
assert page.select_one('.table-field-index a')['href'] == url_for( assert page.select_one('.table-field-index a')['href'] == url_for(
'main.check_messages', service_id=SERVICE_ONE_ID, template_type='sms', upload_id=fake_uuid, row_index=0 'main.check_messages', service_id=SERVICE_ONE_ID, template_type='sms', upload_id=fake_uuid, row_index=2
) )
else: else:
assert not page.select_one('.table-field-index').select_one('a') assert not page.select_one('.table-field-index').select_one('a')
@@ -546,9 +546,12 @@ def test_upload_valid_csv_shows_preview_and_table(
@pytest.mark.parametrize('row_index, expected_status', [ @pytest.mark.parametrize('row_index, expected_status', [
(0, 200), (0, 404),
(1, 404),
(2, 200), (2, 200),
(3, 404), (3, 200),
(4, 200),
(5, 404),
]) ])
def test_404_for_previewing_a_row_out_of_range( def test_404_for_previewing_a_row_out_of_range(
client_request, client_request,
@@ -1480,11 +1483,11 @@ def test_can_start_letters_job(
{'postcode': 'abc123', 'addressline1': '123 street'}, {'postcode': 'abc123', 'addressline1': '123 street'},
), ),
( (
{'row_index': 0}, {'row_index': 2},
{'postcode': 'abc123', 'addressline1': '123 street'}, {'postcode': 'abc123', 'addressline1': '123 street'},
), ),
( (
{'row_index': 1}, {'row_index': 3},
{'postcode': 'cba321', 'addressline1': '321 avenue'}, {'postcode': 'cba321', 'addressline1': '321 avenue'},
), ),
]) ])