darkhelm/notifications-admin
1
0
Fork 0
mirror of https://github.com/GSA/notifications-admin.git synced 2026-07-16 19:30:13 -04:00
Code Issues Packages Projects Releases Wiki Activity

code review feedback

Browse Source
This commit is contained in:
Kenneth Kehl
2024-05-31 09:41:36 -07:00
parent 2332744cdf fc29871b57
commit 2f366ff65c
41 changed files with 281 additions and 108 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
app/config.py
View File

@@ -38,7 +38,7 @@ class Config(object):
NR_MONITOR_ON = settings and settings.monitor_mode
COMMIT_HASH = getenv("COMMIT_HASH", "--------")[0:7]
GOVERNMENT_EMAIL_DOMAIN_NAMES = ["gov"]
GOVERNMENT_EMAIL_DOMAIN_NAMES = ["gov", "mil", "si.edu"]
# Logging
NOTIFY_LOG_LEVEL = getenv("NOTIFY_LOG_LEVEL", "INFO")

6
app/main/views/register.py
View File

@@ -26,6 +26,7 @@ from app.main.views import sign_in
from app.main.views.verify import activate_user
from app.models.user import InvitedOrgUser, InvitedUser, User
from app.utils import hide_from_search_engines, hilite
from app.utils.user import is_gov_user
@main.route("/register", methods=["GET", "POST"])
@@ -147,6 +148,11 @@ def check_invited_user_email_address_matches_expected(
flash("You cannot accept an invite for another person.")
abort(403)
if not is_gov_user(user_email):
debug_msg("invited user has a non-government email address.")
flash("You must use a government email address.")
abort(403)
@main.route("/set-up-your-profile", methods=["GET", "POST"])
@hide_from_search_engines

4
app/main/views/send.py
View File

@@ -948,7 +948,9 @@ def send_notification(service_id, template_id):
vals = ",".join(values)
data = f"{data}\r\n{vals}"
filename = f"one-off-{current_user.name}-{uuid.uuid4()}.csv"
filename = (
f"one-off-{uuid.uuid4()}.csv" # {current_user.name} removed from filename
)
my_data = {"filename": filename, "template_id": template_id, "data": data}
upload_id = s3upload(service_id, my_data)
form = CsvUploadForm()

18
app/main/views/sign_in.py
View File

@@ -4,7 +4,16 @@ import uuid
import jwt
import requests
from flask import Response, current_app, redirect, render_template, request, url_for
from flask import (
Response,
abort,
current_app,
flash,
redirect,
render_template,
request,
url_for,
)
from flask_login import current_user
from app import login_manager, user_api_client
@@ -15,6 +24,7 @@ from app.models.user import User
from app.utils import hide_from_search_engines
from app.utils.login import is_safe_redirect_url
from app.utils.time import is_less_than_days_ago
from app.utils.user import is_gov_user
from notifications_utils.url_safe_token import generate_token
@@ -88,6 +98,12 @@ def _do_login_dot_gov():
try:
access_token = _get_access_token(code, state)
user_email, user_uuid = _get_user_email_and_uuid(access_token)
if not is_gov_user(user_email):
current_app.logger.error(
"invited user has a non-government email address."
)
flash("You must use a government email address.")
abort(403)
redirect_url = request.args.get("next")
user = user_api_client.get_user_by_uuid_or_email(user_uuid, user_email)

2
app/main/views/sub_navigation_dictionaries.py
View File

@@ -32,7 +32,7 @@ def using_notify_nav():
"link": "main.trial_mode_new",
},
{
"name": "Pricing",
"name": "Tracking usage",
"link": "main.pricing",
},
{

2
app/s3_client/__init__.py
View File

@@ -40,7 +40,7 @@ def get_s3_object(
teststr = str(s3.Bucket(bucket_name).creation_date).lower()
if "magicmock" not in teststr:
raise Exception(
f"xxxxxtest not mocked, use @mock_aws creation date is {teststr}"
f"Test is not mocked, use @mock_aws or the relevant mocker.patch to avoid accessing S3"
)
return obj

2
app/templates/base.html
View File

@@ -47,7 +47,7 @@
{% endblock %}
{% block content %}
{% block flash_messages %}
{% include 'flash_messages.html' %}
{% include 'new/components/flash_messages.html' %}
{% endblock %}
{% block maincolumn_content %}
<div class="grid-row">

0
app/templates/flash_messages.html → app/templates/new/components/flash_messages.html
View File

0
app/templates/new/components/service_navigation.html → app/templates/new/components/service_nav.html
View File

0
app/templates/settings_nav.html → app/templates/new/components/settings_nav.html
View File

57
app/templates/new/layouts/withnav_template.html
View File

@@ -1,57 +0,0 @@
{% extends "/new/base.html" %}
{% block per_page_title %}
{% block service_page_title %}{% endblock %}{% if current_service.name %} – {{ current_service.name }}{% endif %}
{% block org_page_title %}{% endblock %}{% if current_org.name %} – {{ current_org.name }}{% endif %}
{% endblock %}
{% block main %}
<div class="grid-container">
{% block serviceNavigation %}
{% if current_org.name %}
{% else %}
{% include "new/components/service_navigation.html" %}
{% endif %}
{% endblock %}
{#
The withnav_template can serve as a replacement for both settings_template and org_template.html.
The file service_navigation.html is included only in withnav_template. It's not used in settings_template. That is one out of the two differences between settings template and withnav template. As a result, when other templates extend settings_template, they include the serviceNavigation block but keep it empty. The settings_template.html is specifically used for these pages in the app: manage-users.html, service-settings.html, and user-profile.html.
In addition, serviceNavigation should be empty on templates that previously extended org_template. For templates that previously extended org_template.html, there's an addition of the orgNavBreadcrumb block.
{% block orgNavBreadcrumb %}
{% include "/new/components/org_nav_breadcrumb.html" %}
{% endblock %}
#}
{% if current_org.name %}
{% block orgNavBreadcrumb %}{% include "/new/components/org_nav_breadcrumb.html" %}{% endblock %}
{% endif %}
<div class="grid-row margin-top-5">
<div class="tablet:grid-col-3">
{% block sideNavigation %}
{% if org_navigation_links %}
{% include "/new/components/org_nav.html" %}
{% else %}
{% include "/new/components/main_nav.html" %}
{% endif %}
{#
Include settings_nav.html for child templates that previously extended settings_template.
Include "org_nav.html" for child templates that previously extended org_template html
#}
{% endblock %}
</div>
<div class="tablet:grid-col-9 tablet:padding-left-4">
{% block beforeContent %}
{% block backLink %}{% endblock %}
{% endblock %}
<main id="main-content" role="main" class="usa-prose site-prose margin-bottom-10">
{% block content %}
{% include 'flash_messages.html' %}
{% block maincolumn_content %}{% endblock %}
{% endblock %}
</main>
</div>
</div>
</div>
{% endblock %}

1
app/templates/main_nav.html → app/templates/old/main_nav.html
View File

@@ -1,3 +1,4 @@
{# This template is an old version #}
{% if help %}
{% include 'partials/tour.html' %}
{% else %}

1
app/templates/org_nav.html → app/templates/old/org_nav.html
View File

@@ -1,3 +1,4 @@
{# This template is an old version #}
<nav class="navigation">
<ul>
<li><a class="usa-link{{ org_navigation.is_selected('dashboard') }}" href="{{ url_for('.organization_dashboard', org_id=current_org.id) }}">Usage</a></li>

0
app/templates/org_template.html → app/templates/old/org_template.html
View File

1
app/templates/service_navigation.html → app/templates/old/service_navigation.html
View File

@@ -1,3 +1,4 @@
{# This template is an old version #}
{% macro navigation_service_name(service) %}
<div class="font-body-2xl text-bold">
{{ service.name }}

1
app/templates/new/components/settings_navigation.html → app/templates/old/settings_nav.html
View File

@@ -1,3 +1,4 @@
{# This template is an old version #}
{% if help %}
{% include 'partials/tour.html' %}
{% else %}

3
app/templates/settings_template.html → app/templates/old/settings_template.html
View File

@@ -1,4 +1,5 @@
{% extends "base.html" %}
{# This template is an old version #}
{% extends "admin_template.html" %}
{% block per_page_title %}
{% block service_page_title %}{% endblock %} – {{ current_service.name }}

36
app/templates/old/withnav_template.html Normal file
View File

@@ -0,0 +1,36 @@
{# This template is an old version #}
{% extends "admin_template.html" %}
{% block per_page_title %}
{% block service_page_title %}{% endblock %} – {{ current_service.name }}
{% endblock %}
{% block main %}
<div class="grid-container">
{% include "service_navigation.html" %}
<div class="grid-row margin-top-5">
{% if help %}
<div class="tablet:grid-col-3">
{% else %}
<div class="tablet:grid-col-3">
{% endif %}
{% include "main_nav.html" %}
</div>
{% if help %}
<div class="grid-col-8">
{% else %}
<div class="tablet:grid-col-9 tablet:padding-left-4">
{% endif %}
{% block beforeContent %}
{% block backLink %}{% endblock %}
{% endblock %}
<main id="main-content" role="main" class="usa-prose site-prose margin-bottom-10">
{% block content %}
{% include 'flash_messages.html' %}
{% block maincolumn_content %}{% endblock %}
{% endblock %}
</main>
</div>
</div>
</div>
{% endblock %}

2
app/templates/views/dashboard/dashboard.html
View File

@@ -59,7 +59,7 @@
{% set notification = job.notifications[0] %}
<tr class="table-row" id="{{ job.job_id }}">
<td class="table-field file-name">
{{ notification.job.original_file_name if notification.job.original_file_name else 'Manually entered number'}}
{{ notification.job.original_file_name[:12] if notification.job.original_file_name else 'Manually entered number'}}
<br>
<a class="usa-link file-list-filename" href="{{ job.view_job_link }}">View Batch</a>
</td>

8
app/templates/views/manage-users.html
View File

@@ -1,4 +1,4 @@
{% extends "settings_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/tick-cross.html" import tick_cross %}
{% from "components/live-search.html" import live_search %}
{% from "components/components/button/macro.njk" import usaButton %}
@@ -7,6 +7,12 @@
Team members
{% endblock %}
{% block serviceNavigation %}{% endblock %}
{% block sideNavigation %}
{% include "/new/components/settings_nav.html" %}
{% endblock %}
{% block maincolumn_content %}
<div class="button-flex-header">

2
app/templates/views/organizations/organization/billing.html
View File

@@ -1,4 +1,4 @@
{% extends "org_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/page-header.html" import page_header %}
{% block org_page_title %}

2
app/templates/views/organizations/organization/index.html
View File

@@ -2,7 +2,7 @@
{% from "components/big-number.html" import big_number %}
{% from "components/live-search.html" import live_search %}
{% from "components/pill.html" import pill %}
{% extends "org_template.html" %}
{% extends "withnav_template.html" %}
{% block org_page_title %}
Usage

2
app/templates/views/organizations/organization/settings/edit-domains.html
View File

@@ -1,4 +1,4 @@
{% extends "org_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/page-footer.html" import page_footer %}
{% from "components/page-header.html" import page_header %}
{% from "components/list-entry.html" import list_entry %}

2
app/templates/views/organizations/organization/settings/edit-name.html
View File

@@ -1,4 +1,4 @@
{% extends "org_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/page-header.html" import page_header %}
{% from "components/page-footer.html" import page_footer %}
{% from "components/form.html" import form_wrapper %}

2
app/templates/views/organizations/organization/settings/edit-organization-billing-details.html
View File

@@ -1,4 +1,4 @@
{% extends "org_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/page-header.html" import page_header %}
{% from "components/page-footer.html" import page_footer %}
{% from "components/form.html" import form_wrapper %}

2
app/templates/views/organizations/organization/settings/edit-organization-notes.html
View File

@@ -1,4 +1,4 @@
{% extends "org_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/page-header.html" import page_header %}
{% from "components/page-footer.html" import page_footer %}
{% from "components/form.html" import form_wrapper %}

2
app/templates/views/organizations/organization/settings/edit-type.html
View File

@@ -1,4 +1,4 @@
{% extends "org_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/page-footer.html" import page_footer %}
{% from "components/page-header.html" import page_header %}
{% from "components/form.html" import form_wrapper %}

2
app/templates/views/organizations/organization/settings/index.html
View File

@@ -1,4 +1,4 @@
{% extends "org_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/table.html" import mapping_table, optional_text_field, row, text_field, edit_field with context %}
{% block org_page_title %}

2
app/templates/views/organizations/organization/trial-mode-services.html
View File

@@ -1,4 +1,4 @@
{% extends "org_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/live-search.html" import live_search %}
{% block org_page_title %}

2
app/templates/views/organizations/organization/users/index.html
View File

@@ -1,4 +1,4 @@
{% extends "org_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/table.html" import list_table, row, field, hidden_field_heading %}
{% from "components/page-footer.html" import page_footer %}
{% from "components/live-search.html" import live_search %}

2
app/templates/views/organizations/organization/users/invite-org-user.html
View File

@@ -1,4 +1,4 @@
{% extends "org_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/page-footer.html" import page_footer %}
{% from "components/page-header.html" import page_header %}

2
app/templates/views/platform-admin/_base_template.html
View File

@@ -45,7 +45,7 @@
{% block backLink %}{% endblock %}
<main id="main-content" role="main">
{% block content %}
{% include 'flash_messages.html' %}
{% include 'new/components/flash_messages.html' %}
{% block platform_admin_content %}{% endblock %}
{% endblock %}
</main>

19
app/templates/views/pricing/index.html
View File

@@ -15,7 +15,7 @@ Message parts
{{ content_metadata(
data={
"Last updated": "February 5, 2024"
"Last updated": "April 10, 2024"
}
) }}
@@ -26,13 +26,15 @@ more parts towards the allowance if you:</p>
<ul class="list list-bullet">
<li>send text messages longer than 160 characters</a></li>
<li>use certain <a class="usa-link" href="#symbols">signs and symbols</a></li>
<li>use <a class="usa-link" href="#accents">accents and accented letters</a></li>
<li>use <a class="usa-link" href="#accents">accents and accented letters, including non-romanized scripts</a></li>
</ul>
<h3 class="font-body-lg" id="long-text-messages">Long text messages</h3>
<p>If a text message is longer than 160 characters (including spaces and service name), it counts as more than one message
part.</p>
<h4>Calculation of message parts without special characters</h4>
<div class="bottom-gutter-3-2">
{% call mapping_table(
caption='Text message pricing',
@@ -72,12 +74,17 @@ and the number of parts you’ll have left.</p>
<p>Using them can increase the cost of sending text messages.</p>
<h3 class="font-body-lg" id="accents">Accents and accented characters</h3>
<p>Some languages use accented characters.</p>
<h3 class="font-body-lg" id="accents">Accented characters and non-romanized scripts</h3>
<p>Notify can handle a wide range of different languages and scripts. However, occasionally some phone carriers may
struggle to display special characters or non-romanized scripts. (Languages such as Arabic, Chinese, Japanese, Korean,
and Russian use non-romanized scripts with different characters.) Best practices encourage communication in the
recipient’s preferred language, but we are aware that, rarely, a phone carrier will not be able to handle the message.</p>
<p>The following accented characters do not affect the cost of sending text messages: Ä, É, Ö, Ü, à, ä, é, è, ì, ò, ö,
ù, ü.</p>
<p>Using other accented characters can increase the cost of sending text messages.
<p>
<p>Using other accented characters or scripts will increase the cost of sending text messages. Even one accented character
(with the exception of those noted above), or use of a non-romanized or logographic script will cause the entire message
to be calculated as detailed below.<p>
<h4>Calculation of message parts with special characters or non-romanized scripts</h4>
{% set accentedChars %}
<div class="bottom-gutter-3-2">
{% call mapping_table(

8
app/templates/views/service-settings.html
View File

@@ -1,4 +1,4 @@
{% extends "settings_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/banner.html" import banner_wrapper %}
{% from "components/table.html" import mapping_table, row, settings_row, text_field, optional_text_field, edit_field, field, boolean_field with context %}
@@ -6,6 +6,12 @@
Settings
{% endblock %}
{% block serviceNavigation %}{% endblock %}
{% block sideNavigation %}
{% include "/new/components/settings_nav.html" %}
{% endblock %}
{% block maincolumn_content %}
<h1 class="font-body-lg">Settings</h1>

8
app/templates/views/user-profile.html
View File

@@ -1,4 +1,4 @@
{% extends "settings_template.html" %}
{% extends "withnav_template.html" %}
{% from "components/table.html" import list_table, row, field %}
{% from "components/table.html" import mapping_table, row, text_field, optional_text_field, edit_field, field, boolean_field with context %}
@@ -6,6 +6,12 @@
User profile
{% endblock %}
{% block serviceNavigation %}{% endblock %}
{% block sideNavigation %}
{% include "/new/components/settings_nav.html" %}
{% endblock %}
{% block maincolumn_content %}
<h1 class="font-body-2xl margin-bottom-3">User profile</h1>

55
app/templates/withnav_template.html
View File

@@ -1,31 +1,56 @@
{% extends "base.html" %}
{% block per_page_title %}
{% block service_page_title %}{% endblock %} – {{ current_service.name }}
{% if current_org.name %}
{% block org_page_title %}{% endblock %} – {{ current_org.name }}
{% else %}
{% block service_page_title %}{% endblock %} – {{ current_service.name }}
{% endif %}
{% endblock %}
{% block main %}
<div class="grid-container">
{% include "service_navigation.html" %}
{% block serviceNavigation %}
{% if current_org.name %}
{% else %}
{% include "new/components/service_nav.html" %}
{% endif %}
{% endblock %}
{#
The withnav_template can serve as a replacement for both settings_template and org_template.html.
The file service_navigation.html is included only in withnav_template. It's not used in settings_template. That is one out of the two differences between settings template and withnav template. As a result, when other templates extend settings_template, they include the serviceNavigation block but keep it empty. The settings_template.html is specifically used for these pages in the app: manage-users.html, service-settings.html, and user-profile.html.
In addition, serviceNavigation should be empty on templates that previously extended org_template. For templates that previously extended org_template.html, there's an addition of the orgNavBreadcrumb block.
{% block orgNavBreadcrumb %}
{% include "/new/components/org_nav_breadcrumb.html" %}
{% endblock %}
#}
{% if current_org.name %}
{% block orgNavBreadcrumb %}{% include "/new/components/org_nav_breadcrumb.html" %}{% endblock %}
{% endif %}
<div class="grid-row margin-top-5">
{% if help %}
<div class="tablet:grid-col-3">
{% else %}
<div class="tablet:grid-col-3">
{% endif %}
{% include "main_nav.html" %}
</div>
{% if help %}
<div class="grid-col-8">
{% else %}
<div class="tablet:grid-col-9 tablet:padding-left-4">
{% endif %}
<div class="tablet:grid-col-3">
{% block sideNavigation %}
{% if current_org.name %}
{% include "/new/components/org_nav.html" %}
{% else %}
{% include "/new/components/main_nav.html" %}
{% endif %}
{#
Include settings_nav.html for child templates that previously extended settings_template.
Include "org_nav.html" for child templates that previously extended org_template html
#}
{% endblock %}
</div>
<div class="tablet:grid-col-9 tablet:padding-left-4">
{% block beforeContent %}
{% block backLink %}{% endblock %}
{% endblock %}
<main id="main-content" role="main" class="usa-prose site-prose margin-bottom-10">
{% block content %}
{% include 'flash_messages.html' %}
{% include 'new/components/flash_messages.html' %}
{% block maincolumn_content %}{% endblock %}
{% endblock %}
</main>

3
app/utils/user.py
View File

@@ -4,7 +4,6 @@ from flask import abort, current_app
from flask_login import current_user, login_required
from app import config
from app.notify_client.organizations_api_client import organizations_client
user_is_logged_in = login_required
@@ -51,7 +50,7 @@ def user_is_platform_admin(f):
def is_gov_user(email_address):
return _email_address_ends_with(
email_address, config.Config.GOVERNMENT_EMAIL_DOMAIN_NAMES
) or _email_address_ends_with(email_address, organizations_client.get_domains())
) # or _email_address_ends_with(email_address, organizations_client.get_domains())
def _email_address_ends_with(email_address, known_domains):