From 61e601a46d4f233b6453cdf2e69fa0b45c50ca23 Mon Sep 17 00:00:00 2001 From: dgtlmoon Date: Fri, 16 Oct 2026 10:37:13 +0200 Subject: [PATCH] Fixing notification settings still trying to access main settings form that caused a crash on save #4554 --- .../blueprint/settings/__init__.py | 10 +-- changedetectionio/forms.py | 61 +++++++++--------- .../tests/test_settings_notifications_page.py | 62 +++++++++++++++++-- changedetectionio/widgets/ternary_boolean.py | 2 +- changedetectionio/widgets/test_custom_text.py | 4 +- 5 files changed, 92 insertions(+), 47 deletions(-) diff --git a/changedetectionio/blueprint/settings/__init__.py b/changedetectionio/blueprint/settings/__init__.py index f6b062187..bff387d5a 100644 --- a/changedetectionio/blueprint/settings/__init__.py +++ b/changedetectionio/blueprint/settings/__init__.py @@ -66,9 +66,6 @@ def construct_blueprint(datastore: ChangeDetectionStore): extra_notification_tokens=datastore.get_unique_notification_tokens_available() ) - # Remove the last option 'System default' - form.application.form.notification_format.choices.pop() - if datastore.proxy_list is None: # @todo - Couldn't get setattr() etc dynamic addition working, so remove it instead del form.requests.form.proxy @@ -95,10 +92,9 @@ def construct_blueprint(datastore: ChangeDetectionStore): if 'password' in app_update and not app_update['password']: del (app_update['password']) - # Notification fields live on the dedicated /settings/notifications page now. - # The application sub-form still defines them (inherited from commonSettingsForm), - # so an unrelated save here would clobber the stored values with empty WTForms - # defaults. Drop them from the merge to leave the notifications config alone. + # Notification fields live on the dedicated /settings/notifications page and are + # no longer part of this form (#4554). Defensive - never let a save here clobber + # the stored notification config should one get re-added to the application sub-form. for nf in ('notification_urls', 'notification_title', 'notification_body', 'notification_format', 'base_url'): app_update.pop(nf, None) diff --git a/changedetectionio/forms.py b/changedetectionio/forms.py index 69f64a817..6a50c0c7f 100644 --- a/changedetectionio/forms.py +++ b/changedetectionio/forms.py @@ -51,6 +51,7 @@ from changedetectionio.browser_steps.browser_steps import browser_step_ui_config from changedetectionio import html_tools, content_fetchers from changedetectionio.notification import ( + USE_SYSTEM_DEFAULT_NOTIFICATION_FORMAT_FOR_WATCH, valid_notification_formats, ) @@ -860,21 +861,37 @@ class quickWatchForm(Form): edit_and_watch_submit_button = SubmitField(_l('Edit > Watch'), render_kw={"class": "pure-button pure-button-primary"}) +# Apprise notification fields, common to a single watch and the global +# /settings/notifications page (NOT the main /settings page, which no longer +# renders them - a hidden field there can fail validation with no visible error, see #4554) +class commonNotificationSettingsForm(Form): + def __init__(self, formdata=None, obj=None, prefix="", data=None, meta=None, **kwargs): + super().__init__(formdata, obj, prefix, data, meta, **kwargs) + extra = kwargs.get('extra_notification_tokens', {}) + self.notification_body.extra_notification_tokens = extra + self.notification_title.extra_notification_tokens = extra + self.notification_urls.extra_notification_tokens = extra + + notification_urls = StringListField(_l('Notification URL List'), + validators=[validators.Optional(), ValidateAppRiseServers(), ValidateJinja2Template()]) + notification_title = StringField(_l('Notification Title'), + default='ChangeDetection.io Notification - {{ watch_url }}', + validators=[validators.Optional(), ValidateJinja2Template()]) + notification_body = TextAreaField(_l('Notification Body'), + default='{{ watch_url }} had a change.', + validators=[validators.Optional(), ValidateJinja2Template()]) + notification_format = SelectField(_l('Notification format'), + choices=list(valid_notification_formats.items())) + + # Common to a single watch and the global settings class commonSettingsForm(Form): from . import processors def __init__(self, formdata=None, obj=None, prefix="", data=None, meta=None, **kwargs): super().__init__(formdata, obj, prefix, data, meta, **kwargs) - self.notification_body.extra_notification_tokens = kwargs.get('extra_notification_tokens', {}) - self.notification_title.extra_notification_tokens = kwargs.get('extra_notification_tokens', {}) - self.notification_urls.extra_notification_tokens = kwargs.get('extra_notification_tokens', {}) fetch_backend = RadioField(_l('Fetch Method'), choices=content_fetchers.available_fetchers(), validators=[ValidateContentFetcherIsReady()]) - notification_body = TextAreaField(_l('Notification Body'), default='{{ watch_url }} had a change.', validators=[validators.Optional(), ValidateJinja2Template()]) - notification_format = SelectField(_l('Notification format'), choices=list(valid_notification_formats.items())) - notification_title = StringField(_l('Notification Title'), default='ChangeDetection.io Notification - {{ watch_url }}', validators=[validators.Optional(), ValidateJinja2Template()]) - notification_urls = StringListField(_l('Notification URL List'), validators=[validators.Optional(), ValidateAppRiseServers(), ValidateJinja2Template()]) processor = RadioField( label=_l("Processor - What do you want to achieve?"), choices=lambda: processors.available_processors(), default=processors.get_default_processor) scheduler_timezone_default = StringField(_l("Default timezone for watch check scheduler"), render_kw={"list": "timezones"}, validators=[validateTimeZoneName()]) webdriver_delay = IntegerField(_l('Wait seconds before extracting text'), validators=[validators.Optional(), validators.NumberRange(min=1, message=_l("Should contain one or more seconds"))]) @@ -900,24 +917,11 @@ class commonSettingsForm(Form): # # Named for the backend (apprise) on purpose: future backends (simple_email, # webhook, etc.) will land as their own forms next to this one. -class globalSettingsAppriseNotificationForm(Form): - def __init__(self, formdata=None, obj=None, prefix="", data=None, meta=None, **kwargs): - super().__init__(formdata, obj, prefix, data, meta, **kwargs) - extra = kwargs.get('extra_notification_tokens', {}) - self.notification_body.extra_notification_tokens = extra - self.notification_title.extra_notification_tokens = extra - self.notification_urls.extra_notification_tokens = extra - - notification_urls = StringListField(_l('Notification URL List'), - validators=[validators.Optional(), ValidateAppRiseServers(), ValidateJinja2Template()]) - notification_title = StringField(_l('Notification Title'), - default='ChangeDetection.io Notification - {{ watch_url }}', - validators=[validators.Optional(), ValidateJinja2Template()]) - notification_body = TextAreaField(_l('Notification Body'), - default='{{ watch_url }} had a change.', - validators=[validators.Optional(), ValidateJinja2Template()]) +class globalSettingsAppriseNotificationForm(commonNotificationSettingsForm): + # 'System default' only makes sense per-watch (it means "use this global value") notification_format = SelectField(_l('Notification format'), - choices=list(valid_notification_formats.items())) + choices=[(k, v) for k, v in valid_notification_formats.items() + if k != USE_SYSTEM_DEFAULT_NOTIFICATION_FORMAT_FOR_WATCH]) base_url = StringField(_l('Notification base URL override'), validators=[validators.Optional()], render_kw={"placeholder": os.getenv('BASE_URL', _l('Not set'))}) @@ -941,7 +945,7 @@ class SingleBrowserStep(Form): # remove_button = SubmitField(_l('-'), render_kw={"type": "button", "class": "pure-button pure-button-primary", 'title': 'Remove'}) # add_button = SubmitField(_l('+'), render_kw={"type": "button", "class": "pure-button pure-button-primary", 'title': 'Add new step after'}) -class processor_text_json_diff_form(commonSettingsForm): +class processor_text_json_diff_form(commonSettingsForm, commonNotificationSettingsForm): url = StringField(_l('Web Page URL'), validators=[validateURL()]) link_to_open = StringField(_l('Open Link Override'), validators=[validators.Optional(), validateURL()], default='') @@ -1192,10 +1196,6 @@ class globalSettingsApplicationUIForm(Form): class globalSettingsApplicationForm(commonSettingsForm): api_access_token_enabled = BooleanField(_l('API access token security check enabled'), default=True, validators=[validators.Optional()]) - base_url = StringField(_l('Notification base URL override'), - validators=[validators.Optional()], - render_kw={"placeholder": os.getenv('BASE_URL', _l('Not set'))} - ) empty_pages_are_a_change = BooleanField(_l('Treat empty pages as a change?'), default=False) fetch_backend = RadioField(_l('Fetch Method'), default="html_requests", choices=content_fetchers.available_fetchers(), validators=[ValidateContentFetcherIsReady()]) global_ignore_text = StringListField(_l('Ignore Text'), [ValidateListRegex()]) @@ -1417,9 +1417,6 @@ class globalSettingsForm(Form): # datastore.data['settings']['requests'].. def __init__(self, formdata=None, obj=None, prefix="", data=None, meta=None, **kwargs): super().__init__(formdata, obj, prefix, data, meta, **kwargs) - self.application.notification_body.extra_notification_tokens = kwargs.get('extra_notification_tokens', {}) - self.application.notification_title.extra_notification_tokens = kwargs.get('extra_notification_tokens', {}) - self.application.notification_urls.extra_notification_tokens = kwargs.get('extra_notification_tokens', {}) requests = FormField(globalSettingsRequestForm) application = FormField(globalSettingsApplicationForm) diff --git a/changedetectionio/tests/test_settings_notifications_page.py b/changedetectionio/tests/test_settings_notifications_page.py index bccbbf320..1ad8dd8d1 100644 --- a/changedetectionio/tests/test_settings_notifications_page.py +++ b/changedetectionio/tests/test_settings_notifications_page.py @@ -118,10 +118,8 @@ def test_main_settings_save_does_not_clobber_notification_config( """ Regression — the main /settings page and the new /settings/notifications page write to the SAME dict (datastore.data['settings']['application']). The main - page's application sub-form still inherits the notification_* fields from - commonSettingsForm, so a naive `app.update(form.data['application'])` here - would wipe notification_urls / title / body / format / base_url with empty - WTForms defaults whenever the user saves the main settings page. + page must never write notification_urls / title / body / format / base_url, + otherwise saving it would wipe them with empty WTForms defaults. This test seeds a real notification config, then POSTs the main settings page WITHOUT touching the notification fields, and asserts every notification @@ -139,7 +137,6 @@ def test_main_settings_save_does_not_clobber_notification_config( 'application-fetch_backend': 'html_requests', 'application-rss_diff_length': '5', 'application-filter_failure_notification_threshold_attempts': '0', - 'application-notification_format': 'html', # required choice, no Optional() 'requests-time_between_check-days': '0', 'requests-time_between_check-hours': '0', 'requests-time_between_check-minutes': '5', @@ -225,3 +222,58 @@ def test_notifications_page_save_does_not_clobber_other_settings( # And the notification field we DID submit was actually written. assert ds.data['settings']['application'].get('notification_urls') == \ ['json://example.invalid/only-this'] + + +def test_main_settings_save_with_global_system_default_format( + client, live_server, measure_memory_usage, datastore_path): + """ + Regression for #4554 - a global notification_format of 'System default' made + every save of the main /settings page fail with "An error occurred" and no + visible field error, because the main form still carried a hidden + notification_format SelectField whose choices excluded that value. + """ + from changedetectionio.model import USE_SYSTEM_DEFAULT_NOTIFICATION_FORMAT_FOR_WATCH + + ds = client.application.config.get('DATASTORE') + ds.data['settings']['application']['notification_format'] = USE_SYSTEM_DEFAULT_NOTIFICATION_FORMAT_FOR_WATCH + + res = client.post( + url_for('settings.settings_page'), + data={ + 'application-pager_size': '50', + 'application-fetch_backend': 'html_requests', + 'application-rss_diff_length': '5', + 'application-filter_failure_notification_threshold_attempts': '0', + 'requests-time_between_check-days': '0', + 'requests-time_between_check-hours': '0', + 'requests-time_between_check-minutes': '5', + 'requests-time_between_check-seconds': '0', + 'requests-time_between_check-weeks': '0', + 'requests-jitter_seconds': '0', + 'requests-workers': '10', + 'requests-timeout': '60', + }, + follow_redirects=True, + ) + assert res.status_code == 200 + assert b'Settings updated.' in res.data + assert b'An error occurred' not in res.data + + +def test_notifications_page_rejects_system_default_format( + client, live_server, measure_memory_usage, datastore_path): + """'System default' is a per-watch choice meaning "use the global value", + it must not be offered or accepted as the global value itself (#4554).""" + from changedetectionio.model import USE_SYSTEM_DEFAULT_NOTIFICATION_FORMAT_FOR_WATCH + + res = client.get(url_for('settings.notifications.apprise')) + assert f'value="{USE_SYSTEM_DEFAULT_NOTIFICATION_FORMAT_FOR_WATCH}"'.encode() not in res.data + + res = client.post( + url_for('settings.notifications.apprise'), + data={'notification_format': USE_SYSTEM_DEFAULT_NOTIFICATION_FORMAT_FOR_WATCH}, + follow_redirects=True, + ) + assert b'Settings updated.' not in res.data + ds = client.application.config.get('DATASTORE') + assert ds.data['settings']['application'].get('notification_format') != USE_SYSTEM_DEFAULT_NOTIFICATION_FORMAT_FOR_WATCH diff --git a/changedetectionio/widgets/ternary_boolean.py b/changedetectionio/widgets/ternary_boolean.py index 54bb49696..cbe32bd4c 100644 --- a/changedetectionio/widgets/ternary_boolean.py +++ b/changedetectionio/widgets/ternary_boolean.py @@ -59,7 +59,7 @@ class TernaryNoneBooleanField(Field): Custom text can be provided for each option: - yes_text: Text for True option (default: "Yes") - no_text: Text for False option (default: "No") - - none_text: Text for None option (default: "Default") + - none_text: Text for None option (default: "Main settings") """ widget = TernaryNoneBooleanWidget() diff --git a/changedetectionio/widgets/test_custom_text.py b/changedetectionio/widgets/test_custom_text.py index 5dad06ae8..887173502 100644 --- a/changedetectionio/widgets/test_custom_text.py +++ b/changedetectionio/widgets/test_custom_text.py @@ -63,8 +63,8 @@ def test_custom_text(): default_html = default_field.widget(default_field) print(f"Contains 'Yes': {'Yes' in default_html}") print(f"Contains 'No': {'No' in default_html}") - print(f"Contains 'Default': {'Default' in default_html}") - assert 'Yes' in default_html and 'No' in default_html and 'Default' in default_html + print(f"Contains 'Main settings': {'Main settings' in default_html}") + assert 'Yes' in default_html and 'No' in default_html and 'Main settings' in default_html # Test custom text field print("\n--- Custom Text Field with Emojis ---")