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

Let user choose which version of Notify docs they want to view #5235

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
45 changes: 45 additions & 0 deletions app/main/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from functools import partial
from itertools import chain
from numbers import Number
from urllib.parse import urlparse

import pytz
from flask import request
Expand Down Expand Up @@ -2280,6 +2281,50 @@ def validate(self, *args, **kwargs):
return super().validate(*args, **kwargs) or self.url.data == ""


class UrlForm(StripWhitespaceForm):
url = GovukTextInputField(
"URL",
validators=[
DataRequired(message="Cannot be empty"),
Regexp(regex="^https.*", message="Must be a valid https URL"),
],
)

def validate(self, *args, **kwargs):
self.url.validators.append(self.check_url)
return super().validate(*args, **kwargs)

def check_url(self, *args, **kwargs):
parsed_url = urlparse(self.url.data)

if parsed_url.hostname == "docs.notifications.service.gov.uk" and parsed_url.fragment:
return parsed_url
else:
raise ValidationError("Must be a valid https URL, pointing to a section within the GOV.UK Notify API docs.")
Copy link
Contributor

Choose a reason for hiding this comment

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

feel free to ignore

Suggested change
raise ValidationError("Must be a valid https URL, pointing to a section within the GOV.UK Notify API docs.")
raise ValidationError("The URL must point to a specific section in the GOV.UK Notify API docs, and start with 'https://docs.notifications.service.gov.uk'.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice one, I will add this to code (and tests)

Copy link
Contributor

@karlchillmaid karlchillmaid Oct 16, 2024

Choose a reason for hiding this comment

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

We can probably be a little more consistent with our existing validation errors.

Suggested change
raise ValidationError("Must be a valid https URL, pointing to a section within the GOV.UK Notify API docs.")
raise ValidationError("Enter a URL in the correct format, beginning https://docs.notifications.service.gov.uk")
Suggested change
raise ValidationError("Must be a valid https URL, pointing to a section within the GOV.UK Notify API docs.")
raise ValidationError("Enter a URL beginning https://docs.notifications.service.gov.uk")



class ChooseDocsForm(StripWhitespaceForm):

def __init__(self, section_tag):
super().__init__(section_tag=section_tag)

docs_version = GovukRadiosField(
"Which version of the docs would you like to view?",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Which version of the docs would you like to view?",
"Choose a client library",

choices=[
("python", "Python"),
("ruby", "Ruby"),
("java", "Java"),
("node", "Node JS"),
("net", ".Net"),
("php", "PHP"),
("rest-api", "Rest API"),
("rest-api", "Any is fine"),
joybytes marked this conversation as resolved.
Show resolved Hide resolved
],
thing="a language version of GOV.UK Notify's API docs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Apart from the REST API documentation, we refer to these as ‘client libraries’ elsewhere.

)
section_tag = HiddenField("section tag")


class SMSPrefixForm(StripWhitespaceForm):
enabled = OnOffField("") # label is assigned on instantiation

Expand Down
35 changes: 34 additions & 1 deletion app/main/views/index.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from app import status_api_client
from app.formatters import message_count
from app.main import main
from app.main.forms import FieldWithNoneOption
from app.main.forms import ChooseDocsForm, FieldWithNoneOption, UrlForm
from app.main.views.sub_navigation_dictionaries import features_nav, using_notify_nav
from app.models.branding import EmailBranding
from app.models.letter_rates import LetterRates
Expand Down Expand Up @@ -159,6 +159,39 @@ def guidance_api_documentation():
)


