From 2a6158c9f707cee41a12a4b1f0bdbd363da696c9 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Fri, 24 Jul 2026 14:49:44 -0700 Subject: [PATCH 01/18] correct doc string typo --- tom_dataservices/dataservices.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tom_dataservices/dataservices.py b/tom_dataservices/dataservices.py index eca37b705..57673e7c1 100644 --- a/tom_dataservices/dataservices.py +++ b/tom_dataservices/dataservices.py @@ -24,7 +24,7 @@ def get_data_service_classes(): """ Imports the Dataservice class from relevant apps and generates a list of data service names. - Each dataservice class should be contained in a list of dictionaries in an app's apps.py `dataservices` method. + Each dataservice class should be contained in a list of dictionaries in an app's apps.py `data_services` method. Each dataservice dictionary should contain a 'class' key with the dot separated path to the dataservice class (typically an extension of DataService). From f0f432600c936b775a3137897fc045ab761e2fe9 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Fri, 24 Jul 2026 14:53:52 -0700 Subject: [PATCH 02/18] explain in settings.py tmpl that INSTALLED_APPS can add facilities with the new AppConfig.observation_facilities() integration point. So, facility.get_service_classes collects facilities from the new integration point and `settings.TOM_FACILITY_CLASSES` combined. --- tom_setup/templates/tom_setup/settings.tmpl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tom_setup/templates/tom_setup/settings.tmpl b/tom_setup/templates/tom_setup/settings.tmpl index 9a98a0ec7..d97f35ace 100644 --- a/tom_setup/templates/tom_setup/settings.tmpl +++ b/tom_setup/templates/tom_setup/settings.tmpl @@ -237,6 +237,9 @@ DATA_PROCESSORS = { 'spectroscopy': 'tom_dataproducts.processors.spectroscopy_processor.SpectroscopyProcessor', } +# Facilities listed here are available to your TOM. In addition, any app in INSTALLED_APPS +# that implements the observation_facilities() AppConfig integration point (e.g. tom_keck, +# tom_cfht) contributes its facilities automatically and does not need to be listed here. TOM_FACILITY_CLASSES = [ 'tom_observations.facilities.lco_redirect.LCORedirectFacility', 'tom_observations.facilities.gemini.GEMFacility', From 2058c8614a815cb746304aae9c9fb8d723455e92 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Fri, 24 Jul 2026 14:57:10 -0700 Subject: [PATCH 03/18] add Facilities nav-bar item via TomObaservations integration point --- tom_observations/apps.py | 12 +++++ .../partials/navbar_facilities_list.html | 15 ++++++ .../templatetags/observation_extras.py | 54 ++++++++++++++++++- 3 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 tom_observations/templates/tom_observations/partials/navbar_facilities_list.html diff --git a/tom_observations/apps.py b/tom_observations/apps.py index a4c446f7f..6bbc6ae0f 100644 --- a/tom_observations/apps.py +++ b/tom_observations/apps.py @@ -3,3 +3,15 @@ class TomObservationsConfig(AppConfig): name = 'tom_observations' + + def nav_items(self): + """ + Integration point for adding items to the navbar. + This method should return a list of partial templates to be included in the navbar. + + Here, the "Facilities" dropdown menu, listing the facilities contributed by installed + apps via the observation_facilities() integration point (see + tom_observations.facility.get_service_classes()). + """ + return [{'partial': 'tom_observations/partials/navbar_facilities_list.html', + 'context': 'tom_observations.templatetags.observation_extras.observation_facilities_list'}] diff --git a/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html b/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html new file mode 100644 index 000000000..98944740e --- /dev/null +++ b/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html @@ -0,0 +1,15 @@ +{# "Facilities" navbar dropdown: one menu item per facility contributed via the #} +{# observation_facilities() AppConfig integration point. Context comes from #} +{# observation_extras.observation_facilities_list. #} +{% if observation_facilities %} +
  • + Facilities + +
    • +{% endif %} diff --git a/tom_observations/templatetags/observation_extras.py b/tom_observations/templatetags/observation_extras.py index ec09b24f4..12888545e 100644 --- a/tom_observations/templatetags/observation_extras.py +++ b/tom_observations/templatetags/observation_extras.py @@ -1,9 +1,12 @@ from datetime import datetime, timedelta +import logging from urllib.parse import urlencode from django import forms, template +from django.apps import apps from django.conf import settings -from django.urls import reverse +from django.urls import NoReverseMatch, reverse +from django.utils.module_loading import import_string from guardian.shortcuts import get_objects_for_user from plotly import offline import plotly.graph_objs as go @@ -16,9 +19,58 @@ from tom_targets.models import Target +logger = logging.getLogger(__name__) + register = template.Library() +@register.inclusion_tag('tom_observations/partials/navbar_facilities_list.html', takes_context=True) +def observation_facilities_list(context: dict) -> dict: + """ + Returns the list of app-contributed observation facilities used to generate the + "Facilities" navbar dropdown links. + + Facilities are gathered from installed apps implementing the observation_facilities() + AppConfig integration point (see tom_observations.facility.get_service_classes()). + In addition to the 'class' key consumed by get_service_classes(), each facility + dictionary may include an optional 'url' key: the namespaced URL name of that + facility's landing page, to which its navbar menu item will link. A facility without + a 'url' is registration-only: it gets no navbar menu item (and if no facility + supplies a 'url', the "Facilities" dropdown is not displayed at all). + """ + facilities = [] + for app in apps.get_app_configs(): + try: + observation_facilities = app.observation_facilities() + except AttributeError: + continue # this app doesn't implement the integration point + for facility in observation_facilities: + # a facility without a 'url' key declares no landing page; it is registered by + # get_service_classes() but deliberately gets no navbar menu item + url_name = facility.get('url') + if url_name is None: + continue + # resolve the facility class for its display name, and the landing page URL + # for the link target; skip (with a warning) anything that doesn't resolve, + # so one bad entry doesn't take out the whole navbar + try: + clazz = import_string(facility['class']) + except ImportError as e: + logger.warning(f'WARNING: Could not import facility class for {app.name} from ' + f'{facility["class"]}: {e}') + continue + try: + url = reverse(url_name) + except NoReverseMatch as e: + logger.warning(f'WARNING: Could not resolve landing page URL for facility {clazz.name} ' + f'of {app.name}: {e}') + continue + facilities.append({'name': clazz.name, 'url': url}) + + context['observation_facilities'] = facilities + return context + + @register.inclusion_tag('tom_observations/partials/update_status_button.html', takes_context=True) def update_status_button(context): """ From 2e86f2be954e76f8678a6ca1dd4ac53555e1ba94 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Fri, 24 Jul 2026 14:58:52 -0700 Subject: [PATCH 04/18] collect facilities from both settings and installed apps --- tom_observations/facility.py | 46 ++++++++++++- tom_observations/tests/tests.py | 112 +++++++++++++++++++++++++++++++- 2 files changed, 156 insertions(+), 2 deletions(-) diff --git a/tom_observations/facility.py b/tom_observations/facility.py index 8b878dfa4..1a4f14004 100644 --- a/tom_observations/facility.py +++ b/tom_observations/facility.py @@ -7,6 +7,7 @@ from crispy_forms.helper import FormHelper from crispy_forms.layout import ButtonHolder, Layout, Submit, Div, HTML from django import forms +from django.apps import apps from django.conf import settings from django.contrib.auth.models import Group from django.core.exceptions import ImproperlyConfigured @@ -48,7 +49,32 @@ class CredentialStatus(Enum): AUTO_THUMBNAILS = False -def get_service_classes(): +def get_service_classes() -> dict: + """Return a dictionary mapping facility name to facility class for all known facilities. + + Facilities come from two sources, combined here: + 1. ``settings.TOM_FACILITY_CLASSES`` (falling back to ``DEFAULT_FACILITY_CLASSES``), the + traditional explicit configuration mechanism. + 2. The ``observation_facilities()`` AppConfig integration point: any INSTALLED_APP whose + AppConfig defines an ``observation_facilities()`` method contributes its facilities + automatically, with no settings changes required. (This is analogous to the + ``data_services()`` integration point consumed by + ``tom_dataservices.dataservices.get_data_service_classes()``.) + + ``observation_facilities()`` should return a list of dictionaries, each with a ``class`` key + whose value is the dot-separated path to the facility class. Entries may carry additional + keys for other consumers of the integration point — e.g. an optional ``url`` key naming the + facility's landing page for the navbar "Facilities" menu (see + ``tom_observations.templatetags.observation_extras.observation_facilities_list``) — but only + ``class`` is consumed here. + + FOR EXAMPLE: + [{'class': 'tom_keck.keck.KeckFacility'}] + + Returns: + dict: mapping of ``Facility.name`` to facility class. A facility appearing in both + sources (same ``name``) is only included once; the app-supplied class wins. + """ try: TOM_FACILITY_CLASSES = settings.TOM_FACILITY_CLASSES except AttributeError: @@ -61,6 +87,24 @@ def get_service_classes(): except (ImportError, AttributeError) as e: raise ImportError(f'Could not import {service}: {e}') service_choices[clazz.name] = clazz + + # Combine the settings-declared facilities with those contributed by installed apps + # via the observation_facilities() AppConfig integration point. + for app in apps.get_app_configs(): + try: + observation_facilities = app.observation_facilities() + except AttributeError: + continue # this app doesn't implement the integration point + for facility in observation_facilities: + try: + clazz = import_string(facility['class']) + except ImportError as e: + logger.warning(f'WARNING: Could not import facility class for {app.name} from ' + f'{facility["class"]}.\n' + f'{e}') + continue + service_choices[clazz.name] = clazz + return service_choices diff --git a/tom_observations/tests/tests.py b/tom_observations/tests/tests.py index 8db26478c..dc1c7aa6f 100644 --- a/tom_observations/tests/tests.py +++ b/tom_observations/tests/tests.py @@ -6,6 +6,7 @@ from django.contrib.auth.models import User from django.contrib.messages import get_messages from django.forms import ValidationError +from django.template.loader import render_to_string from django.test import TestCase, override_settings from django.urls import reverse from django.utils import timezone @@ -15,8 +16,10 @@ from astropy.time import Time from .factories import ObservingRecordFactory, ObservationTemplateFactory, SiderealTargetFactory, TargetNameFactory +from tom_observations.facility import get_service_classes +from tom_observations.templatetags.observation_extras import observation_facilities_list from tom_observations.utils import get_astroplan_sun_and_time, get_sidereal_visibility -from tom_observations.tests.utils import FakeRoboticFacility +from tom_observations.tests.utils import FakeManualFacility, FakeRoboticFacility from tom_observations.models import ObservationRecord, ObservationGroup, ObservationTemplate from tom_targets.models import Target from guardian.shortcuts import assign_perm @@ -497,3 +500,110 @@ def test_get_visibility_sidereal(self, mock_facility): self.assertEqual(len(airmass_data), len(expected_airmass)) for i, expected_airmass_value in enumerate(expected_airmass): self.assertAlmostEqual(airmass_data[i], expected_airmass_value, places=3) + + +class TestGetServiceClasses(TestCase): + """ + Tests for the observation_facilities() AppConfig integration point as consumed by + tom_observations.facility.get_service_classes(). + """ + + def _fake_app_config(self, facilities: list) -> mock.Mock: + """Return a mock AppConfig whose observation_facilities() returns the given list.""" + app_config = mock.Mock() + app_config.name = 'fake_facility_app' + app_config.observation_facilities.return_value = facilities + return app_config + + @override_settings(TOM_FACILITY_CLASSES=['tom_observations.tests.utils.FakeRoboticFacility']) + def test_app_contributed_facilities_merged_with_settings(self): + fake_app_config = self._fake_app_config([{'class': 'tom_observations.tests.utils.FakeManualFacility'}]) + with mock.patch('tom_observations.facility.apps.get_app_configs', return_value=[fake_app_config]): + service_classes = get_service_classes() + self.assertEqual(service_classes, + {'FakeRoboticFacility': FakeRoboticFacility, 'FakeManualFacility': FakeManualFacility}) + + @override_settings(TOM_FACILITY_CLASSES=['tom_observations.tests.utils.FakeRoboticFacility']) + def test_apps_without_integration_point_are_skipped(self): + # mock.Mock(spec=[]) raises AttributeError for observation_facilities(), behaving + # like a regular AppConfig that doesn't implement the integration point + with mock.patch('tom_observations.facility.apps.get_app_configs', return_value=[mock.Mock(spec=[])]): + service_classes = get_service_classes() + self.assertEqual(service_classes, {'FakeRoboticFacility': FakeRoboticFacility}) + + @override_settings(TOM_FACILITY_CLASSES=['tom_observations.tests.utils.FakeRoboticFacility']) + def test_facility_in_both_sources_is_deduplicated(self): + fake_app_config = self._fake_app_config([{'class': 'tom_observations.tests.utils.FakeRoboticFacility'}]) + with mock.patch('tom_observations.facility.apps.get_app_configs', return_value=[fake_app_config]): + service_classes = get_service_classes() + self.assertEqual(service_classes, {'FakeRoboticFacility': FakeRoboticFacility}) + + @override_settings(TOM_FACILITY_CLASSES=['tom_observations.tests.utils.FakeRoboticFacility']) + def test_unimportable_app_facility_is_skipped_with_warning(self): + fake_app_config = self._fake_app_config([{'class': 'no.such.module.NoSuchFacility'}]) + with mock.patch('tom_observations.facility.apps.get_app_configs', return_value=[fake_app_config]), \ + self.assertLogs('tom_observations.facility', level='WARNING'): + service_classes = get_service_classes() + self.assertEqual(service_classes, {'FakeRoboticFacility': FakeRoboticFacility}) + + +class TestObservationFacilitiesNavbar(TestCase): + """ + Tests for the "Facilities" navbar dropdown: the observation_facilities_list templatetag + and its navbar_facilities_list.html partial. + """ + + def _fake_app_config(self, facilities: list) -> mock.Mock: + """Return a mock AppConfig whose observation_facilities() returns the given list.""" + app_config = mock.Mock() + app_config.name = 'fake_facility_app' + app_config.observation_facilities.return_value = facilities + return app_config + + def test_responding_app_facility_is_listed(self): + # 'home' is a URL name that always resolves, standing in for a facility landing page + fake_app_config = self._fake_app_config( + [{'class': 'tom_observations.tests.utils.FakeRoboticFacility', 'url': 'home'}]) + with mock.patch('tom_observations.templatetags.observation_extras.apps.get_app_configs', + return_value=[fake_app_config]): + context = observation_facilities_list({}) + self.assertEqual(context['observation_facilities'], + [{'name': 'FakeRoboticFacility', 'url': reverse('home')}]) + + def test_no_responding_apps_yields_empty_context(self): + with mock.patch('tom_observations.templatetags.observation_extras.apps.get_app_configs', + return_value=[mock.Mock(spec=[])]): + context = observation_facilities_list({}) + self.assertEqual(context['observation_facilities'], []) + + def test_dropdown_hidden_when_no_facilities(self): + html = render_to_string('tom_observations/partials/navbar_facilities_list.html', + {'observation_facilities': []}) + self.assertNotIn('Facilities', html) + + def test_facility_with_unresolvable_url_is_skipped_with_warning(self): + fake_app_config = self._fake_app_config( + [{'class': 'tom_observations.tests.utils.FakeRoboticFacility', 'url': 'no-such-url-name'}]) + with mock.patch('tom_observations.templatetags.observation_extras.apps.get_app_configs', + return_value=[fake_app_config]), \ + self.assertLogs('tom_observations.templatetags.observation_extras', level='WARNING'): + context = observation_facilities_list({}) + self.assertEqual(context['observation_facilities'], []) + + def test_facility_without_url_gets_no_navbar_item_and_no_warning(self): + # omitting 'url' declares a facility with no landing page (e.g. tom_lt): it is + # registered by get_service_classes() but deliberately absent from the navbar, + # and that absence is not a misconfiguration worth warning about + fake_app_config = self._fake_app_config([{'class': 'tom_observations.tests.utils.FakeRoboticFacility'}]) + with mock.patch('tom_observations.templatetags.observation_extras.apps.get_app_configs', + return_value=[fake_app_config]), \ + mock.patch('tom_observations.templatetags.observation_extras.logger') as mock_logger: + context = observation_facilities_list({}) + self.assertEqual(context['observation_facilities'], []) + mock_logger.warning.assert_not_called() + + def test_index_page_has_no_facilities_dropdown(self): + # tom_base's own test project has no app implementing observation_facilities(), + # so the rendered navbar should not contain the Facilities dropdown at all + response = self.client.get(reverse('home')) + self.assertNotContains(response, '>Facilities<') From 4bce18a7aaab6199f154b6f09ea7a4fb830f6283 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Mon, 3 Aug 2026 16:22:15 -0700 Subject: [PATCH 05/18] simplify comment --- tom_setup/templates/tom_setup/settings.tmpl | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tom_setup/templates/tom_setup/settings.tmpl b/tom_setup/templates/tom_setup/settings.tmpl index d97f35ace..ae30eb60c 100644 --- a/tom_setup/templates/tom_setup/settings.tmpl +++ b/tom_setup/templates/tom_setup/settings.tmpl @@ -237,9 +237,8 @@ DATA_PROCESSORS = { 'spectroscopy': 'tom_dataproducts.processors.spectroscopy_processor.SpectroscopyProcessor', } -# Facilities listed here are available to your TOM. In addition, any app in INSTALLED_APPS -# that implements the observation_facilities() AppConfig integration point (e.g. tom_keck, -# tom_cfht) contributes its facilities automatically and does not need to be listed here. +# Facilities listed appear on your TOM's TargetDetail page. INSTALLED_APPS that implement the +# observation_facilities() AppConfig integration point do not need to be listed here. TOM_FACILITY_CLASSES = [ 'tom_observations.facilities.lco_redirect.LCORedirectFacility', 'tom_observations.facilities.gemini.GEMFacility', From dd5e8104791973731a1940faf569c93061732dae Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Mon, 3 Aug 2026 16:39:15 -0700 Subject: [PATCH 06/18] make index_url_name a Facility class attribute this is better than having it be an integration point dictionary item --- tom_observations/facility.py | 25 ++------ .../templatetags/observation_extras.py | 64 +++++++------------ 2 files changed, 29 insertions(+), 60 deletions(-) diff --git a/tom_observations/facility.py b/tom_observations/facility.py index 1a4f14004..9cdedc6be 100644 --- a/tom_observations/facility.py +++ b/tom_observations/facility.py @@ -53,27 +53,11 @@ def get_service_classes() -> dict: """Return a dictionary mapping facility name to facility class for all known facilities. Facilities come from two sources, combined here: - 1. ``settings.TOM_FACILITY_CLASSES`` (falling back to ``DEFAULT_FACILITY_CLASSES``), the - traditional explicit configuration mechanism. - 2. The ``observation_facilities()`` AppConfig integration point: any INSTALLED_APP whose - AppConfig defines an ``observation_facilities()`` method contributes its facilities - automatically, with no settings changes required. (This is analogous to the - ``data_services()`` integration point consumed by - ``tom_dataservices.dataservices.get_data_service_classes()``.) - - ``observation_facilities()`` should return a list of dictionaries, each with a ``class`` key - whose value is the dot-separated path to the facility class. Entries may carry additional - keys for other consumers of the integration point — e.g. an optional ``url`` key naming the - facility's landing page for the navbar "Facilities" menu (see - ``tom_observations.templatetags.observation_extras.observation_facilities_list``) — but only - ``class`` is consumed here. - - FOR EXAMPLE: - [{'class': 'tom_keck.keck.KeckFacility'}] + 1. ``settings.TOM_FACILITY_CLASSES`` + 2. ``observation_facilities()`` AppConfig integration point (see ``tom_demoapp`` for example). Returns: - dict: mapping of ``Facility.name`` to facility class. A facility appearing in both - sources (same ``name``) is only included once; the app-supplied class wins. + dict: {facility_name: FacilityClass} """ try: TOM_FACILITY_CLASSES = settings.TOM_FACILITY_CLASSES @@ -268,6 +252,9 @@ class BaseObservationFacility(ABC): is_redirect = False button_label = "" button_tooltip = "" + #: Namespaced URL name of this facility's index page. + #: None means no index page and no Facilities nav-bar menu item. + index_url_name: str | None = None def __init__(self): self.user = None diff --git a/tom_observations/templatetags/observation_extras.py b/tom_observations/templatetags/observation_extras.py index 12888545e..8a3a4ca63 100644 --- a/tom_observations/templatetags/observation_extras.py +++ b/tom_observations/templatetags/observation_extras.py @@ -3,10 +3,8 @@ from urllib.parse import urlencode from django import forms, template -from django.apps import apps from django.conf import settings from django.urls import NoReverseMatch, reverse -from django.utils.module_loading import import_string from guardian.shortcuts import get_objects_for_user from plotly import offline import plotly.graph_objs as go @@ -27,47 +25,31 @@ @register.inclusion_tag('tom_observations/partials/navbar_facilities_list.html', takes_context=True) def observation_facilities_list(context: dict) -> dict: """ - Returns the list of app-contributed observation facilities used to generate the - "Facilities" navbar dropdown links. - - Facilities are gathered from installed apps implementing the observation_facilities() - AppConfig integration point (see tom_observations.facility.get_service_classes()). - In addition to the 'class' key consumed by get_service_classes(), each facility - dictionary may include an optional 'url' key: the namespaced URL name of that - facility's landing page, to which its navbar menu item will link. A facility without - a 'url' is registration-only: it gets no navbar menu item (and if no facility - supplies a 'url', the "Facilities" dropdown is not displayed at all). + Returns the facilities linked from the "Facilities" navbar dropdown. + + A facility from ``get_service_classes()`` is listed when its class sets ``index_url_name``. """ - facilities = [] - for app in apps.get_app_configs(): + navbar_facilities = [] + for facility_class in get_service_classes().values(): + index_url_name = getattr(facility_class, 'index_url_name', None) + if not index_url_name: + continue # registration-only facility: no index page, so no menu item + + # make sure we can reverse the URL try: - observation_facilities = app.observation_facilities() - except AttributeError: - continue # this app doesn't implement the integration point - for facility in observation_facilities: - # a facility without a 'url' key declares no landing page; it is registered by - # get_service_classes() but deliberately gets no navbar menu item - url_name = facility.get('url') - if url_name is None: - continue - # resolve the facility class for its display name, and the landing page URL - # for the link target; skip (with a warning) anything that doesn't resolve, - # so one bad entry doesn't take out the whole navbar - try: - clazz = import_string(facility['class']) - except ImportError as e: - logger.warning(f'WARNING: Could not import facility class for {app.name} from ' - f'{facility["class"]}: {e}') - continue - try: - url = reverse(url_name) - except NoReverseMatch as e: - logger.warning(f'WARNING: Could not resolve landing page URL for facility {clazz.name} ' - f'of {app.name}: {e}') - continue - facilities.append({'name': clazz.name, 'url': url}) - - context['observation_facilities'] = facilities + url = reverse(index_url_name) + except NoReverseMatch as e: + logger.warning(f'WARNING: Could not resolve index page URL for facility ' + f'{facility_class.name}: {e}') + continue + navbar_facilities.append( + { + "name": facility_class.name, + "url": url, + } + ) + + context['observation_facilities'] = navbar_facilities return context From e5e5d5f4fc76e1a31f86400f7e0e7b8328a4cf50 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Mon, 3 Aug 2026 16:40:37 -0700 Subject: [PATCH 07/18] update doc for observation_facilities integration point --- docs/common/customsettings.rst | 3 ++ docs/observing/observation_module.rst | 40 +++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/docs/common/customsettings.rst b/docs/common/customsettings.rst index 912024159..733c0d667 100644 --- a/docs/common/customsettings.rst +++ b/docs/common/customsettings.rst @@ -181,6 +181,9 @@ A list of observation facility classes to make available to your TOM. If you have written or downloaded a custom observation facility you would add the class to this list to make your TOM load it. +INSTALLED_APPS that implement the ``observation_facilities()`` AppConfig integration +point do not need to be listed here. + `TOM_LATEX_PROCESSORS <#tom-latex-processors>`__ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/observing/observation_module.rst b/docs/observing/observation_module.rst index 85b3fcf15..433f59d26 100644 --- a/docs/observing/observation_module.rst +++ b/docs/observing/observation_module.rst @@ -105,6 +105,25 @@ like this: This means our new observation facility module has been successfully loaded. +Adding a facility from an app +----------------------------- + +A reusable app can contribute its facilities without the TOM editing ``settings.py``. +Implement ``observation_facilities()`` on the app's ``AppConfig``: + +.. code:: python + + class MyAppConfig(AppConfig): + name = 'myapp' + + def observation_facilities(self): + return [{'class': f'{self.name}.myfacility.MyObservationFacility'}] + +Facilities from both routes are merged by +``tom_observations.facility.get_service_classes()``, so adding the app to +``INSTALLED_APPS`` is all that is required of the TOM. See ``tom_demoapp`` for a +worked example. + BaseRoboticObservationFacility and BaseRoboticObservationForm ------------------------------------------------------------- @@ -126,6 +145,27 @@ use. The ``BaseRoboticObservationForm`` class, just like the previous super class, contains logic and layout that all observation facility form classes should contain. +Linking to a facility index page +-------------------------------- + +A facility may set ``index_url_name`` to the namespaced Django URL name of a page +describing the facility. Facilities that set it appear in the navbar **Facilities** +dropdown, linked to that page: + +.. code:: python + + class MyObservationFacility(BaseRoboticObservationFacility): + name = 'MyFacility' + index_url_name = 'myapp:facility-index' + +``index_url_name`` is optional and is omitted from the minimal example above. A facility +that leaves it unset is still fully registered -- it has an observe button and observation +forms -- but gets no menu item. If no facility sets it, the dropdown is not displayed. + +The namespace is the one the facility's URLs are deployed under. For an app, that is the +``namespace`` argument its ``include_url_paths()`` passes to ``include()``, which is not +necessarily the app's name: ``tom_demoapp`` is deployed under ``demoapp``. + Implementing observation submission ----------------------------------- From 9cb13994c01785970684510ae098e9a5e80357c0 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Mon, 3 Aug 2026 16:41:08 -0700 Subject: [PATCH 08/18] improve docstring with tom_demoapp reference --- tom_observations/apps.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tom_observations/apps.py b/tom_observations/apps.py index 6bbc6ae0f..7b3934e24 100644 --- a/tom_observations/apps.py +++ b/tom_observations/apps.py @@ -10,8 +10,7 @@ def nav_items(self): This method should return a list of partial templates to be included in the navbar. Here, the "Facilities" dropdown menu, listing the facilities contributed by installed - apps via the observation_facilities() integration point (see - tom_observations.facility.get_service_classes()). + apps via the observation_facilities() integration point (see ``tom_demoapp`` for example). """ return [{'partial': 'tom_observations/partials/navbar_facilities_list.html', 'context': 'tom_observations.templatetags.observation_extras.observation_facilities_list'}] From 0ad385f87ecf08bf0832ea72a18cff2fe3214962 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Mon, 3 Aug 2026 17:47:12 -0700 Subject: [PATCH 09/18] update for index_url_name class attribute (not int point config) --- .../tom_observations/partials/navbar_facilities_list.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html b/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html index 98944740e..283d44617 100644 --- a/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html +++ b/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html @@ -1,6 +1,6 @@ -{# "Facilities" navbar dropdown: one menu item per facility contributed via the #} -{# observation_facilities() AppConfig integration point. Context comes from #} -{# observation_extras.observation_facilities_list. #} +{# "Facilities" navbar dropdown: one menu item per facility whose class sets #} +{# index_url_name (see BaseObservationFacility). Context comes from #} +{# observation_extras.observation_facilities_list. #} {% if observation_facilities %}
  • Facilities From ee70d918fa8539ca5f7d16e6aa0fb40c5d7de14a Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Mon, 3 Aug 2026 17:48:24 -0700 Subject: [PATCH 10/18] clarify what is being tested and why --- tom_observations/tests/tests.py | 115 +++++++++++++++++++------------- 1 file changed, 69 insertions(+), 46 deletions(-) diff --git a/tom_observations/tests/tests.py b/tom_observations/tests/tests.py index dc1c7aa6f..757973af5 100644 --- a/tom_observations/tests/tests.py +++ b/tom_observations/tests/tests.py @@ -523,14 +523,6 @@ def test_app_contributed_facilities_merged_with_settings(self): self.assertEqual(service_classes, {'FakeRoboticFacility': FakeRoboticFacility, 'FakeManualFacility': FakeManualFacility}) - @override_settings(TOM_FACILITY_CLASSES=['tom_observations.tests.utils.FakeRoboticFacility']) - def test_apps_without_integration_point_are_skipped(self): - # mock.Mock(spec=[]) raises AttributeError for observation_facilities(), behaving - # like a regular AppConfig that doesn't implement the integration point - with mock.patch('tom_observations.facility.apps.get_app_configs', return_value=[mock.Mock(spec=[])]): - service_classes = get_service_classes() - self.assertEqual(service_classes, {'FakeRoboticFacility': FakeRoboticFacility}) - @override_settings(TOM_FACILITY_CLASSES=['tom_observations.tests.utils.FakeRoboticFacility']) def test_facility_in_both_sources_is_deduplicated(self): fake_app_config = self._fake_app_config([{'class': 'tom_observations.tests.utils.FakeRoboticFacility'}]) @@ -553,57 +545,88 @@ class TestObservationFacilitiesNavbar(TestCase): and its navbar_facilities_list.html partial. """ - def _fake_app_config(self, facilities: list) -> mock.Mock: - """Return a mock AppConfig whose observation_facilities() returns the given list.""" - app_config = mock.Mock() - app_config.name = 'fake_facility_app' - app_config.observation_facilities.return_value = facilities - return app_config + def _patch_service_classes(self, *facility_classes): + """Patch get_service_classes() to return exactly the given facility classes.""" + return mock.patch( + 'tom_observations.templatetags.observation_extras.get_service_classes', + return_value={clazz.name: clazz for clazz in facility_classes}) - def test_responding_app_facility_is_listed(self): - # 'home' is a URL name that always resolves, standing in for a facility landing page - fake_app_config = self._fake_app_config( - [{'class': 'tom_observations.tests.utils.FakeRoboticFacility', 'url': 'home'}]) - with mock.patch('tom_observations.templatetags.observation_extras.apps.get_app_configs', - return_value=[fake_app_config]): - context = observation_facilities_list({}) + def test_facility_with_index_url_name_is_listed(self): + """Does the inclusiontag return a context with the correct Facility name and URL? + """ + # 'home' is a URL name that always resolves, standing in for a facility index page + with mock.patch.object(FakeRoboticFacility, 'index_url_name', 'home'), \ + self._patch_service_classes(FakeRoboticFacility): + context = observation_facilities_list({}) # function under test self.assertEqual(context['observation_facilities'], [{'name': 'FakeRoboticFacility', 'url': reverse('home')}]) - def test_no_responding_apps_yields_empty_context(self): - with mock.patch('tom_observations.templatetags.observation_extras.apps.get_app_configs', - return_value=[mock.Mock(spec=[])]): - context = observation_facilities_list({}) + def test_facility_without_index_url_name_gets_no_navbar_item_and_no_warning(self): + """A facility that leaves index_url_name as None is omitted from the navbar without logging a warning.""" + # registration-only facilities (e.g. tom_lt) have no index page; their absence from + # the navbar is deliberate, not a misconfiguration worth warning about + with self._patch_service_classes(FakeRoboticFacility), \ + mock.patch('tom_observations.templatetags.observation_extras.logger') as mock_logger: + context = observation_facilities_list({}) # function under test self.assertEqual(context['observation_facilities'], []) - - def test_dropdown_hidden_when_no_facilities(self): - html = render_to_string('tom_observations/partials/navbar_facilities_list.html', - {'observation_facilities': []}) - self.assertNotIn('Facilities', html) + mock_logger.warning.assert_not_called() def test_facility_with_unresolvable_url_is_skipped_with_warning(self): - fake_app_config = self._fake_app_config( - [{'class': 'tom_observations.tests.utils.FakeRoboticFacility', 'url': 'no-such-url-name'}]) - with mock.patch('tom_observations.templatetags.observation_extras.apps.get_app_configs', - return_value=[fake_app_config]), \ + """A facility whose index_url_name does not reverse() is skipped with a warning, not an exception.""" + with mock.patch.object(FakeRoboticFacility, 'index_url_name', 'no-such-url-name'), \ + self._patch_service_classes(FakeRoboticFacility), \ self.assertLogs('tom_observations.templatetags.observation_extras', level='WARNING'): - context = observation_facilities_list({}) + context = observation_facilities_list({}) # function under test self.assertEqual(context['observation_facilities'], []) - def test_facility_without_url_gets_no_navbar_item_and_no_warning(self): - # omitting 'url' declares a facility with no landing page (e.g. tom_lt): it is - # registered by get_service_classes() but deliberately absent from the navbar, - # and that absence is not a misconfiguration worth warning about - fake_app_config = self._fake_app_config([{'class': 'tom_observations.tests.utils.FakeRoboticFacility'}]) - with mock.patch('tom_observations.templatetags.observation_extras.apps.get_app_configs', - return_value=[fake_app_config]), \ - mock.patch('tom_observations.templatetags.observation_extras.logger') as mock_logger: + def test_facility_class_without_the_attribute_is_skipped(self): + """A facility class with no index_url_name attribute at all is skipped rather than raising AttributeError.""" + # pins the getattr() in the tag: a facility class that doesn't inherit from + # BaseObservationFacility must not 500 every page that renders the navbar + class RogueFacility: + name = 'RogueFacility' + # no index_url_name attribute + + with self._patch_service_classes(RogueFacility): + context = observation_facilities_list({}) # function under test + self.assertEqual(context['observation_facilities'], []) + + def test_no_facilities_yields_empty_context(self): + """With no facilities registered, the tag still supplies the (empty) observation_facilities context key.""" + # the partial's {% if observation_facilities %} guard relies on the key always being present + with self._patch_service_classes(): context = observation_facilities_list({}) self.assertEqual(context['observation_facilities'], []) - mock_logger.warning.assert_not_called() + + def test_settings_declared_facility_can_appear_in_navbar(self): + """A facility from TOM_FACILITY_CLASSES gets a navbar entry too, since the navbar is built + from get_service_classes(), which merges settings- and app-declared facilities.""" + with mock.patch.object(FakeRoboticFacility, 'index_url_name', 'home'), \ + self.settings(TOM_FACILITY_CLASSES=['tom_observations.tests.utils.FakeRoboticFacility']): + context = observation_facilities_list({}) # function under test + self.assertEqual(context['observation_facilities'], + [{'name': 'FakeRoboticFacility', 'url': reverse('home')}]) + + def test_dropdown_rendered_with_facility_links(self): + """The partial renders a Facilities dropdown with one link per facility, pointing at its URL.""" + html = render_to_string( + template_name='tom_observations/partials/navbar_facilities_list.html', # template under test + context={'observation_facilities': [{'name': 'FakeRoboticFacility', 'url': '/fake/'}]}) + + self.assertIn('Facilities', html) + self.assertIn('href="/fake/"', html) + self.assertIn('FakeRoboticFacility', html) + + def test_dropdown_hidden_when_no_facilities(self): + """The partial renders no Facilities dropdown at all when there are no facilities to list.""" + html = render_to_string( + template_name='tom_observations/partials/navbar_facilities_list.html', # template under test + context={'observation_facilities': []}) + self.assertNotIn('Facilities', html) def test_index_page_has_no_facilities_dropdown(self): - # tom_base's own test project has no app implementing observation_facilities(), - # so the rendered navbar should not contain the Facilities dropdown at all + """End to end: with no facility setting index_url_name, the home page renders without the dropdown.""" + # nothing in tom_base's own test project sets index_url_name (the built-in + # LCO/Gemini/SOAR/Blanco facilities have no index pages) response = self.client.get(reverse('home')) self.assertNotContains(response, '>Facilities<') From ede300bc177ebbcde2da7f4b77dbeb467b14b798 Mon Sep 17 00:00:00 2001 From: Joey Chatelain Date: Tue, 4 Aug 2026 13:41:40 -0700 Subject: [PATCH 11/18] update some doc strings --- tom_dataservices/dataservices.py | 15 ++++++----- tom_observations/facility.py | 44 ++++++++++++-------------------- tom_observations/tests/tests.py | 1 + 3 files changed, 26 insertions(+), 34 deletions(-) diff --git a/tom_dataservices/dataservices.py b/tom_dataservices/dataservices.py index 57673e7c1..182a3d5cd 100644 --- a/tom_dataservices/dataservices.py +++ b/tom_dataservices/dataservices.py @@ -84,8 +84,9 @@ class QueryServiceError(Exception): class DataService(ABC): - """ - Base class for all Data Services. Data Services are classes that are responsible for querying external services + """Base class for all Data Services. + + Data Services are classes that are responsible for querying external services and returning data. """ # Recognizable name for the DataService (Gaia, TNS, etc) @@ -123,11 +124,11 @@ def query_service(self, query_parameters, **kwargs): """Takes in the serialized data from the query form and actually submits the query to the service""" def pre_query_validation(self, query_parameters): - """Same thing as query_service, but a dry run""" + """Same thing as query_service, but a dry run.""" raise NotImplementedError(f'pre_query_validation method has not been implemented for {self.name}') def build_query_parameters(self, parameters, **kwargs): - """Builds the query parameters from the form data""" + """Builds the query parameters from the form data.""" raise NotImplementedError(f'build_query_parameters method has not been implemented for {self.name}') # Include this method if you wish for the TOM to be able to query data for an individual Target. @@ -200,7 +201,7 @@ def get_credentials(cls, **kwargs): @classmethod def urls(cls, **kwargs) -> dict: - """Dictionary of URLS for the DataService""" + """Dictionary of URLS for the DataService.""" return {'base_url': cls.base_url, 'info_url': cls.info_url} @classmethod @@ -387,6 +388,7 @@ def to_target(self, target_result=None, **kwargs): def create_target_from_query(self, target_result, **kwargs): """Create a new target from a single instance of the target results. + :param target_result: dictionary describing target details based on query result :returns: target object :rtype: `Target` @@ -420,7 +422,8 @@ def to_aliases(self, target, alias_results: List, **kwargs) -> List: return new_aliases def create_aliases_from_query(self, alias_results: List, **kwargs) -> List: - """Create a new target name from the query results + """Create a new target name from the query results. + This method should be over ridden with a method that creates a list of TargetName objects: `TargetName(name=alias)` that will be saved as part of the `Target.save(extras=extras, names=aliases)` call. :param query_result: list of dictionaries describing target details based on query result diff --git a/tom_observations/facility.py b/tom_observations/facility.py index 9cdedc6be..0beae45c6 100644 --- a/tom_observations/facility.py +++ b/tom_observations/facility.py @@ -20,8 +20,7 @@ class CredentialStatus(Enum): - """ - Enum representing the status of facility credentials. + """Enum representing the status of facility credentials. This enum is used to track the state of credentials throughout the facility lifecycle, providing clear information about whether credentials are available, where they came from, @@ -101,8 +100,8 @@ def get_service_class(name): class BaseObservationForm(forms.Form): - """ - This is the class that is responsible for displaying the observation request form. + """Class that is responsible for displaying the observation request form. + This form is meant to be subclassed by more specific BaseForm classes that represent a form for a particular type of facility. For implementing your own form, please look to the other BaseObservationForms. @@ -142,6 +141,7 @@ def __init__(self, *args, **kwargs): def layout(self) -> Layout: """Define (and return) a crispy_forms.Layout for the fields of your subclass. + It will be inserted after the common_layout and before the button_layout, as defined above in __init__(), where self.helper.layout is assigned. @@ -159,8 +159,8 @@ def button_layout(self): ) def get_validation_message(self): - """ Override this or self.validation_message to return a validation message that is shown when - the Validate button is clicked and the form is valid + """Override this or self.validation_message to return a validation message that is shown when + the Validate button is clicked and the form is valid """ return self.validation_message @@ -310,8 +310,7 @@ def _get_setting_credentials(self, facility_name, credential_keys): return credentials def _raise_no_profile_error(self, user, facility_name): - """ - Raise ImproperlyConfigured for missing user profile. + """Raise ImproperlyConfigured for missing user profile. Args: user: Django User instance @@ -326,8 +325,7 @@ def _raise_no_profile_error(self, user, facility_name): ) def _raise_no_defaults_error(self, user, facility_name): - """ - Raise ImproperlyConfigured when default credentials are needed but missing. + """Raise ImproperlyConfigured when default credentials are needed but missing. Args: user: Django User instance @@ -369,16 +367,14 @@ def all_data_products(self, observation_record): @abstractmethod def get_form(self, observation_type): - """ - This method takes in an observation type and returns the form type that matches it. + """This method takes in an observation type and returns the form type that matches it. Note: This method returns form classes, not instances, to support composite form creation in ObservationCreateView. Use create_form_instance() for direct form instantiation. """ def create_form_instance(self, observation_type, **kwargs): - """ - Create a form instance with facility context injected. + """Create a form instance with facility context injected. The ObservationCreateView handles setting the user context on the facility instance via set_user() in its dispatch() method. Forms receive the facility instance and @@ -394,8 +390,7 @@ def create_form_instance(self, observation_type, **kwargs): return form_class(**kwargs) def get_form_classes_for_display(self, **kwargs): - """ - This method returns a dictionary of the format: + """This method returns a dictionary of the format: {'OBSERVATION_TYPE': FacilityFormClass} @@ -430,14 +425,10 @@ def validate_observation(self, observation_payload): """ def get_flux_constant(self): - """ - Returns the astropy quantity that a facility uses for its spectral flux conversion. - """ + """Returns the astropy quantity that a facility uses for its spectral flux conversion.""" def get_wavelength_units(self): - """ - Returns the astropy units that a facility uses for its spectral wavelengths - """ + """Returns the astropy units that a facility uses for its spectral wavelengths.""" def is_fits_facility(self, header): """ @@ -455,10 +446,7 @@ def get_start_end_keywords(self): @abstractmethod def get_terminal_observing_states(self): - """ - Returns the states for which an observation is not expected - to change. - """ + """Returns the states for which an observation is not expected to change.""" @abstractmethod def get_observing_sites(self): @@ -524,11 +512,11 @@ def get_date_obs_from_fits_header(self, header): return None def get_button_label(self): - """ The label that will appear on observe button""" + """.The label that will appear on observe button.""" return self.button_label or self.name def get_button_tooltip(self): - """ The tooltip that will appear on observe button""" + """The tooltip that will appear on observe button.""" return self.button_tooltip diff --git a/tom_observations/tests/tests.py b/tom_observations/tests/tests.py index 757973af5..5b882c52b 100644 --- a/tom_observations/tests/tests.py +++ b/tom_observations/tests/tests.py @@ -306,6 +306,7 @@ def setUp(self): def test_callback(self): """ + The callback url is constructed by the OCS and the user is redirected to it after the observation record is created. This tests that a corresponding ObvservationRecord is created on the TOM side, just as if one was created using the built-in OCS form. From e0958c50a69aba6451ad66b58586b0e320f8c667 Mon Sep 17 00:00:00 2001 From: Joey Chatelain Date: Tue, 4 Aug 2026 13:54:19 -0700 Subject: [PATCH 12/18] fix a few more doc strings --- tom_observations/apps.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tom_observations/apps.py b/tom_observations/apps.py index 7b3934e24..9cfa8c2de 100644 --- a/tom_observations/apps.py +++ b/tom_observations/apps.py @@ -5,8 +5,8 @@ class TomObservationsConfig(AppConfig): name = 'tom_observations' def nav_items(self): - """ - Integration point for adding items to the navbar. + """Integration point for adding items to the navbar. + This method should return a list of partial templates to be included in the navbar. Here, the "Facilities" dropdown menu, listing the facilities contributed by installed From 42d52b5d91ee05ac518b069e3e8ba53377454ea6 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Tue, 4 Aug 2026 16:32:58 -0700 Subject: [PATCH 13/18] =?UTF-8?q?rename=20index=5Furl=5Fname=20=E2=86=92?= =?UTF-8?q?=20detail=5Furl=5Fname?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit in anticipation of Facility model with list- and detail views --- docs/observing/observation_module.rst | 6 ++-- tom_observations/facility.py | 6 ++-- .../partials/navbar_facilities_list.html | 2 +- .../templatetags/observation_extras.py | 12 ++++---- tom_observations/tests/tests.py | 30 +++++++++---------- 5 files changed, 28 insertions(+), 28 deletions(-) diff --git a/docs/observing/observation_module.rst b/docs/observing/observation_module.rst index 433f59d26..15d067cba 100644 --- a/docs/observing/observation_module.rst +++ b/docs/observing/observation_module.rst @@ -145,10 +145,10 @@ use. The ``BaseRoboticObservationForm`` class, just like the previous super class, contains logic and layout that all observation facility form classes should contain. -Linking to a facility index page --------------------------------- +Linking to a facility detail page +--------------------------------- -A facility may set ``index_url_name`` to the namespaced Django URL name of a page +A facility may set ``detail_url_name`` to the namespaced Django URL name of a page describing the facility. Facilities that set it appear in the navbar **Facilities** dropdown, linked to that page: diff --git a/tom_observations/facility.py b/tom_observations/facility.py index 0beae45c6..c151a6bd3 100644 --- a/tom_observations/facility.py +++ b/tom_observations/facility.py @@ -252,9 +252,9 @@ class BaseObservationFacility(ABC): is_redirect = False button_label = "" button_tooltip = "" - #: Namespaced URL name of this facility's index page. - #: None means no index page and no Facilities nav-bar menu item. - index_url_name: str | None = None + #: Namespaced URL name of this facility's detail page. + #: None means no detail page and no Facilities nav-bar menu item. + detail_url_name: str | None = None def __init__(self): self.user = None diff --git a/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html b/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html index 283d44617..cc38481a0 100644 --- a/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html +++ b/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html @@ -1,5 +1,5 @@ {# "Facilities" navbar dropdown: one menu item per facility whose class sets #} -{# index_url_name (see BaseObservationFacility). Context comes from #} +{# detail_url_name (see BaseObservationFacility). Context comes from #} {# observation_extras.observation_facilities_list. #} {% if observation_facilities %}
  • diff --git a/tom_observations/templatetags/observation_extras.py b/tom_observations/templatetags/observation_extras.py index 8a3a4ca63..6cfb53945 100644 --- a/tom_observations/templatetags/observation_extras.py +++ b/tom_observations/templatetags/observation_extras.py @@ -27,19 +27,19 @@ def observation_facilities_list(context: dict) -> dict: """ Returns the facilities linked from the "Facilities" navbar dropdown. - A facility from ``get_service_classes()`` is listed when its class sets ``index_url_name``. + A facility from ``get_service_classes()`` is listed when its class sets ``detail_url_name``. """ navbar_facilities = [] for facility_class in get_service_classes().values(): - index_url_name = getattr(facility_class, 'index_url_name', None) - if not index_url_name: - continue # registration-only facility: no index page, so no menu item + detail_url_name = getattr(facility_class, 'detail_url_name', None) + if not detail_url_name: + continue # registration-only facility: no detail page, so no menu item # make sure we can reverse the URL try: - url = reverse(index_url_name) + url = reverse(detail_url_name) except NoReverseMatch as e: - logger.warning(f'WARNING: Could not resolve index page URL for facility ' + logger.warning(f'WARNING: Could not resolve detail page URL for facility ' f'{facility_class.name}: {e}') continue navbar_facilities.append( diff --git a/tom_observations/tests/tests.py b/tom_observations/tests/tests.py index 5b882c52b..c4b136274 100644 --- a/tom_observations/tests/tests.py +++ b/tom_observations/tests/tests.py @@ -552,19 +552,19 @@ def _patch_service_classes(self, *facility_classes): 'tom_observations.templatetags.observation_extras.get_service_classes', return_value={clazz.name: clazz for clazz in facility_classes}) - def test_facility_with_index_url_name_is_listed(self): + def test_facility_with_detail_url_name_is_listed(self): """Does the inclusiontag return a context with the correct Facility name and URL? """ - # 'home' is a URL name that always resolves, standing in for a facility index page - with mock.patch.object(FakeRoboticFacility, 'index_url_name', 'home'), \ + # 'home' is a URL name that always resolves, standing in for a facility detail page + with mock.patch.object(FakeRoboticFacility, 'detail_url_name', 'home'), \ self._patch_service_classes(FakeRoboticFacility): context = observation_facilities_list({}) # function under test self.assertEqual(context['observation_facilities'], [{'name': 'FakeRoboticFacility', 'url': reverse('home')}]) - def test_facility_without_index_url_name_gets_no_navbar_item_and_no_warning(self): - """A facility that leaves index_url_name as None is omitted from the navbar without logging a warning.""" - # registration-only facilities (e.g. tom_lt) have no index page; their absence from + def test_facility_without_detail_url_name_gets_no_navbar_item_and_no_warning(self): + """A facility that leaves detail_url_name as None is omitted from the navbar without logging a warning.""" + # registration-only facilities (e.g. tom_lt) have no detail page; their absence from # the navbar is deliberate, not a misconfiguration worth warning about with self._patch_service_classes(FakeRoboticFacility), \ mock.patch('tom_observations.templatetags.observation_extras.logger') as mock_logger: @@ -573,20 +573,20 @@ def test_facility_without_index_url_name_gets_no_navbar_item_and_no_warning(self mock_logger.warning.assert_not_called() def test_facility_with_unresolvable_url_is_skipped_with_warning(self): - """A facility whose index_url_name does not reverse() is skipped with a warning, not an exception.""" - with mock.patch.object(FakeRoboticFacility, 'index_url_name', 'no-such-url-name'), \ + """A facility whose detail_url_name does not reverse() is skipped with a warning, not an exception.""" + with mock.patch.object(FakeRoboticFacility, 'detail_url_name', 'no-such-url-name'), \ self._patch_service_classes(FakeRoboticFacility), \ self.assertLogs('tom_observations.templatetags.observation_extras', level='WARNING'): context = observation_facilities_list({}) # function under test self.assertEqual(context['observation_facilities'], []) def test_facility_class_without_the_attribute_is_skipped(self): - """A facility class with no index_url_name attribute at all is skipped rather than raising AttributeError.""" + """A facility class with no detail_url_name attribute at all is skipped rather than raising AttributeError.""" # pins the getattr() in the tag: a facility class that doesn't inherit from # BaseObservationFacility must not 500 every page that renders the navbar class RogueFacility: name = 'RogueFacility' - # no index_url_name attribute + # no detail_url_name attribute with self._patch_service_classes(RogueFacility): context = observation_facilities_list({}) # function under test @@ -602,7 +602,7 @@ def test_no_facilities_yields_empty_context(self): def test_settings_declared_facility_can_appear_in_navbar(self): """A facility from TOM_FACILITY_CLASSES gets a navbar entry too, since the navbar is built from get_service_classes(), which merges settings- and app-declared facilities.""" - with mock.patch.object(FakeRoboticFacility, 'index_url_name', 'home'), \ + with mock.patch.object(FakeRoboticFacility, 'detail_url_name', 'home'), \ self.settings(TOM_FACILITY_CLASSES=['tom_observations.tests.utils.FakeRoboticFacility']): context = observation_facilities_list({}) # function under test self.assertEqual(context['observation_facilities'], @@ -625,9 +625,9 @@ def test_dropdown_hidden_when_no_facilities(self): context={'observation_facilities': []}) self.assertNotIn('Facilities', html) - def test_index_page_has_no_facilities_dropdown(self): - """End to end: with no facility setting index_url_name, the home page renders without the dropdown.""" - # nothing in tom_base's own test project sets index_url_name (the built-in - # LCO/Gemini/SOAR/Blanco facilities have no index pages) + def test_home_page_has_no_facilities_dropdown(self): + """End to end: with no facility setting detail_url_name, the home page renders without the dropdown.""" + # nothing in tom_base's own test project sets detail_url_name (the built-in + # LCO/Gemini/SOAR/Blanco facilities have no detail pages) response = self.client.get(reverse('home')) self.assertNotContains(response, '>Facilities<') From b07b1f6243e05cc200ae18fcf4d2765846bfa890 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Thu, 6 Aug 2026 09:44:23 -0700 Subject: [PATCH 14/18] update docs for facility_detail name change --- docs/observing/observation_module.rst | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/docs/observing/observation_module.rst b/docs/observing/observation_module.rst index 15d067cba..2bca41121 100644 --- a/docs/observing/observation_module.rst +++ b/docs/observing/observation_module.rst @@ -156,15 +156,13 @@ dropdown, linked to that page: class MyObservationFacility(BaseRoboticObservationFacility): name = 'MyFacility' - index_url_name = 'myapp:facility-index' + detail_url_name = 'myapp:facility-detail' -``index_url_name`` is optional and is omitted from the minimal example above. A facility +``detail_url_name`` is optional and is omitted from the minimal example above. A facility that leaves it unset is still fully registered -- it has an observe button and observation forms -- but gets no menu item. If no facility sets it, the dropdown is not displayed. - -The namespace is the one the facility's URLs are deployed under. For an app, that is the -``namespace`` argument its ``include_url_paths()`` passes to ``include()``, which is not -necessarily the app's name: ``tom_demoapp`` is deployed under ``demoapp``. +See ``tom_demoapp`` for a worked example, including composing the namespace from the +AppConfig's ``url_namespace`` attribute. Implementing observation submission ----------------------------------- From 3860480016c758971450071b3a7d550a5bae2ea3 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Mon, 10 Aug 2026 08:15:36 -0700 Subject: [PATCH 15/18] clean up some doc strings --- tom_observations/facility.py | 68 +++++++++++++++------------------ tom_observations/tests/tests.py | 3 +- 2 files changed, 32 insertions(+), 39 deletions(-) diff --git a/tom_observations/facility.py b/tom_observations/facility.py index c151a6bd3..846fd9794 100644 --- a/tom_observations/facility.py +++ b/tom_observations/facility.py @@ -208,8 +208,8 @@ class BaseRoboticObservationForm(BaseObservationForm): class BaseManualObservationForm(BaseObservationForm): - """ - This is the class that is responsible for displaying the observation request form. + """Base class for observation request forms. + Facility classes that provide a form should subclass this form. It provides some base shared functionality. Extra fields are provided below. The layout is handled by Django crispy forms which allows customizability of the @@ -241,12 +241,7 @@ def layout(self): class BaseObservationFacility(ABC): - """ - This is the class that is responsible for defining the base facility class. - This form is meant to be subclassed by more specific BaseFacility classes that represent a - form for a particular type of facility. For implementing your own form, please look to - the other BaseObservationFacilities. - """ + """Base class for observation facilities.""" name = 'BaseObservation' observation_forms = {} is_redirect = False @@ -410,17 +405,16 @@ def get_facility_context_data(self, **kwargs): # TODO: consider making submit_observation create ObservationRecords as well @abstractmethod def submit_observation(self, observation_payload): - """ - This method takes in the serialized data from the form and actually + """This method takes in the serialized data from the form and actually submits the observation to the remote api """ @abstractmethod def validate_observation(self, observation_payload): - """ - Same thing as submit_observation, but a dry run. You can - skip this in different modules by just using "pass" + """Validate an observation request through the facility's API, + but don't submit the request (i.e. a "dry-run"). + You can skip this in different modules by just using "pass" Typically called by the ObservationForm.is_valid() method. """ @@ -431,16 +425,16 @@ def get_wavelength_units(self): """Returns the astropy units that a facility uses for its spectral wavelengths.""" def is_fits_facility(self, header): - """ - Returns True if the FITS header is from this facility based on valid keywords and associated - values, False otherwise. + """Returns True if the FITS header is from this facility. + + Return value is based on valid keywords and associated values. """ return False def get_start_end_keywords(self): - """ - Returns the keywords representing the start and end of an observation window for a facility. Defaults to - ``start`` and ``end``. + """Returns the keywords representing the start and end of an observation window for a facility. + + Defaults to ``start`` and ``end``. """ return 'start', 'end' @@ -450,19 +444,19 @@ def get_terminal_observing_states(self): @abstractmethod def get_observing_sites(self): - """ - Return an iterable of dictionaries that contain the information - necessary to be used in the planning (visibility) tool. The - iterable should contain dictionaries each that contain sitecode, + """Return an iterable of dictionaries that contain the information + necessary to be used in the planning (visibility) tool. + + The returned iterable should contain dictionaries each that contain sitecode, latitude, longitude and elevation. This is the static information about a site. """ def get_facility_weather_urls(self): - """ - Returns a dictionary containing a URL for weather information - for each site in the Facility SITES. This is intended to be useful - in observation planning. + """Returns a dictionary containing a URL for weather information + for each site in the Facility SITES. + + This is intended to be useful in observation planning. `facility_weather = {'code': 'XYZ', 'sites': [ site_dict, ... ]}` where @@ -472,9 +466,9 @@ def get_facility_weather_urls(self): return {} def get_facility_status(self): - """ - Returns a dictionary describing the current availability of the Facility - telescopes. This is intended to be useful in observation planning. + """Returns a dictionary describing the current availability of the Facility telescopes. + + This is intended to be useful in observation planning. The top-level (Facility) dictionary has a list of sites. Each site is represented by a site dictionary which has a list of telescopes. Each telescope has an identifier (code) and an status string. @@ -492,8 +486,8 @@ def get_facility_status(self): return {} def cancel_observation(self, observation_id): - """ - Takes an observation id and submits a request to the observatory that the observation be cancelled. + """Takes an observation id and submits a request to the observation facility + that the observation be cancelled. If the cancellation was successful, return True. Otherwise, return False. """ @@ -501,10 +495,10 @@ def cancel_observation(self, observation_id): @abstractmethod def get_observation_url(self, observation_id): - """ - Takes an observation id and return the url for which a user - can view the observation at an external location. In this case, - we return a URL to the LCO observation portal's observation + """Takes an observation id and returns the url with which a user can view the observation + at the observation facility. + + For example, we return a URL to the LCO observation portal's observation record page. """ @@ -512,7 +506,7 @@ def get_date_obs_from_fits_header(self, header): return None def get_button_label(self): - """.The label that will appear on observe button.""" + """The label that will appear on observe button.""" return self.button_label or self.name def get_button_tooltip(self): diff --git a/tom_observations/tests/tests.py b/tom_observations/tests/tests.py index c4b136274..91fc1063f 100644 --- a/tom_observations/tests/tests.py +++ b/tom_observations/tests/tests.py @@ -306,9 +306,8 @@ def setUp(self): def test_callback(self): """ - The callback url is constructed by the OCS and the user is redirected to it after - the observation record is created. This tests that a corresponding ObvservationRecord is created + the observation record is created. This tests that a corresponding ObservationRecord is created on the TOM side, just as if one was created using the built-in OCS form. The view should redirect the user to the detail view of the observation record. """ From cd3c0f88372ec1167cd22dff863c3a5bf4cf6756 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Tue, 11 Aug 2026 15:56:23 -0700 Subject: [PATCH 16/18] clean up doc strings, comments, and error messages --- tom_observations/facility.py | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/tom_observations/facility.py b/tom_observations/facility.py index 846fd9794..328b83c60 100644 --- a/tom_observations/facility.py +++ b/tom_observations/facility.py @@ -64,6 +64,7 @@ def get_service_classes() -> dict: TOM_FACILITY_CLASSES = DEFAULT_FACILITY_CLASSES service_choices = {} + # 1 get the facilities from settings.py for service in TOM_FACILITY_CLASSES: try: clazz = import_string(service) @@ -71,17 +72,21 @@ def get_service_classes() -> dict: raise ImportError(f'Could not import {service}: {e}') service_choices[clazz.name] = clazz - # Combine the settings-declared facilities with those contributed by installed apps - # via the observation_facilities() AppConfig integration point. + # 2 add the faciliites from apps implementing the integration point for app in apps.get_app_configs(): - try: - observation_facilities = app.observation_facilities() - except AttributeError: + observation_facilities_hook = getattr(app, 'observation_facilities', None) + if observation_facilities_hook is None: continue # this app doesn't implement the integration point - for facility in observation_facilities: + for facility in observation_facilities_hook() or []: # `or []` tolerates a hook returning None try: clazz = import_string(facility['class']) + except KeyError: + # the integration point returned a mal-formed conifguration dict + logger.warning(f'WARNING: observation_facilities() entry from {app.name} is missing ' + f'the required "class" key: {facility!r}. Facility skipped.') + continue except ImportError as e: + # the class couldn't be imported logger.warning(f'WARNING: Could not import facility class for {app.name} from ' f'{facility["class"]}.\n' f'{e}') @@ -92,11 +97,16 @@ def get_service_classes() -> dict: def get_service_class(name): + """Return the single, named facility class. + + Note: Implementation gets all the facilities and returns the named one. + """ available_classes = get_service_classes() try: return available_classes[name] except KeyError: - raise ImportError('Could not a find a facility with that name. Did you add it to TOM_FACILITY_CLASSES?') + raise ImportError(f'Could not find a facility named {name}. Add it to settings.TOM_FACILITY_CLASSES or ' + f'implement the observation_facilities() integration point in the AppConfig subclass.') class BaseObservationForm(forms.Form): From 6f835ea36dc51266b494667ba96a82111cd03256 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Tue, 11 Aug 2026 15:59:19 -0700 Subject: [PATCH 17/18] make Facilities drop-down css consisten with other nav-bar menus This way we response to CSS_THEME over rides like everyone else. --- .../tom_observations/partials/navbar_facilities_list.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html b/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html index cc38481a0..313167ec3 100644 --- a/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html +++ b/tom_observations/templates/tom_observations/partials/navbar_facilities_list.html @@ -6,8 +6,8 @@ Facilities From fa03486f58d0b252f664dec415dd79105a199518 Mon Sep 17 00:00:00 2001 From: "William (Lindy) Lindstrom" Date: Tue, 11 Aug 2026 16:42:29 -0700 Subject: [PATCH 18/18] add documentation for observation_facilities integration point --- docs/observing/observation_module.rst | 37 ++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/docs/observing/observation_module.rst b/docs/observing/observation_module.rst index 2bca41121..bd1088c0a 100644 --- a/docs/observing/observation_module.rst +++ b/docs/observing/observation_module.rst @@ -108,8 +108,10 @@ loaded. Adding a facility from an app ----------------------------- -A reusable app can contribute its facilities without the TOM editing ``settings.py``. -Implement ``observation_facilities()`` on the app's ``AppConfig``: +A facility implemented as a Django reusable app can advertise itself +to a TOM without the editing ``settings.py``, which simplifies it's installation. +To do this, implement the ``observation_facilities()`` integration point +on the app's ``AppConfig`` subclass: .. code:: python @@ -119,10 +121,11 @@ Implement ``observation_facilities()`` on the app's ``AppConfig``: def observation_facilities(self): return [{'class': f'{self.name}.myfacility.MyObservationFacility'}] -Facilities from both routes are merged by -``tom_observations.facility.get_service_classes()``, so adding the app to -``INSTALLED_APPS`` is all that is required of the TOM. See ``tom_demoapp`` for a -worked example. +Facilities from both ``settings.TOM_FACILITY_CLASSES`` and the apps implementing +the integration point are merged by ``tom_observations.facility.get_service_classes()``. +So, if your facility implements the ``observation_facilities()`` integration point, +adding the app to ``INSTALLED_APPS`` is all that is required for installation. +See ``tom_demoapp`` for a worked example. BaseRoboticObservationFacility and BaseRoboticObservationForm ------------------------------------------------------------- @@ -164,6 +167,28 @@ forms -- but gets no menu item. If no facility sets it, the dropdown is not disp See ``tom_demoapp`` for a worked example, including composing the namespace from the AppConfig's ``url_namespace`` attribute. +For ``detail_url_name`` to resolve, the app's URLs must be mounted in the TOM under that +namespace. A reusable app mounts its own ``urls.py`` through the ``include_url_paths()`` +AppConfig integration point -- ``tom_common`` collects these paths from every installed +app, so no TOM ``urls.py`` edits are required: + +.. code:: python + + class MyAppConfig(AppConfig): + name = 'myapp' + url_prefix = 'myapp' # URL path prefix for this app's pages: HOST/myapp/... + url_namespace = 'myapp' # the namespace half of detail_url_name + + def include_url_paths(self): + return [ + path(f'{self.url_prefix}/', include(f'{self.name}.urls', namespace=self.url_namespace)), + ] + +(``url_prefix`` and ``url_namespace`` are TOM plugin conventions, not Django AppConfig +attributes.) The included ``urls.py`` must set ``app_name`` -- Django requires it when +``include()`` is called with ``namespace=`` -- and must contain a ``path()`` whose +``name=`` is the second half of ``detail_url_name``. + Implementing observation submission -----------------------------------