From 4cddb170708f18ee0e7c6a56dcd3df6b25a56b97 Mon Sep 17 00:00:00 2001 From: JP Bruins Slot Date: Tue, 1 Oct 2024 16:09:39 +0200 Subject: [PATCH 1/2] Fix grace period is being used instead of interval for boefjes that have interval specified in scheduler (#3602) Co-authored-by: Jan Klopper --- mula/scheduler/schedulers/boefje.py | 16 ++++++++++++---- mula/tests/integration/test_boefje_scheduler.py | 8 ++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/mula/scheduler/schedulers/boefje.py b/mula/scheduler/schedulers/boefje.py index af92ea5e68b..5d2d19ffa5a 100644 --- a/mula/scheduler/schedulers/boefje.py +++ b/mula/scheduler/schedulers/boefje.py @@ -893,6 +893,16 @@ def has_boefje_task_grace_period_passed(self, task: BoefjeTask) -> bool: Returns: True if the grace period has passed, False otherwise. """ + # Does boefje have an interval specified? + plugin = self.ctx.services.katalogus.get_plugin_by_id_and_org_id( + task.boefje.id, + self.organisation.id, + ) + if plugin is not None and plugin.interval is not None and plugin.interval > 0: + timeout = timedelta(minutes=plugin.interval) + else: + timeout = timedelta(seconds=self.ctx.config.pq_grace_period) + try: task_db = self.ctx.datastores.task_store.get_latest_task_by_hash(task.hash) except Exception as exc_db: @@ -907,9 +917,7 @@ def has_boefje_task_grace_period_passed(self, task: BoefjeTask) -> bool: raise exc_db # Has grace period passed according to datastore? - if task_db is not None and datetime.now(timezone.utc) - task_db.modified_at < timedelta( - seconds=self.ctx.config.pq_grace_period - ): + if task_db is not None and datetime.now(timezone.utc) - task_db.modified_at < timeout: self.logger.debug( "Task has not passed grace period, according to the datastore", task_id=task_db.id, @@ -939,7 +947,7 @@ def has_boefje_task_grace_period_passed(self, task: BoefjeTask) -> bool: if ( task_bytes is not None and task_bytes.ended_at is not None - and datetime.now(timezone.utc) - task_bytes.ended_at < timedelta(seconds=self.ctx.config.pq_grace_period) + and datetime.now(timezone.utc) - task_bytes.ended_at < timeout ): self.logger.debug( "Task has not passed grace period, according to bytes", diff --git a/mula/tests/integration/test_boefje_scheduler.py b/mula/tests/integration/test_boefje_scheduler.py index 7fde9801190..faf8899628a 100644 --- a/mula/tests/integration/test_boefje_scheduler.py +++ b/mula/tests/integration/test_boefje_scheduler.py @@ -375,6 +375,7 @@ def test_has_boefje_task_started_running_stalled_before_grace_period(self): # Mock self.mock_get_latest_task_by_hash.return_value = task_db self.mock_get_last_run_boefje.return_value = None + self.mock_get_plugin.return_value = None # Act self.assertFalse(self.scheduler.has_boefje_task_stalled(boefje_task)) @@ -403,6 +404,7 @@ def test_has_boefje_task_started_running_stalled_after_grace_period(self): # Mock self.mock_get_latest_task_by_hash.return_value = task_db self.mock_get_last_run_boefje.return_value = None + self.mock_get_plugin.return_value = None # Act self.assertTrue(self.scheduler.has_boefje_task_stalled(boefje_task)) @@ -434,6 +436,7 @@ def test_has_boefje_task_started_running_mismatch_before_grace_period(self): # Mock self.mock_get_latest_task_by_hash.return_value = task_db self.mock_get_last_run_boefje.return_value = None + self.mock_get_plugin.return_value = None # Act with self.assertRaises(RuntimeError): @@ -468,6 +471,7 @@ def test_has_boefje_task_started_running_mismatch_after_grace_period(self): # Mock self.mock_get_latest_task_by_hash.return_value = task_db self.mock_get_last_run_boefje.return_value = None + self.mock_get_plugin.return_value = None # Act self.assertFalse(self.scheduler.has_boefje_task_started_running(boefje_task)) @@ -497,6 +501,7 @@ def test_has_boefje_task_grace_period_passed_datastore_passed(self): # Mock self.mock_get_latest_task_by_hash.return_value = task_db self.mock_get_last_run_boefje.return_value = None + self.mock_get_plugin.return_value = None # Act has_passed = self.scheduler.has_boefje_task_grace_period_passed(boefje_task) @@ -529,6 +534,7 @@ def test_has_boefje_task_grace_period_passed_datastore_not_passed(self): # Mock self.mock_get_latest_task_by_hash.return_value = task_db self.mock_get_last_run_boefje.return_value = None + self.mock_get_plugin.return_value = None # Act has_passed = self.scheduler.has_boefje_task_grace_period_passed(boefje_task) @@ -567,6 +573,7 @@ def test_has_boefje_task_grace_period_passed_bytes_passed(self): # Mock self.mock_get_latest_task_by_hash.return_value = task_db self.mock_get_last_run_boefje.return_value = last_run_boefje + self.mock_get_plugin.return_value = None # Act has_passed = self.scheduler.has_boefje_task_grace_period_passed(boefje_task) @@ -605,6 +612,7 @@ def test_has_boefje_task_grace_period_passed_bytes_not_passed(self): # Mock self.mock_get_latest_task_by_hash.return_value = task_db self.mock_get_last_run_boefje.return_value = last_run_boefje + self.mock_get_plugin.return_value = None # Act has_passed = self.scheduler.has_boefje_task_grace_period_passed(boefje_task) From b3b052a6dba8516843e4f23c6d32a3882409f181 Mon Sep 17 00:00:00 2001 From: Roelof Korporaal Date: Tue, 1 Oct 2024 16:20:01 +0200 Subject: [PATCH 2/2] Use identifiers on modal triggers and modal component instead of integral trigger (#3541) Co-authored-by: ammar92 Co-authored-by: Peter-Paul van Gemerden Co-authored-by: stephanie0x00 <9821756+stephanie0x00@users.noreply.github.com> Co-authored-by: Jan Klopper --- rocky/components/modal/README.md | 32 ++++--- rocky/components/modal/modal.py | 1 + rocky/components/modal/script.js | 92 +++++++++---------- rocky/components/modal/template.html | 9 +- .../templates/partials/plugin_tile_modal.html | 14 +-- 5 files changed, 74 insertions(+), 74 deletions(-) diff --git a/rocky/components/modal/README.md b/rocky/components/modal/README.md index 9dad3585dbe..b6f996e6dc4 100644 --- a/rocky/components/modal/README.md +++ b/rocky/components/modal/README.md @@ -20,22 +20,25 @@ First you need to add `{% load component_tags %}` at the top of your template. N {% endblock html_at_end_body %} ``` -After that, `{% component "modal" size="xx" %}` is enough to instantiate the dialog modal component where size should contain the appropriate class name to achieve the correct sizing. This can be either `dialog-small`, `dialog-medium` or `dialog-large`. +After that, `{% component "modal" modal_id="xx" size="xx" %}` is enough to instantiate the dialog modal component where `modal_id` should contain a unique identifier that must be also used in the triggers `data-modal-id` attribute, and `size` should contain the appropriate class name to achieve the correct sizing. This can be either `dialog-small`, `dialog-medium` or `dialog-large`. -### Slots and fills +### The trigger element -Each named `fill` corresponds with a placeholder/target `slot` in the component template. The contents between the `fill` tag will be passed to the corresponding `slot`. As shown in the below example it's possible to utilise Django template tags and `HTML` tags with these `fill` tags. This enables us to entirely build the contents of the modal in the template where we implement it. Because we can use `HTML` tags here, we can also use `forms` and leave the handling of said form up to the Django template that knows about the context and applicable data, where we implement the modal. The defaults are used when no `fill` tags are implemented for this slot at all. +The trigger element needs some explaining. This will be the element that `on click` will show the modal dialog, +the `element` gets assigned the click handler by JavaScript. +It's essential to include the data attribute `data-modal-id="xx"`, where "xx" is the same as the `id` attribute on the intended modal. This is what defines the modal dialog that this trigger is meant to target. The only exception to this is when an `` is used, with an `href` containing an anchor `"#xx"`, which points to `the modal-id` of the target modal. +While it might seem obvious to use a `button` or a `a` as a trigger, the modal is set up in a way that allows for any HTML element to be used as a trigger. +The trigger doesn't need to be part of the `{% component %}` itself and can be placed anywhere in the HTML template, though it makes sense to place them adjacent or as close as possible to each other. -There's a total of four slots you can fill: +### Slots and fills -1. `trigger`: call to action `button` by default, with the caption "Open modal". -2. `header`: empty by default -3. `content`: empty by default -4. `footer_buttons`: cancel `button` by default. To have _no buttons_ show at all, it's needed to implement empty `fill` tags for this `slot`. +Each named `fill` corresponds with a placeholder/target `slot` in the component template. The contents between the `fill` tag will be passed to the corresponding `slot`. As shown in the below example it's possible to utilise Django template tags and `HTML` tags with these `fill` tags. This enables us to entirely build the contents of the modal in the template where we implement it. Because we can use `HTML` tags here, we can also use `forms` and leave the handling of said form up to the Django template that knows about the context and applicable data, where we implement the modal. The defaults are used when no `fill` tags are implemented for this slot at all. -### The trigger element +There's a total of three slots you can fill: -The trigger `slot` is a special one. This needs to contain the HTML `element` that gets assigned the click handler by JavaScript. It's essential to include the `class="modal-trigger"` attribute, because this is what we target to assign the click handler, using JS. While it might seem obvious to use a `button` as a trigger, the modal is setup in a way that allows for any HTML element to be used as a trigger. +1. `header`: empty by default +2. `content`: empty by default +3. `footer_buttons`: cancel `button` by default. To have _no buttons_ show at all, it's needed to implement empty `fill` tags for this `slot`. ### CSS dependencies @@ -44,10 +47,11 @@ Including `{% component_css_dependencies %}` is needed to inject the reference t ### Example implementation ``` -{% component "modal" size="dialog-small" %} - {% fill "trigger" %} - - {% endfill %} + +``` + +``` +{% component "modal" modal_id="rename-modal" size="dialog-small" %} {% fill "header" %} {% translate "This is an example header." %} {% endfill %} diff --git a/rocky/components/modal/modal.py b/rocky/components/modal/modal.py index f3ad0d6dd6d..c2c55f668cb 100644 --- a/rocky/components/modal/modal.py +++ b/rocky/components/modal/modal.py @@ -8,6 +8,7 @@ class Modal(component.Component): def get_context_data(self, **kwargs): return { "size": kwargs["size"], + "modal_id": kwargs["modal_id"], } class Media: diff --git a/rocky/components/modal/script.js b/rocky/components/modal/script.js index 006c8cc8ccb..24e077f7b0a 100644 --- a/rocky/components/modal/script.js +++ b/rocky/components/modal/script.js @@ -1,6 +1,17 @@ import { onDomReady } from "../js/imports/utils.js"; -onDomReady(initDialogs); +onDomReady(function () { + initDialogs(); + openDialogFromUrl(); +}); + +export function openDialogFromUrl() { + // If ID is present in the URL on DomReady, open the dialog immediately. + let id = window.location.hash.slice(1); + if (id) { + showModalBasedOnAnchor(id); + } +} export function initDialogs(element) { let root = element || document; @@ -10,67 +21,52 @@ export function initDialogs(element) { } export function initDialog(modal) { - let input_elements = []; + let dialog_element = modal.querySelector("dialog"); + if (!dialog_element) return; - modal.querySelector(".modal-trigger").addEventListener("click", (event) => { - // Get and clone input elements to be able to "reset" them on "cancel". - input_elements = modal.querySelectorAll("input, textarea"); + let trigger = document.querySelector( + "[data-modal-id='" + dialog_element.id + "']:not(a)", + ); - // Store the initial value in a data attribute - input_elements.forEach((element) => { - element.defaultvalue = element.value; + // Check if trigger element is , if not, on click, + // alter the URL to open the dialog using the onhaschange event. + if (trigger) { + trigger.addEventListener("click", (event) => { + window.location.hash = "#" + dialog_element.id; }); + } - // Used ".closest" instead of ".parentNode" to make sure we stay flexible in terms of - // HTML-structure when implementing the trigger. - event.target.closest(".modal-wrapper").querySelector("dialog").showModal(); - }); - - modal.querySelector("dialog").addEventListener("click", (event) => { + dialog_element.addEventListener("click", (event) => { // The actual handling (like posting) of the input values should be done when implementing the component. - if (event.target.classList.contains("confirm-modal-button")) { - if (input_elements) { - // Closing is only allowed when the inputs are 'valid'. - if (checkValidity(input_elements)) { - event.target - .closest(".modal-wrapper") - .querySelector("dialog") - .close(); - } - return; - } - } // event.target.nodeName === 'DIALOG' is needed to check if the ::backdrop is clicked. if ( + event.target.classList.contains("confirm-modal-button") || event.target.classList.contains("close-modal-button") || event.target.nodeName === "DIALOG" ) { - // When canceling or closing using the 'x' remove the "error" styles. - input_elements.forEach((element) => { - element.classList.remove("error"); - }); - - // When canceling or closing the modal, the inputs get reset to their initial value. - input_elements.forEach((element) => { - element.value = element.defaultvalue; - }); - event.target.closest(".modal-wrapper").querySelector("dialog").close(); } }); -} - -export function checkValidity(elements) { - let valid = true; - elements.forEach((element) => { - if (!element.checkValidity()) { - valid = false; - element.classList.add("error"); - } else { - element.classList.remove("error"); - } + dialog_element.addEventListener("close", (event) => { + removeDialogAnchor(); }); +} - return valid; +export function removeDialogAnchor() { + // Remove the anchor from the URL when closing the modal + let baseUrl = window.location.toString().split("#")[0]; + window.history.pushState("", "Base URL", baseUrl); } + +export function showModalBasedOnAnchor(id) { + if (id && document.querySelector("dialog#" + id + ".modal")) { + // Show modal, selected by ID + document.querySelector("#" + id).showModal(); + } +} + +addEventListener("hashchange", function () { + let id = window.location.toString().split("#")[1]; + showModalBasedOnAnchor(id); +}); diff --git a/rocky/components/modal/template.html b/rocky/components/modal/template.html index 7e1235532f2..f0dc8be4356 100644 --- a/rocky/components/modal/template.html +++ b/rocky/components/modal/template.html @@ -3,13 +3,10 @@ {% load compress %}
- {% slot "trigger" %} - - {% endslot %} - + aria-modal="true" + role="dialog" + id="{{ modal_id }}">

{% slot "header" %}{% endslot %}

diff --git a/rocky/katalogus/templates/partials/plugin_tile_modal.html b/rocky/katalogus/templates/partials/plugin_tile_modal.html index 51479c5c035..dadf623ac0b 100644 --- a/rocky/katalogus/templates/partials/plugin_tile_modal.html +++ b/rocky/katalogus/templates/partials/plugin_tile_modal.html @@ -1,11 +1,12 @@ {% load i18n %} -{% component "modal" size="dialog-medium" %} -{% fill "trigger" %} -{% translate "See details" %} -{% endfill %} -{% fill "header" %} -{% translate "Boefje details" %} +{% translate "See details" %} +{% with modal_id=plugin.id|add:"-detail-modal" %} + {% component "modal" modal_id=modal_id size="dialog-medium" %} + {% fill "header" %} + {% translate "Boefje details" %} {% endfill %} {% fill "content" %}
@@ -86,3 +87,4 @@

{% translate "Produces" %}

{% endfill %} {% endcomponent %} {% component_css_dependencies %} +{% endwith %}