@main.route("/using-notify/api-documentation/section", methods=["GET", "POST"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this either be platform admin only or have the @hide_from_search_engines decorator? There's nothing sensitive on the page, but it might be confusing to people if they come across it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea to hide it from search engines 🙌🏼

def guidance_api_documentation_section():

form = UrlForm()

if form.validate_on_submit():
section_tag = form.url.data.split("#")[-1]
joybytes marked this conversation as resolved.
Show resolved Hide resolved
return redirect(url_for(".guidance_api_documentation_section_choose_docs", section_tag=section_tag))

return render_template(
"views/guidance/using-notify/api-documentation-section.html",
navigation_links=using_notify_nav(),
form=form,
)


@main.route("/using-notify/api-documentation/section/choose-docs", methods=["GET", "POST"])
def guidance_api_documentation_section_choose_docs():
form = ChooseDocsForm(section_tag=request.args.get("section_tag"))

if form.validate_on_submit():
redirect_url = (
f"https://docs.notifications.service.gov.uk/{form.docs_version.data}.html#{form.section_tag.data}"
)
return redirect(redirect_url)

return render_template(
"views/guidance/using-notify/api-documentation-section-choose-docs.html",
navigation_links=using_notify_nav(),
form=form,
)


@main.route("/using-notify/attach-pages")
def guidance_attach_pages():
return render_template(
Expand Down
2 changes: 2 additions & 0 deletions app/navigation.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ class HeaderNavigation(Navigation):
"using-notify": {
"guidance_using_notify",
"guidance_api_documentation",
"guidance_api_documentation_section",
"guidance_api_documentation_section_choose_docs",
"guidance_attach_pages",
"guidance_bulk_sending",
"guidance_data_retention_period",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{% extends "content_template.html" %}
{% from "components/form.html" import form_wrapper %}
{% from "components/page-footer.html" import page_footer %}

{# Used by the content_template.html layout, prefixes the "navigation" accessible name #}
{% set navigation_label_prefix = 'Using Notify' %}
{% set page_title = "You have been sent a link to GOV.UK Notify API documentation" %}
Copy link
Contributor

@karlchillmaid karlchillmaid Oct 16, 2024

Choose a reason for hiding this comment

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

Suggested change
{% set page_title = "You have been sent a link to GOV.UK Notify API documentation" %}
{% set page_title = "You have been sent a link to the GOV.UK Notify API documentation" %}



{% block per_page_title %}
{{page_title}}
{% endblock %}

{% block content_column_content %}

<h1 class="heading-large">{{page_title}}</h1>

{% call form_wrapper() %}
{{ form.docs_version }}
{{ page_footer("Let's go!") }}
{% endcall %}

{% endblock %}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{% extends "content_template.html" %}
{% from "components/form.html" import form_wrapper %}
{% from "components/page-footer.html" import page_footer %}

{# Used by the content_template.html layout, prefixes the "navigation" accessible name #}
{% set navigation_label_prefix = 'Using Notify' %}
{% set page_title = "Send a link to an API docs section" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{% set page_title = "Send a link to an API docs section" %}
{% set page_title = "Send a link to the Notify API documentation" %}

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is for platform admins, do we need to specify ‘Notify’?

Can we just say the API documentation instead?



{% block per_page_title %}
{{page_title}}
{% endblock %}

{% block content_column_content %}

<h1 class="heading-large">{{page_title}}</h1>
<p class="govuk-body">Below, copy paste the full URL for docs section you would like to send to a service user, and click "Submit".</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p class="govuk-body">Below, copy paste the full URL for docs section you would like to send to a service user, and click "Submit".</p>
<p class="govuk-body">Enter the full URL for a section of the API documentation and select <b class="govuk-!-font-weight-bold">Submit</b>.</p>


<p class="govuk-body">You will be taken to
a landing page for that section. Send link to that page to your user, and they will be able to choose which language version they want to see.</p>
Copy link
Contributor

@karlchillmaid karlchillmaid Oct 16, 2024

Choose a reason for hiding this comment

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

Suggested change
a landing page for that section. Send link to that page to your user, and they will be able to choose which language version they want to see.</p>
a landing page for that section. Send the URL of that page to the user. When they open it in their browser, they’ll be able to choose a client library.</p>


{% call form_wrapper() %}
{{ form.url }}
{{ page_footer('Submit') }}
{% endcall %}

{% endblock %}
14 changes: 12 additions & 2 deletions app/templates/views/guidance/using-notify/api-documentation.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,22 @@
{% set navigation_label_prefix = 'Using Notify' %}

{% block per_page_title %}
Documentation
API documentation
{% endblock %}

{% block content_column_content %}

<h1 class="heading-large">Documentation</h1>
<h1 class="heading-large">API documentation</h1>

{% if current_user.platform_admin %}
<a href="{{ url_for(".guidance_api_documentation_section") }}"
class="govuk-link govuk-link--no-visited-state"
target="_blank" rel="noopener">
Send a link to docs section
</a>
Comment on lines +17 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
target="_blank" rel="noopener">
Send a link to docs section
</a>
target="_blank" rel="noopener">
Send a link to the Notify API documentation
</a>

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is for platform admins, do we need to specify ‘Notify’?

Can we just say the API documentation instead?

<p></p>
{% endif %}

<p class="govuk-body">This documentation is for developers who want to integrate the GOV.UK Notify API with a web application or back office system.</p>
<h2 class="heading-medium" id="client-libraries">Client libraries</h2>
<p class="govuk-body">Links to documentation open in a new tab.</p>
Expand Down
83 changes: 83 additions & 0 deletions tests/app/main/views/test_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -384,3 +384,86 @@ def test_trial_mode_sending_limits(client_request):
"send 50 text messages per day",
"create letter templates, but not send them",
]


def test_guidance_api_documentation_links_to_section_flow_for_platform_admins(client_request, platform_admin_user):
client_request.login(platform_admin_user)

page = client_request.get("main.guidance_api_documentation")

assert len(page.select('a[href^="{link}"]'.format(link=url_for(".guidance_api_documentation_section")))) == 1


def test_guidance_api_documentation_does_not_link_to_section_flow_for_non_platform_admins(client_request):
page = client_request.get("main.guidance_api_documentation")

assert len(page.select('a[href^="{link}"]'.format(link=url_for(".guidance_api_documentation_section")))) == 0


def test_GET_guidance_api_documentation_section(client_request):
page = client_request.get("main.guidance_api_documentation_section")

assert page.select_one("h1").text == "Send a link to an API docs section"
assert page.select_one("input", attrs={"type": "text"})["name"] == "url"


def test_POST_guidance_api_documentation_section(client_request):
client_request.post(
"main.guidance_api_documentation_section",
_data={"url": "https://docs.notifications.service.gov.uk/python.html#send-a-file-by-email"},
_expected_redirect=url_for(
"main.guidance_api_documentation_section_choose_docs",
section_tag="send-a-file-by-email",
),
)


@pytest.mark.parametrize(
"url, expected_error_message",
[
["", "Cannot be empty"], # empty string
[
"https://docs.notifications.service.gov.uk/python.html",
"Must be a valid https URL, pointing to a section within the GOV.UK Notify API docs.",
], # no section
[
"https://docs.payments.service.gov.uk/making_payments/#creating-a-payment",
"Must be a valid https URL, pointing to a section within the GOV.UK Notify API docs.",
], # URL is notfor Notify's docs
[
"http://docs.notifications.service.gov.uk/python.html#send-a-file-by-email",
"Must be a valid https URL",
], # http instead of https
],
)
def test_POST_guidance_api_documentation_section_with_incorrect_url(client_request, url, expected_error_message):
page = client_request.post(
"main.guidance_api_documentation_section",
_data={"url": url},
_expected_status=200,
)

assert expected_error_message in page.select_one(".govuk-error-message").text


def test_GET_guidance_api_documentation_section_choose_docs(client_request):
page = client_request.get("main.guidance_api_documentation_section_choose_docs", section_tag="send-a-file-by-email")

assert page.select_one("h1").text == "You have been sent a link to GOV.UK Notify API documentation"

assert ["python", "ruby", "java", "node", "net", "php", "rest-api", "rest-api"] == [
radio["value"] for radio in page.select("input[type=radio]")
]
form = page.select_one("form")
assert form["action"] == url_for(
"main.guidance_api_documentation_section_choose_docs",
section_tag="send-a-file-by-email",
)


def test_POST_guidance_api_documentation_section_choose_docs(client_request):
client_request.post(
"main.guidance_api_documentation_section_choose_docs",
_data={"docs_version": "python", "section_tag": "send-a-file-by-email"},
_expected_redirect="https://docs.notifications.service.gov.uk/python.html#send-a-file-by-email",
)
2 changes: 2 additions & 0 deletions tests/app/test_navigation.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@
"go_to_dashboard_after_tour",
"guest_list",
"guidance_api_documentation",
"guidance_api_documentation_section",
"guidance_api_documentation_section_choose_docs",
"guidance_attach_pages",
"guidance_billing_details",
"guidance_bulk_sending",
Expand Down
2 changes: 2 additions & 0 deletions tests/route-list.json
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,8 @@
"/users/<uuid:user_id>/change_auth",
"/using-notify",
"/using-notify/api-documentation",
"/using-notify/api-documentation/section",
"/using-notify/api-documentation/section/choose-docs",
"/using-notify/attach-pages",
"/using-notify/bulk-sending",
"/using-notify/data-retention-period",
Expand Down