Merge pull request #60 from man-group/fix-scheduler-options

Fix hide_code and generate_pdf scheduler options

Author: jonbannister

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)

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,

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

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:

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 = "[email protected]"
+        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,
+            )

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(

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