diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b0066df..03a31c40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,9 @@ 0.3.1 (2021-10-??) ------------------ -* Bugfix: Large notebooks were causing serialisation errors; now properly stored in gridfs -* Improvement: index page should be a lot quicker due to storage improvements +* Improvement: index page should be a lot quicker due to storage improvements. +* Bugfix: hide_code and generate_pdf options now work as intended with the scheduler. +* Bugfix: Large notebooks were causing serialisation errors; now safely stored in gridfs. * **Incompatibility**: Reports run with this version onwards will not be readable by older versions of Notebooker. 0.3.0 (2021-10-05) diff --git a/notebooker/web/routes/run_report.py b/notebooker/web/routes/run_report.py index 7cfbea61..6312dc85 100644 --- a/notebooker/web/routes/run_report.py +++ b/notebooker/web/routes/run_report.py @@ -232,21 +232,24 @@ class RunReportParams(NamedTuple): def validate_run_params(params, issues: List[str]) -> RunReportParams: + logger.info(f"Validating input params: {params}") # Find and cleanse the title of the report report_title = validate_title(params.get("report_title"), issues) # Get mailto email address mailto = validate_mailto(params.get("mailto"), issues) - # Find whether to generate PDF output - generate_pdf_output = params.get("generate_pdf") == "on" - hide_code = params.get("hide_code") == "on" + # "on" comes from HTML, "True" comes from urlencoded JSON params + generate_pdf_output = params.get("generate_pdf") in ("on", "True") + hide_code = params.get("hide_code") in ("on", "True") - return RunReportParams( + out = RunReportParams( report_title=report_title, mailto=mailto, generate_pdf_output=generate_pdf_output, hide_code=hide_code, scheduler_job_id=params.get("scheduler_job_id"), ) + logger.info(f"Validated params: {out}") + return out def _handle_run_report( @@ -256,6 +259,13 @@ def _handle_run_report( if issues: return jsonify({"status": "Failed", "content": ("\n".join(issues))}) report_name = convert_report_name_url_to_path(report_name) + logger.info(f"Handling run report with parameters report_name={report_name} " + f"report_title={params.report_title}" + f"mailto={params.mailto} " + f"overrides_dict={overrides_dict} " + f"generate_pdf_output={params.generate_pdf_output} " + f"hide_code={params.hide_code} " + f"scheduler_job_id={params.scheduler_job_id}") try: job_id = run_report( report_name, diff --git a/notebooker/web/routes/scheduling.py b/notebooker/web/routes/scheduling.py index deafa64f..1419a3da 100644 --- a/notebooker/web/routes/scheduling.py +++ b/notebooker/web/routes/scheduling.py @@ -1,6 +1,6 @@ import json from typing import Optional, List -import uuid +import logging from apscheduler.jobstores.base import ConflictingIdError from flask import Blueprint, jsonify, render_template, current_app, request, url_for, abort @@ -12,6 +12,7 @@ from apscheduler.triggers import cron scheduling_bp = Blueprint("scheduling_bp", __name__) +logger = logging.getLogger(__name__) @scheduling_bp.route("/scheduler/health") @@ -101,6 +102,7 @@ def create_schedule(report_name): "hide_code": params.hide_code, "scheduler_job_id": job_id, } + logger.info(f"Creating job with params: {dict_params}") try: job = current_app.apscheduler.add_job( "notebooker.web.scheduler:run_report", jobstore="mongo", trigger=trigger, kwargs=dict_params, id=job_id diff --git a/notebooker/web/scheduler.py b/notebooker/web/scheduler.py index 9d766bc0..8146d472 100644 --- a/notebooker/web/scheduler.py +++ b/notebooker/web/scheduler.py @@ -18,6 +18,10 @@ def run_report( hide_code: bool, scheduler_job_id: str, ): + """ + This is the entrypoint of the scheduler; APScheduler has to + run a python function and so we invoke an API call from a thin wrapper. + """ if GLOBAL_CONFIG is None: url = f"http://localhost/run_report_json/{report_name}" else: diff --git a/tests/integration/web/test_run_report.py b/tests/integration/web/test_run_report.py new file mode 100644 index 00000000..d8cab378 --- /dev/null +++ b/tests/integration/web/test_run_report.py @@ -0,0 +1,39 @@ +import json +import urllib + +import mock + + +def test_run_report_json_parameters(flask_app, setup_workspace): + with flask_app.test_client() as client: + report_name = "fake/report" + overrides = {"a": 1} + report_title = "title" + mailto = "abc@email.asdkj" + generate_pdf = True + hide_code = True + scheduler_job_id = "abc/123" + payload = { + "overrides": json.dumps(overrides), + "report_title": report_title, + "mailto": mailto, + "generate_pdf": generate_pdf, + "hide_code": hide_code, + "scheduler_job_id": scheduler_job_id, + } + with mock.patch("notebooker.web.routes.run_report.run_report") as rr: + rr.return_value = "fake_job_id" + rv = client.post( + f"/run_report_json/{report_name}?{urllib.parse.urlencode(payload)}" + ) + assert rv.data == b'{"id":"fake_job_id"}\n' + assert rv.status_code == 202, rv.data + rr.assert_called_with( + report_name, + report_title, + mailto, + overrides, + generate_pdf_output=generate_pdf, + hide_code=hide_code, + scheduler_job_id=scheduler_job_id, + ) diff --git a/tests/integration/web/test_scheduling.py b/tests/integration/web/test_scheduling.py index f7fd2e4a..7c039e05 100644 --- a/tests/integration/web/test_scheduling.py +++ b/tests/integration/web/test_scheduling.py @@ -12,6 +12,7 @@ def test_create_schedule(flask_app, setup_workspace): "report_name": "fake/report", "overrides": "", "mailto": "", + "generate_pdf": True, "cron_schedule": "* * * * *", }, ) @@ -23,7 +24,7 @@ def test_create_schedule(flask_app, setup_workspace): "delete_url": "/scheduler/fake/report_test2", "id": "fake/report_test2", "params": { - "generate_pdf": False, + "generate_pdf": True, "hide_code": False, "mailto": "", "overrides": "", @@ -46,6 +47,27 @@ def test_create_schedule(flask_app, setup_workspace): } +def test_scheduler_handles_booleans_properly(flask_app, setup_workspace): + with flask_app.test_client() as client: + rv = client.post( + "/scheduler/create/fake/report", + data={ + "report_title": "test2", + "report_name": "fake/report", + "overrides": "", + "mailto": "", + "generate_pdf": True, + "hide_code": True, + "cron_schedule": "* * * * *", + }, + ) + assert rv.status_code == 201 + data = json.loads(rv.data) + assert data.pop("next_run_time") + assert data["params"]["generate_pdf"] is True + assert data["params"]["hide_code"] is True + + def test_create_schedule_bad_report_name(flask_app, setup_workspace): with flask_app.test_client() as client: rv = client.post( diff --git a/tests/unit/test_run_report.py b/tests/unit/test_run_report.py index 0f60138b..115d8bf2 100644 --- a/tests/unit/test_run_report.py +++ b/tests/unit/test_run_report.py @@ -2,9 +2,10 @@ import sys import mock +from werkzeug.datastructures import CombinedMultiDict, ImmutableMultiDict from notebooker.constants import DEFAULT_SERIALIZER -from notebooker.web.routes.run_report import _monitor_stderr +from notebooker.web.routes.run_report import _monitor_stderr, validate_run_params, RunReportParams def test_monitor_stderr(): @@ -31,3 +32,32 @@ def test_monitor_stderr(): mock.call("abc123", new_lines=["This is going to stderr a bit later\n"]), ] ) + + +def test_validate_run_params(): + input_params = CombinedMultiDict( + [ + ImmutableMultiDict( + [ + ("overrides", "{}"), + ("report_title", "asdas"), + ("mailto", ""), + ("generate_pdf", "True"), + ("hide_code", "True"), + ("scheduler_job_id", "plot_random_asdas"), + ] + ), + ImmutableMultiDict([]), + ] + ) + issues = [] + expected_output = RunReportParams( + report_title="asdas", + mailto="", + generate_pdf_output=True, + hide_code=True, + scheduler_job_id="plot_random_asdas", + ) + actual_output = validate_run_params(input_params, issues) + assert issues == [] + assert actual_output == expected_output