From fea85cb07befef01a8293d233abbc11b6355bd65 Mon Sep 17 00:00:00 2001 From: dandds Date: Thu, 8 Nov 2018 16:23:08 -0500 Subject: [PATCH 01/16] utility function for caching form data --- atst/utils/form_cache.py | 18 ++++++++++++++++++ tests/utils/test_form_cache.py | 15 +++++++++++++++ 2 files changed, 33 insertions(+) create mode 100644 atst/utils/form_cache.py create mode 100644 tests/utils/test_form_cache.py diff --git a/atst/utils/form_cache.py b/atst/utils/form_cache.py new file mode 100644 index 00000000..e8e249f5 --- /dev/null +++ b/atst/utils/form_cache.py @@ -0,0 +1,18 @@ +import os +import hashlib +import pickle + + +DEFAULT_CACHE_NAME = "formcache" + + +def cache_form_data(redis, formdata, expiry_seconds=3600, key_prefix=DEFAULT_CACHE_NAME): + value = pickle.dumps(formdata) + key = hashlib.sha1(os.urandom(64)).hexdigest() + redis.setex(name="{}:{}".format(key_prefix, key), value=value, time=expiry_seconds) + return key + + +def retrieve_form_data(redis, formdata_key, key_prefix="formcache"): + data = redis.get("{}:{}".format(key_prefix, formdata_key)) + return pickle.loads(data) diff --git a/tests/utils/test_form_cache.py b/tests/utils/test_form_cache.py new file mode 100644 index 00000000..7eaabeee --- /dev/null +++ b/tests/utils/test_form_cache.py @@ -0,0 +1,15 @@ +from werkzeug.datastructures import ImmutableDict + +from atst.utils.form_cache import DEFAULT_CACHE_NAME, cache_form_data, retrieve_form_data + +def test_cache_form_data(app): + data = ImmutableDict({"kessel_run": "12 parsecs"}) + key = cache_form_data(app.redis, data) + assert app.redis.get("{}:{}".format(DEFAULT_CACHE_NAME, key)) + + +def test_retrieve_form_data(app): + data = ImmutableDict({"class": "corellian"}) + key = cache_form_data(app.redis, data) + retrieved = retrieve_form_data(app.redis, key) + assert retrieved == data From 3fe20eda62ba6ebdc8f71883335c8cc00cf42d45 Mon Sep 17 00:00:00 2001 From: dandds Date: Thu, 8 Nov 2018 17:07:53 -0500 Subject: [PATCH 02/16] WIP basic demo implementation --- atst/routes/__init__.py | 5 ++++- atst/routes/errors.py | 6 +++++- atst/routes/requests/financial_verification.py | 11 +++++++++-- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/atst/routes/__init__.py b/atst/routes/__init__.py index 780ff0c3..0ea12952 100644 --- a/atst/routes/__init__.py +++ b/atst/routes/__init__.py @@ -96,7 +96,10 @@ def _make_authentication_context(): def redirect_after_login_url(): if request.args.get("next"): - return request.args.get("next") + returl = request.args.get("next") + if request.args.get("formCache"): + returl += "?" + url.urlencode({"formCache": request.args.get("formCache")}) + return returl else: return url_for("atst.home") diff --git a/atst/routes/errors.py b/atst/routes/errors.py index 7cf58b46..56551ee0 100644 --- a/atst/routes/errors.py +++ b/atst/routes/errors.py @@ -8,6 +8,7 @@ from atst.domain.invitations import ( ExpiredError as InvitationExpiredError, WrongUserError as InvitationWrongUserError, ) +from atst.utils.form_cache import cache_form_data def log_error(e): @@ -37,7 +38,10 @@ def make_error_pages(app): # pylint: disable=unused-variable def session_expired(e): log_error(e) - return redirect(url_for("atst.root", sessionExpired=True, next=request.path)) + url_args = {"sessionExpired": True, "next": request.path} + if request.method == "POST": + url_args["formCache"] = cache_form_data(app.redis, request.form) + return redirect(url_for("atst.root", **url_args)) @app.errorhandler(Exception) # pylint: disable=unused-variable diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 320db9c6..58940be0 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -1,4 +1,4 @@ -from flask import g, render_template, redirect, url_for +from flask import g, render_template, redirect, url_for, current_app as app from flask import request as http_request from werkzeug.datastructures import ImmutableMultiDict, FileStorage @@ -13,6 +13,7 @@ from atst.domain.requests.financial_verification import ( ) from atst.models.attachment import Attachment from atst.domain.task_orders import TaskOrders +from atst.utils.form_cache import retrieve_form_data def fv_extended(_http_request): @@ -91,6 +92,12 @@ class FinancialVerificationBase(object): raise FormValidationError(form) +def existing_form_data(): + key = http_request.args.get("formCache") + if key: + return retrieve_form_data(app.redis, key) + + class GetFinancialVerificationForm(FinancialVerificationBase): def __init__(self, user, request, is_extended=False): self.user = user @@ -98,7 +105,7 @@ class GetFinancialVerificationForm(FinancialVerificationBase): self.is_extended = is_extended def execute(self): - form = self._get_form(self.request, self.is_extended) + form = self._get_form(self.request, self.is_extended, formdata=existing_form_data()) return form From e06a28c9187c48c9ad000a985adfb4d674f9355d Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 15 Nov 2018 14:23:06 -0500 Subject: [PATCH 03/16] Add GET draft route so form state redirect works --- atst/routes/requests/financial_verification.py | 1 + 1 file changed, 1 insertion(+) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 58940be0..9f939ea0 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -185,6 +185,7 @@ class SaveFinancialVerificationDraft(FinancialVerificationBase): return updated_request +@requests_bp.route("/requests/verify//draft", methods=["GET"]) @requests_bp.route("/requests/verify/", methods=["GET"]) def financial_verification(request_id): request = Requests.get(g.current_user, request_id) From a475cb31560cdac01e9f46345b7205af372aed77 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 15 Nov 2018 16:03:29 -0500 Subject: [PATCH 04/16] FormCache class --- atst/app.py | 3 +++ atst/utils/form_cache.py | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/atst/app.py b/atst/app.py index 591e5c65..b309806a 100644 --- a/atst/app.py +++ b/atst/app.py @@ -24,6 +24,7 @@ from atst.models.permissions import Permissions from atst.eda_client import MockEDAClient from atst.uploader import Uploader from atst.utils import mailer +from atst.utils.form_cache import FormCache from atst.queue import queue @@ -67,6 +68,8 @@ def make_app(config): if ENV != "prod": app.register_blueprint(dev_routes) + app.form_cache = FormCache(app.redis) + apply_authentication(app) return app diff --git a/atst/utils/form_cache.py b/atst/utils/form_cache.py index e8e249f5..f199bdc5 100644 --- a/atst/utils/form_cache.py +++ b/atst/utils/form_cache.py @@ -16,3 +16,27 @@ def cache_form_data(redis, formdata, expiry_seconds=3600, key_prefix=DEFAULT_CAC def retrieve_form_data(redis, formdata_key, key_prefix="formcache"): data = redis.get("{}:{}".format(key_prefix, formdata_key)) return pickle.loads(data) + + +class FormCache(object): + + def __init__(self, redis): + self.redis = redis + + def from_request(self, http_request): + cache_key = http_request.args.get("formCache") + if cache_key: + return self.read(cache_key) + + def write(self, formdata, expiry_seconds=3600, key_prefix="formcache"): + value = pickle.dumps(formdata) + hash_ = hashlib.sha1(os.urandom(64)).hexdigest() + self.redis.setex(name=self._key(key_prefix, hash_), value=value, time=expiry_seconds) + + def read(self, formdata_key, key_prefix="formcache"): + data = self.redis.get(self._key(key_prefix, formdata_key)) + return pickle.loads(data) + + @staticmethod + def _key(prefix, hash_): + return "{}:{}".format(prefix, hash_) From 365a60015235068afaca5005a4ff4dfc46a5c29e Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 15 Nov 2018 16:03:40 -0500 Subject: [PATCH 05/16] Cache new request form data --- atst/routes/requests/financial_verification.py | 4 ++-- atst/routes/requests/requests_form.py | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 9f939ea0..dc2f10c0 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -13,7 +13,7 @@ from atst.domain.requests.financial_verification import ( ) from atst.models.attachment import Attachment from atst.domain.task_orders import TaskOrders -from atst.utils.form_cache import retrieve_form_data +from atst.utils.form_cache import FormCache def fv_extended(_http_request): @@ -95,7 +95,7 @@ class FinancialVerificationBase(object): def existing_form_data(): key = http_request.args.get("formCache") if key: - return retrieve_form_data(app.redis, key) + return FormCache(app.redis).read(key) class GetFinancialVerificationForm(FinancialVerificationBase): diff --git a/atst/routes/requests/requests_form.py b/atst/routes/requests/requests_form.py index c88c1c38..e1bb022c 100644 --- a/atst/routes/requests/requests_form.py +++ b/atst/routes/requests/requests_form.py @@ -1,4 +1,4 @@ -from flask import g, redirect, render_template, url_for, request as http_request +from flask import g, redirect, render_template, url_for, request as http_request, current_app from . import requests_bp from atst.domain.requests import Requests @@ -29,7 +29,8 @@ def option_data(): @requests_bp.route("/requests/new/", methods=["GET"]) def requests_form_new(screen): - jedi_flow = JEDIRequestFlow(screen, request=None, current_user=g.current_user) + cached_data = current_app.form_cache.from_request(http_request) + jedi_flow = JEDIRequestFlow(screen, request=None, current_user=g.current_user, post_data=cached_data) return render_template( "requests/screen-%d.html" % int(screen), From cd4a5df2d685d823e5558997bcabc1d47d654bf7 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Fri, 16 Nov 2018 10:59:11 -0500 Subject: [PATCH 06/16] Reset finver form to prevent UII nonsense --- atst/routes/requests/financial_verification.py | 1 + 1 file changed, 1 insertion(+) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index dc2f10c0..340a00dc 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -106,6 +106,7 @@ class GetFinancialVerificationForm(FinancialVerificationBase): def execute(self): form = self._get_form(self.request, self.is_extended, formdata=existing_form_data()) + form.reset() return form From 6177f46587632cfcbee159f7760b6b471254b377 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Fri, 16 Nov 2018 11:38:58 -0500 Subject: [PATCH 07/16] Use FormCache --- atst/routes/errors.py | 3 +-- atst/utils/form_cache.py | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/atst/routes/errors.py b/atst/routes/errors.py index 56551ee0..039ae635 100644 --- a/atst/routes/errors.py +++ b/atst/routes/errors.py @@ -8,7 +8,6 @@ from atst.domain.invitations import ( ExpiredError as InvitationExpiredError, WrongUserError as InvitationWrongUserError, ) -from atst.utils.form_cache import cache_form_data def log_error(e): @@ -40,7 +39,7 @@ def make_error_pages(app): log_error(e) url_args = {"sessionExpired": True, "next": request.path} if request.method == "POST": - url_args["formCache"] = cache_form_data(app.redis, request.form) + url_args["formCache"] = app.form_cache.write(request.form) return redirect(url_for("atst.root", **url_args)) @app.errorhandler(Exception) diff --git a/atst/utils/form_cache.py b/atst/utils/form_cache.py index f199bdc5..8ee831b7 100644 --- a/atst/utils/form_cache.py +++ b/atst/utils/form_cache.py @@ -32,10 +32,11 @@ class FormCache(object): value = pickle.dumps(formdata) hash_ = hashlib.sha1(os.urandom(64)).hexdigest() self.redis.setex(name=self._key(key_prefix, hash_), value=value, time=expiry_seconds) + return hash_ def read(self, formdata_key, key_prefix="formcache"): data = self.redis.get(self._key(key_prefix, formdata_key)) - return pickle.loads(data) + return pickle.loads(data) if data is not None else {} @staticmethod def _key(prefix, hash_): From 5447be8b5281b50b128c76e10f26d6b19ca4f5e4 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Fri, 16 Nov 2018 11:42:33 -0500 Subject: [PATCH 08/16] Use FormCache everywhere --- atst/utils/form_cache.py | 16 ++-------------- tests/utils/test_form_cache.py | 18 ++++++++++++------ 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/atst/utils/form_cache.py b/atst/utils/form_cache.py index 8ee831b7..1769cee3 100644 --- a/atst/utils/form_cache.py +++ b/atst/utils/form_cache.py @@ -6,18 +6,6 @@ import pickle DEFAULT_CACHE_NAME = "formcache" -def cache_form_data(redis, formdata, expiry_seconds=3600, key_prefix=DEFAULT_CACHE_NAME): - value = pickle.dumps(formdata) - key = hashlib.sha1(os.urandom(64)).hexdigest() - redis.setex(name="{}:{}".format(key_prefix, key), value=value, time=expiry_seconds) - return key - - -def retrieve_form_data(redis, formdata_key, key_prefix="formcache"): - data = redis.get("{}:{}".format(key_prefix, formdata_key)) - return pickle.loads(data) - - class FormCache(object): def __init__(self, redis): @@ -28,13 +16,13 @@ class FormCache(object): if cache_key: return self.read(cache_key) - def write(self, formdata, expiry_seconds=3600, key_prefix="formcache"): + def write(self, formdata, expiry_seconds=3600, key_prefix=DEFAULT_CACHE_NAME): value = pickle.dumps(formdata) hash_ = hashlib.sha1(os.urandom(64)).hexdigest() self.redis.setex(name=self._key(key_prefix, hash_), value=value, time=expiry_seconds) return hash_ - def read(self, formdata_key, key_prefix="formcache"): + def read(self, formdata_key, key_prefix=DEFAULT_CACHE_NAME): data = self.redis.get(self._key(key_prefix, formdata_key)) return pickle.loads(data) if data is not None else {} diff --git a/tests/utils/test_form_cache.py b/tests/utils/test_form_cache.py index 7eaabeee..f59b9ef5 100644 --- a/tests/utils/test_form_cache.py +++ b/tests/utils/test_form_cache.py @@ -1,15 +1,21 @@ +import pytest from werkzeug.datastructures import ImmutableDict -from atst.utils.form_cache import DEFAULT_CACHE_NAME, cache_form_data, retrieve_form_data +from atst.utils.form_cache import DEFAULT_CACHE_NAME, FormCache -def test_cache_form_data(app): + +@pytest.fixture +def form_cache(app): + return FormCache(app.redis) + +def test_cache_form_data(app, form_cache): data = ImmutableDict({"kessel_run": "12 parsecs"}) - key = cache_form_data(app.redis, data) + key = form_cache.write(data) assert app.redis.get("{}:{}".format(DEFAULT_CACHE_NAME, key)) -def test_retrieve_form_data(app): +def test_retrieve_form_data(form_cache): data = ImmutableDict({"class": "corellian"}) - key = cache_form_data(app.redis, data) - retrieved = retrieve_form_data(app.redis, key) + key = form_cache.write(data) + retrieved = form_cache.read(key) assert retrieved == data From 2f04571767eb7cb6bdc14edd33a0d2c6a8e1e14e Mon Sep 17 00:00:00 2001 From: richard-dds Date: Fri, 16 Nov 2018 13:17:31 -0500 Subject: [PATCH 09/16] Use app's form_cache --- atst/routes/requests/financial_verification.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 340a00dc..9a04e88f 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -13,7 +13,6 @@ from atst.domain.requests.financial_verification import ( ) from atst.models.attachment import Attachment from atst.domain.task_orders import TaskOrders -from atst.utils.form_cache import FormCache def fv_extended(_http_request): @@ -95,7 +94,7 @@ class FinancialVerificationBase(object): def existing_form_data(): key = http_request.args.get("formCache") if key: - return FormCache(app.redis).read(key) + return app.form_cache.read(key) class GetFinancialVerificationForm(FinancialVerificationBase): From 7d78ba4d65ba926a466e87da8555ddb3dd785583 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Fri, 16 Nov 2018 13:20:55 -0500 Subject: [PATCH 10/16] Use form_cache.from_request --- atst/routes/requests/financial_verification.py | 16 +++++++--------- atst/routes/requests/requests_form.py | 13 +++++++++++-- atst/utils/form_cache.py | 5 +++-- tests/utils/test_form_cache.py | 1 + 4 files changed, 22 insertions(+), 13 deletions(-) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 9a04e88f..2f609d31 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -91,20 +91,15 @@ class FinancialVerificationBase(object): raise FormValidationError(form) -def existing_form_data(): - key = http_request.args.get("formCache") - if key: - return app.form_cache.read(key) - - class GetFinancialVerificationForm(FinancialVerificationBase): - def __init__(self, user, request, is_extended=False): + def __init__(self, user, request, cached_data=None, is_extended=False): self.user = user self.request = request + self.cached_data = cached_data or {} self.is_extended = is_extended def execute(self): - form = self._get_form(self.request, self.is_extended, formdata=existing_form_data()) + form = self._get_form(self.request, self.is_extended, formdata=self.cached_data) form.reset() return form @@ -199,7 +194,10 @@ def financial_verification(request_id): ) form = GetFinancialVerificationForm( - g.current_user, request, is_extended=is_extended + g.current_user, + request, + is_extended=is_extended, + cached_data=app.form_cache.from_request(http_request), ).execute() return render_template( diff --git a/atst/routes/requests/requests_form.py b/atst/routes/requests/requests_form.py index e1bb022c..a2a04e4f 100644 --- a/atst/routes/requests/requests_form.py +++ b/atst/routes/requests/requests_form.py @@ -1,4 +1,11 @@ -from flask import g, redirect, render_template, url_for, request as http_request, current_app +from flask import ( + g, + redirect, + render_template, + url_for, + request as http_request, + current_app, +) from . import requests_bp from atst.domain.requests import Requests @@ -30,7 +37,9 @@ def option_data(): @requests_bp.route("/requests/new/", methods=["GET"]) def requests_form_new(screen): cached_data = current_app.form_cache.from_request(http_request) - jedi_flow = JEDIRequestFlow(screen, request=None, current_user=g.current_user, post_data=cached_data) + jedi_flow = JEDIRequestFlow( + screen, request=None, current_user=g.current_user, post_data=cached_data + ) return render_template( "requests/screen-%d.html" % int(screen), diff --git a/atst/utils/form_cache.py b/atst/utils/form_cache.py index 1769cee3..7d359227 100644 --- a/atst/utils/form_cache.py +++ b/atst/utils/form_cache.py @@ -7,7 +7,6 @@ DEFAULT_CACHE_NAME = "formcache" class FormCache(object): - def __init__(self, redis): self.redis = redis @@ -19,7 +18,9 @@ class FormCache(object): def write(self, formdata, expiry_seconds=3600, key_prefix=DEFAULT_CACHE_NAME): value = pickle.dumps(formdata) hash_ = hashlib.sha1(os.urandom(64)).hexdigest() - self.redis.setex(name=self._key(key_prefix, hash_), value=value, time=expiry_seconds) + self.redis.setex( + name=self._key(key_prefix, hash_), value=value, time=expiry_seconds + ) return hash_ def read(self, formdata_key, key_prefix=DEFAULT_CACHE_NAME): diff --git a/tests/utils/test_form_cache.py b/tests/utils/test_form_cache.py index f59b9ef5..17998ee7 100644 --- a/tests/utils/test_form_cache.py +++ b/tests/utils/test_form_cache.py @@ -8,6 +8,7 @@ from atst.utils.form_cache import DEFAULT_CACHE_NAME, FormCache def form_cache(app): return FormCache(app.redis) + def test_cache_form_data(app, form_cache): data = ImmutableDict({"kessel_run": "12 parsecs"}) key = form_cache.write(data) From af105e08bc5c2f53c4dc1ba7a0149f885d1584f5 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Fri, 16 Nov 2018 14:02:10 -0500 Subject: [PATCH 11/16] Use more secure sha256 --- atst/utils/form_cache.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/atst/utils/form_cache.py b/atst/utils/form_cache.py index 7d359227..d9ac5b24 100644 --- a/atst/utils/form_cache.py +++ b/atst/utils/form_cache.py @@ -1,5 +1,4 @@ -import os -import hashlib +from hashlib import sha256 import pickle @@ -17,7 +16,7 @@ class FormCache(object): def write(self, formdata, expiry_seconds=3600, key_prefix=DEFAULT_CACHE_NAME): value = pickle.dumps(formdata) - hash_ = hashlib.sha1(os.urandom(64)).hexdigest() + hash_ = sha256().hexdigest() self.redis.setex( name=self._key(key_prefix, hash_), value=value, time=expiry_seconds ) From 29f9520dda64f3504d242a9d70961d760791353b Mon Sep 17 00:00:00 2001 From: richard-dds Date: Fri, 16 Nov 2018 14:12:42 -0500 Subject: [PATCH 12/16] Use json instead of pickle --- atst/utils/form_cache.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/atst/utils/form_cache.py b/atst/utils/form_cache.py index d9ac5b24..583ca611 100644 --- a/atst/utils/form_cache.py +++ b/atst/utils/form_cache.py @@ -1,5 +1,5 @@ from hashlib import sha256 -import pickle +import json DEFAULT_CACHE_NAME = "formcache" @@ -15,7 +15,7 @@ class FormCache(object): return self.read(cache_key) def write(self, formdata, expiry_seconds=3600, key_prefix=DEFAULT_CACHE_NAME): - value = pickle.dumps(formdata) + value = json.dumps(formdata) hash_ = sha256().hexdigest() self.redis.setex( name=self._key(key_prefix, hash_), value=value, time=expiry_seconds @@ -24,7 +24,7 @@ class FormCache(object): def read(self, formdata_key, key_prefix=DEFAULT_CACHE_NAME): data = self.redis.get(self._key(key_prefix, formdata_key)) - return pickle.loads(data) if data is not None else {} + return json.loads(data) if data is not None else {} @staticmethod def _key(prefix, hash_): From de12aee163b8f34ab847666f962c043a5227ca60 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 19 Nov 2018 11:38:22 -0500 Subject: [PATCH 13/16] .read() should return an ImmutableMultiDict --- atst/utils/form_cache.py | 4 +++- tests/utils/test_form_cache.py | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/atst/utils/form_cache.py b/atst/utils/form_cache.py index 583ca611..3ce4f17a 100644 --- a/atst/utils/form_cache.py +++ b/atst/utils/form_cache.py @@ -1,5 +1,6 @@ from hashlib import sha256 import json +from werkzeug.datastructures import ImmutableMultiDict DEFAULT_CACHE_NAME = "formcache" @@ -24,7 +25,8 @@ class FormCache(object): def read(self, formdata_key, key_prefix=DEFAULT_CACHE_NAME): data = self.redis.get(self._key(key_prefix, formdata_key)) - return json.loads(data) if data is not None else {} + dict_data = json.loads(data) if data is not None else {} + return ImmutableMultiDict(dict_data) @staticmethod def _key(prefix, hash_): diff --git a/tests/utils/test_form_cache.py b/tests/utils/test_form_cache.py index 17998ee7..c399acd1 100644 --- a/tests/utils/test_form_cache.py +++ b/tests/utils/test_form_cache.py @@ -1,5 +1,5 @@ import pytest -from werkzeug.datastructures import ImmutableDict +from werkzeug.datastructures import ImmutableMultiDict from atst.utils.form_cache import DEFAULT_CACHE_NAME, FormCache @@ -10,13 +10,13 @@ def form_cache(app): def test_cache_form_data(app, form_cache): - data = ImmutableDict({"kessel_run": "12 parsecs"}) + data = ImmutableMultiDict({"kessel_run": "12 parsecs"}) key = form_cache.write(data) assert app.redis.get("{}:{}".format(DEFAULT_CACHE_NAME, key)) def test_retrieve_form_data(form_cache): - data = ImmutableDict({"class": "corellian"}) + data = ImmutableMultiDict({"class": "corellian"}) key = form_cache.write(data) retrieved = form_cache.read(key) assert retrieved == data From 1d5bfed5567c4223be7276165cd56873be6cad90 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 19 Nov 2018 11:46:21 -0500 Subject: [PATCH 14/16] Exactract "formCache' into form_cache.PARAM_NAME --- atst/routes/__init__.py | 6 ++++-- atst/routes/errors.py | 2 +- atst/utils/form_cache.py | 4 +++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/atst/routes/__init__.py b/atst/routes/__init__.py index 0ea12952..214fb35c 100644 --- a/atst/routes/__init__.py +++ b/atst/routes/__init__.py @@ -97,8 +97,10 @@ def _make_authentication_context(): def redirect_after_login_url(): if request.args.get("next"): returl = request.args.get("next") - if request.args.get("formCache"): - returl += "?" + url.urlencode({"formCache": request.args.get("formCache")}) + if request.args.get(app.form_cache.PARAM_NAME): + returl += "?" + url.urlencode( + {app.form_cache.PARAM_NAME: request.args.get(app.form_cache.PARAM_NAME)} + ) return returl else: return url_for("atst.home") diff --git a/atst/routes/errors.py b/atst/routes/errors.py index 039ae635..3f32b110 100644 --- a/atst/routes/errors.py +++ b/atst/routes/errors.py @@ -39,7 +39,7 @@ def make_error_pages(app): log_error(e) url_args = {"sessionExpired": True, "next": request.path} if request.method == "POST": - url_args["formCache"] = app.form_cache.write(request.form) + url_args[app.form_cache.PARAM_NAME] = app.form_cache.write(request.form) return redirect(url_for("atst.root", **url_args)) @app.errorhandler(Exception) diff --git a/atst/utils/form_cache.py b/atst/utils/form_cache.py index 3ce4f17a..c7ac6d80 100644 --- a/atst/utils/form_cache.py +++ b/atst/utils/form_cache.py @@ -7,11 +7,13 @@ DEFAULT_CACHE_NAME = "formcache" class FormCache(object): + PARAM_NAME = "formCache" + def __init__(self, redis): self.redis = redis def from_request(self, http_request): - cache_key = http_request.args.get("formCache") + cache_key = http_request.args.get(self.PARAM_NAME) if cache_key: return self.read(cache_key) From 26cd9b4cb00540d6f53b8615fd2321a18a7eb331 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 19 Nov 2018 14:30:24 -0500 Subject: [PATCH 15/16] Automatically use cached form data --- atst/forms/forms.py | 11 ++++++++++- atst/routes/requests/financial_verification.py | 10 +++------- atst/routes/requests/requests_form.py | 5 +---- atst/utils/form_cache.py | 11 ++++++++--- 4 files changed, 22 insertions(+), 15 deletions(-) diff --git a/atst/forms/forms.py b/atst/forms/forms.py index ce0ff791..44405b56 100644 --- a/atst/forms/forms.py +++ b/atst/forms/forms.py @@ -1,7 +1,8 @@ from flask_wtf import FlaskForm +from flask import current_app, request as http_request -class ValidatedForm(FlaskForm): +class _ValidatedForm(FlaskForm): def perform_extra_validation(self, *args, **kwargs): """Performs any applicable extra validation. Must return True if the form is valid or False otherwise.""" @@ -12,3 +13,11 @@ class ValidatedForm(FlaskForm): _data = super().data _data.pop("csrf_token", None) return _data + + +class ValidatedForm(_ValidatedForm): + def __init__(self, formdata=None, **kwargs): + formdata = formdata or {} + cached_data = current_app.form_cache.from_request(http_request) + cached_data.update(formdata) + super().__init__(cached_data, **kwargs) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 2f609d31..65c6d8ed 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -92,14 +92,13 @@ class FinancialVerificationBase(object): class GetFinancialVerificationForm(FinancialVerificationBase): - def __init__(self, user, request, cached_data=None, is_extended=False): + def __init__(self, user, request, is_extended=False): self.user = user self.request = request - self.cached_data = cached_data or {} self.is_extended = is_extended def execute(self): - form = self._get_form(self.request, self.is_extended, formdata=self.cached_data) + form = self._get_form(self.request, self.is_extended) form.reset() return form @@ -194,10 +193,7 @@ def financial_verification(request_id): ) form = GetFinancialVerificationForm( - g.current_user, - request, - is_extended=is_extended, - cached_data=app.form_cache.from_request(http_request), + g.current_user, request, is_extended=is_extended ).execute() return render_template( diff --git a/atst/routes/requests/requests_form.py b/atst/routes/requests/requests_form.py index a2a04e4f..617b0940 100644 --- a/atst/routes/requests/requests_form.py +++ b/atst/routes/requests/requests_form.py @@ -36,10 +36,7 @@ def option_data(): @requests_bp.route("/requests/new/", methods=["GET"]) def requests_form_new(screen): - cached_data = current_app.form_cache.from_request(http_request) - jedi_flow = JEDIRequestFlow( - screen, request=None, current_user=g.current_user, post_data=cached_data - ) + jedi_flow = JEDIRequestFlow(screen, request=None, current_user=g.current_user) return render_template( "requests/screen-%d.html" % int(screen), diff --git a/atst/utils/form_cache.py b/atst/utils/form_cache.py index c7ac6d80..7dec1470 100644 --- a/atst/utils/form_cache.py +++ b/atst/utils/form_cache.py @@ -1,6 +1,6 @@ from hashlib import sha256 import json -from werkzeug.datastructures import ImmutableMultiDict +from werkzeug.datastructures import MultiDict DEFAULT_CACHE_NAME = "formcache" @@ -16,10 +16,11 @@ class FormCache(object): cache_key = http_request.args.get(self.PARAM_NAME) if cache_key: return self.read(cache_key) + return MultiDict() def write(self, formdata, expiry_seconds=3600, key_prefix=DEFAULT_CACHE_NAME): value = json.dumps(formdata) - hash_ = sha256().hexdigest() + hash_ = self._hash() self.redis.setex( name=self._key(key_prefix, hash_), value=value, time=expiry_seconds ) @@ -28,8 +29,12 @@ class FormCache(object): def read(self, formdata_key, key_prefix=DEFAULT_CACHE_NAME): data = self.redis.get(self._key(key_prefix, formdata_key)) dict_data = json.loads(data) if data is not None else {} - return ImmutableMultiDict(dict_data) + return MultiDict(dict_data) @staticmethod def _key(prefix, hash_): return "{}:{}".format(prefix, hash_) + + @staticmethod + def _hash(): + return sha256().hexdigest() From 3215cfca32013781c2df988f0fbb72bbc8eb8420 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Mon, 19 Nov 2018 14:37:24 -0500 Subject: [PATCH 16/16] Rename to CacheableForm --- atst/forms/ccpo_review.py | 4 ++-- atst/forms/edit_user.py | 4 ++-- atst/forms/financial.py | 8 ++++---- atst/forms/forms.py | 4 ++-- atst/forms/internal_comment.py | 4 ++-- atst/forms/new_request.py | 10 +++++----- atst/forms/workspace.py | 4 ++-- atst/routes/requests/financial_verification.py | 2 +- atst/routes/requests/requests_form.py | 9 +-------- 9 files changed, 21 insertions(+), 28 deletions(-) diff --git a/atst/forms/ccpo_review.py b/atst/forms/ccpo_review.py index b2413a3e..b727cea0 100644 --- a/atst/forms/ccpo_review.py +++ b/atst/forms/ccpo_review.py @@ -2,11 +2,11 @@ from wtforms.fields.html5 import EmailField, TelField from wtforms.fields import StringField, TextAreaField from wtforms.validators import Email, Optional -from .forms import ValidatedForm +from .forms import CacheableForm from .validators import Name, PhoneNumber -class CCPOReviewForm(ValidatedForm): +class CCPOReviewForm(CacheableForm): comment = TextAreaField( "Instructions or comments", description="Provide instructions or notes for additional information that is necessary to approve the request here. The requestor may then re-submit the updated request or initiate contact outside of AT-AT if further discussion is required. This message will be shared with the person making the JEDI request..", diff --git a/atst/forms/edit_user.py b/atst/forms/edit_user.py index 3728bf84..6d406942 100644 --- a/atst/forms/edit_user.py +++ b/atst/forms/edit_user.py @@ -5,7 +5,7 @@ from wtforms.fields import RadioField, StringField from wtforms.validators import Email, DataRequired, Optional from .fields import SelectField -from .forms import ValidatedForm +from .forms import CacheableForm from .data import SERVICE_BRANCHES from atst.models.user import User @@ -77,7 +77,7 @@ def inherit_user_field(field_name): return inherit_field(USER_FIELDS[field_name], required=required) -class EditUserForm(ValidatedForm): +class EditUserForm(CacheableForm): first_name = inherit_user_field("first_name") last_name = inherit_user_field("last_name") diff --git a/atst/forms/financial.py b/atst/forms/financial.py index cfc86a20..7ee17cfa 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -7,7 +7,7 @@ from flask_wtf.file import FileAllowed from werkzeug.datastructures import FileStorage from .fields import NewlineListField, SelectField, NumberStringField -from atst.forms.forms import ValidatedForm +from atst.forms.forms import CacheableForm from .data import FUNDING_TYPES from .validators import DateRange @@ -31,7 +31,7 @@ def coerce_choice(val): return val.value -class TaskOrderForm(ValidatedForm): +class TaskOrderForm(CacheableForm): def do_validate_number(self): for field in self: if field.name != "task_order-number": @@ -127,7 +127,7 @@ class TaskOrderForm(ValidatedForm): ) -class RequestFinancialVerificationForm(ValidatedForm): +class RequestFinancialVerificationForm(CacheableForm): uii_ids = NewlineListField( "Unique Item Identifier (UII)s related to your application(s) if you already have them.", description="If you have more than one UII, place each one on a new line.", @@ -174,7 +174,7 @@ class RequestFinancialVerificationForm(ValidatedForm): self.uii_ids.process_data(self.uii_ids.data) -class FinancialVerificationForm(ValidatedForm): +class FinancialVerificationForm(CacheableForm): task_order = FormField(TaskOrderForm) request = FormField(RequestFinancialVerificationForm) diff --git a/atst/forms/forms.py b/atst/forms/forms.py index 44405b56..eeeb48d2 100644 --- a/atst/forms/forms.py +++ b/atst/forms/forms.py @@ -2,7 +2,7 @@ from flask_wtf import FlaskForm from flask import current_app, request as http_request -class _ValidatedForm(FlaskForm): +class ValidatedForm(FlaskForm): def perform_extra_validation(self, *args, **kwargs): """Performs any applicable extra validation. Must return True if the form is valid or False otherwise.""" @@ -15,7 +15,7 @@ class _ValidatedForm(FlaskForm): return _data -class ValidatedForm(_ValidatedForm): +class CacheableForm(ValidatedForm): def __init__(self, formdata=None, **kwargs): formdata = formdata or {} cached_data = current_app.form_cache.from_request(http_request) diff --git a/atst/forms/internal_comment.py b/atst/forms/internal_comment.py index 583db7a1..7711ff04 100644 --- a/atst/forms/internal_comment.py +++ b/atst/forms/internal_comment.py @@ -1,10 +1,10 @@ from wtforms.fields import TextAreaField from wtforms.validators import InputRequired -from .forms import ValidatedForm +from .forms import CacheableForm -class InternalCommentForm(ValidatedForm): +class InternalCommentForm(CacheableForm): text = TextAreaField( "CCPO Internal Notes", default="", diff --git a/atst/forms/new_request.py b/atst/forms/new_request.py index 4b564c37..bc05f412 100644 --- a/atst/forms/new_request.py +++ b/atst/forms/new_request.py @@ -4,7 +4,7 @@ from wtforms.fields import BooleanField, RadioField, StringField, TextAreaField from wtforms.validators import Email, Length, Optional, InputRequired, DataRequired from .fields import SelectField -from .forms import ValidatedForm +from .forms import CacheableForm from .edit_user import USER_FIELDS, inherit_field from .data import ( SERVICE_BRANCHES, @@ -16,7 +16,7 @@ from .validators import DateRange, IsNumber from atst.domain.requests import Requests -class DetailsOfUseForm(ValidatedForm): +class DetailsOfUseForm(CacheableForm): def validate(self, *args, **kwargs): if self.jedi_migration.data == "no": self.rationalization_software_systems.validators.append(Optional()) @@ -162,7 +162,7 @@ class DetailsOfUseForm(ValidatedForm): ) -class InformationAboutYouForm(ValidatedForm): +class InformationAboutYouForm(CacheableForm): fname_request = inherit_field(USER_FIELDS["first_name"]) lname_request = inherit_field(USER_FIELDS["last_name"]) email_request = inherit_field(USER_FIELDS["email"]) @@ -174,7 +174,7 @@ class InformationAboutYouForm(ValidatedForm): date_latest_training = inherit_field(USER_FIELDS["date_latest_training"]) -class WorkspaceOwnerForm(ValidatedForm): +class WorkspaceOwnerForm(CacheableForm): def validate(self, *args, **kwargs): if self.am_poc.data: # Prepend Optional validators so that the validation chain @@ -203,5 +203,5 @@ class WorkspaceOwnerForm(ValidatedForm): ) -class ReviewAndSubmitForm(ValidatedForm): +class ReviewAndSubmitForm(CacheableForm): reviewed = BooleanField("I have reviewed this data and it is correct.") diff --git a/atst/forms/workspace.py b/atst/forms/workspace.py index 9fde98bb..5434676f 100644 --- a/atst/forms/workspace.py +++ b/atst/forms/workspace.py @@ -1,10 +1,10 @@ from wtforms.fields import StringField from wtforms.validators import Length -from .forms import ValidatedForm +from .forms import CacheableForm -class WorkspaceForm(ValidatedForm): +class WorkspaceForm(CacheableForm): name = StringField( "Workspace Name", validators=[ diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 65c6d8ed..8f2369ac 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -1,4 +1,4 @@ -from flask import g, render_template, redirect, url_for, current_app as app +from flask import g, render_template, redirect, url_for from flask import request as http_request from werkzeug.datastructures import ImmutableMultiDict, FileStorage diff --git a/atst/routes/requests/requests_form.py b/atst/routes/requests/requests_form.py index 617b0940..c88c1c38 100644 --- a/atst/routes/requests/requests_form.py +++ b/atst/routes/requests/requests_form.py @@ -1,11 +1,4 @@ -from flask import ( - g, - redirect, - render_template, - url_for, - request as http_request, - current_app, -) +from flask import g, redirect, render_template, url_for, request as http_request from . import requests_bp from atst.domain.requests import Requests