diff --git a/observation_portal/common/configdb.py b/observation_portal/common/configdb.py index 07065bc8..93fad5d3 100644 --- a/observation_portal/common/configdb.py +++ b/observation_portal/common/configdb.py @@ -63,7 +63,7 @@ def get_site_data(self): return self._get_configdb_data('sites') def get_sites_with_instrument_type_and_location( - self, instrument_type: str='', site_code: str='', enclosure_code: str='', telescope_code: str='' + self, instrument_type: str = '', site_code: str = '', enclosure_code: str = '', telescope_code: str = '' ) -> dict: """Get the location details for each site for which a resource exists. @@ -152,13 +152,18 @@ def get_instruments_at_location(self, site_code, enclosure_code, telescope_code, for site in self.get_site_data(): if site['code'].lower() == site_code.lower(): for enclosure in site['enclosure_set']: - if enclosure['code'].lower() == enclosure_code: + if enclosure['code'].lower() == enclosure_code.lower(): for telescope in enclosure['telescope_set']: - if telescope['code'].lower() == telescope_code: + if telescope['code'].lower() == telescope_code.lower(): for instrument in telescope['instrument_set']: - if only_schedulable and instrument['state'] == 'SCHEDULABLE' or not only_schedulable: + if ( + only_schedulable and self.is_schedulable(instrument) + or (not only_schedulable and self.is_active(instrument)) + ): instrument_names.add(instrument['code'].lower()) - instrument_types.add(instrument['science_camera']['camera_type']['code'].lower()) + instrument_types.add( + instrument['science_camera']['camera_type']['code'].lower() + ) return {'names': instrument_names, 'types': instrument_types} def get_telescopes_with_instrument_type_and_location( @@ -174,7 +179,7 @@ def get_telescopes_with_instrument_type_and_location( if not telescope_code or telescope_code == telescope['code']: code = '.'.join([telescope['code'], enclosure['code'], site['code']]) for instrument in telescope['instrument_set']: - if instrument['state'] == 'SCHEDULABLE': + if self.is_schedulable(instrument): camera_type = instrument['science_camera']['camera_type']['code'] if not instrument_type or instrument_type.upper() == camera_type.upper(): if code not in telescope_details: @@ -186,7 +191,6 @@ def get_telescopes_with_instrument_type_and_location( 'ha_limit_pos': telescope['ha_limit_pos'], 'ha_limit_neg': telescope['ha_limit_neg'] } - return telescope_details def is_valid_instrument_type(self, instrument_type): @@ -194,7 +198,10 @@ def is_valid_instrument_type(self, instrument_type): for enclosure in site['enclosure_set']: for telescope in enclosure['telescope_set']: for instrument in telescope['instrument_set']: - if instrument_type.upper() == instrument['science_camera']['camera_type']['code'].upper(): + if ( + instrument_type.upper() == instrument['science_camera']['camera_type']['code'].upper() + and self.is_active(instrument) + ): return True return False @@ -203,8 +210,7 @@ def is_valid_instrument(self, instrument_name): for enclosure in site['enclosure_set']: for telescope in enclosure['telescope_set']: for instrument in telescope['instrument_set']: - if instrument_name.upper() == instrument['code'].upper(): - + if instrument_name.upper() == instrument['code'].upper() and self.is_active(instrument): return True return False @@ -215,9 +221,10 @@ def get_instruments(self, only_schedulable=False): for enclosure in site['enclosure_set']: for telescope in enclosure['telescope_set']: for instrument in telescope['instrument_set']: - if only_schedulable and instrument['state'] != 'SCHEDULABLE': - pass - else: + if ( + (only_schedulable and self.is_schedulable(instrument)) + or (not only_schedulable and self.is_active(instrument)) + ): telescope_key = TelescopeKey( site=site['code'], enclosure=enclosure['code'], @@ -228,7 +235,7 @@ def get_instruments(self, only_schedulable=False): return instruments - def get_instrument_types_per_telescope(self, only_schedulable: bool=False) -> dict: + def get_instrument_types_per_telescope(self, only_schedulable: bool = False) -> dict: """Get a set of available instrument types per telescope. Parameters: @@ -260,14 +267,16 @@ def get_instrument_names( """ instrument_names = set() for instrument in self.get_instruments(): - if (instrument['telescope_key'].site.lower() == site_code.lower() + if ( + instrument['telescope_key'].site.lower() == site_code.lower() and instrument['telescope_key'].enclosure.lower() == enclosure_code.lower() and instrument['telescope_key'].telescope.lower() == telescope_code.lower() - and instrument['science_camera']['camera_type']['code'].lower() == instrument_type.lower()): + and instrument['science_camera']['camera_type']['code'].lower() == instrument_type.lower() + ): instrument_names.add(instrument['science_camera']['code'].lower()) return instrument_names - def get_instrument_types_per_telescope_class(self, only_schedulable: bool=False) -> dict: + def get_instrument_types_per_telescope_class(self, only_schedulable: bool = False) -> dict: """Get a set of instrument types. Instrument types are returned by telescope class (0m4, 1m0, etc...) @@ -285,7 +294,7 @@ def get_instrument_types_per_telescope_class(self, only_schedulable: bool=False) telescope_instrument_types[tel_code].add(instrument['science_camera']['camera_type']['code'].upper()) return telescope_instrument_types - def get_telescopes_per_instrument_type(self, instrument_type: str, only_schedulable: bool=False) -> set: + def get_telescopes_per_instrument_type(self, instrument_type: str, only_schedulable: bool = False) -> set: """Get a set of telescope keys. Parameters: @@ -324,7 +333,7 @@ def get_optical_elements(self, instrument_type: str) -> dict: optical_elements[optical_element_group['type']].append(element) return optical_elements - def get_modes_by_type(self, instrument_type: str, mode_type: str='') -> dict: + def get_modes_by_type(self, instrument_type: str, mode_type: str = '') -> dict: """Get the set of available modes. Parameters: @@ -336,7 +345,10 @@ def get_modes_by_type(self, instrument_type: str, mode_type: str='') -> dict: for instrument in self.get_instruments(): if instrument_type.upper() == instrument['science_camera']['camera_type']['code'].upper(): if not mode_type: - return {mode_group['type']: mode_group for mode_group in instrument['science_camera']['camera_type']['mode_types']} + return { + mode_group['type']: mode_group + for mode_group in instrument['science_camera']['camera_type']['mode_types'] + } else: for mode_group in instrument['science_camera']['camera_type']['mode_types']: if mode_group['type'] == mode_type: @@ -350,20 +362,22 @@ def get_mode_with_code(self, instrument_type, code, mode_type=''): if mode['code'].lower() == code.lower(): return mode - raise ConfigDBException("No mode named {} found for instrument type {}".format(code, instrument_type)) + raise ConfigDBException(f'No mode named {code} found for instrument type {instrument_type}') def get_readout_mode_with_binning(self, instrument_type, binning): readout_modes = self.get_modes_by_type(instrument_type, 'readout') if readout_modes: - modes = sorted(readout_modes['readout']['modes'], key=lambda x: x['code'] == readout_modes['readout']['default'], reverse=True) # Start with the default + modes = sorted( + readout_modes['readout']['modes'], key=lambda x: x['code'] == readout_modes['readout'].get('default'), + reverse=True + ) # Start with the default for mode in modes: if mode['params'].get('binning', -1) == binning: return mode - raise ConfigDBException("No readout mode found with binning {} for instrument type {}".format(binning, - instrument_type)) + raise ConfigDBException(f'No readout mode found with binning {binning} for instrument type {instrument_type}') - def get_default_modes_by_type(self, instrument_type: str, mode_type: str='') -> dict: + def get_default_modes_by_type(self, instrument_type: str, mode_type: str = '') -> dict: """Get the default mode of each available mode_type. Parameters: @@ -373,12 +387,13 @@ def get_default_modes_by_type(self, instrument_type: str, mode_type: str='') -> Default modes """ modes = self.get_modes_by_type(instrument_type, mode_type) - for type, mode_set in modes.items(): - for mode in mode_set['modes']: - if mode['code'] == mode_set['default']: - modes[type] = mode + default_modes = {} + for m_type, m_set in modes.items(): + for mode in m_set['modes']: + if 'default' in m_set and mode['code'] == m_set['default']: + default_modes[m_type] = mode break - return modes + return default_modes def get_binnings(self, instrument_type: str) -> set: """Create a set of available binning modes. @@ -459,7 +474,7 @@ def get_guider_for_instrument_name(self, instrument_name): for instrument in instruments: if instrument['code'].lower() == instrument_name.lower(): return instrument['autoguider_camera']['code'].lower() - raise ConfigDBException(_("Instrument not found: {}".format(instrument_name))) + raise ConfigDBException(_(f'Instrument not found: {instrument_name}')) def is_valid_guider_for_instrument_name(self, instrument_name, guide_camera_name): instruments = self.get_instruments(only_schedulable=False) @@ -481,7 +496,7 @@ def get_exposure_overhead(self, instrument_type, binning, readout_mode=''): if 'readout' in modes_by_type: default_mode = {} for mode in modes_by_type['readout']['modes']: - if mode['code'] == modes_by_type['readout']['default']: + if mode['code'] == modes_by_type['readout'].get('default'): default_mode = mode if readout_mode and readout_mode.lower() != mode['code'].lower(): continue @@ -490,7 +505,7 @@ def get_exposure_overhead(self, instrument_type, binning, readout_mode=''): # if the binning is not found, return the default binning (Added to support legacy 2x2 Sinistro obs) return default_mode['overhead'] + camera_type['fixed_overhead_per_exposure'] - raise ConfigDBException("Instrument type {} not found in configdb.".format(instrument_type)) + raise ConfigDBException(f'Instrument type {instrument_type} not found in configdb.') def get_request_overheads(self, instrument_type: str) -> dict: """Get the set of overheads needed to compute the duration of a request. @@ -513,19 +528,28 @@ def get_request_overheads(self, instrument_type: str) -> dict: for instrument in telescope['instrument_set']: camera_type = instrument['science_camera']['camera_type'] if camera_type['code'].upper() == instrument_type.upper(): - return {'instrument_change_overhead': telescope['instrument_change_overhead'], - 'slew_rate': telescope['slew_rate'], - 'minimum_slew_overhead': telescope['minimum_slew_overhead'], - 'maximum_slew_overhead': telescope.get('maximum_slew_overhead', 0.0), - 'config_change_overhead': camera_type['config_change_time'], - 'default_acquisition_exposure_time': camera_type['acquire_exposure_time'], - 'acquisition_overheads': {am['code']: am['overhead'] for am in modes_by_type['acquisition']['modes']} if 'acquisition' in modes_by_type else {}, - 'guiding_overheads': {gm['code']: gm['overhead'] for gm in modes_by_type['guiding']['modes']} if 'guiding' in modes_by_type else {}, - 'front_padding': camera_type['front_padding'], - 'optical_element_change_overheads': - {oeg['type']: oeg['element_change_overhead'] for oeg in instrument['science_camera']['optical_element_groups']} - } - raise ConfigDBException("Instrument type {} not found in configdb.".format(instrument_type)) + return { + 'instrument_change_overhead': telescope['instrument_change_overhead'], + 'slew_rate': telescope['slew_rate'], + 'minimum_slew_overhead': telescope['minimum_slew_overhead'], + 'maximum_slew_overhead': telescope.get('maximum_slew_overhead', 0.0), + 'config_change_overhead': camera_type['config_change_time'], + 'default_acquisition_exposure_time': camera_type['acquire_exposure_time'], + 'acquisition_overheads': { + am['code']: am['overhead'] + for am in modes_by_type['acquisition']['modes'] + } if 'acquisition' in modes_by_type else {}, + 'guiding_overheads': { + gm['code']: gm['overhead'] + for gm in modes_by_type['guiding']['modes'] + } if 'guiding' in modes_by_type else {}, + 'front_padding': camera_type['front_padding'], + 'optical_element_change_overheads': { + oeg['type']: oeg['element_change_overhead'] + for oeg in instrument['science_camera']['optical_element_groups'] + } + } + raise ConfigDBException(f'Instrument type {instrument_type} not found in configdb.') @staticmethod def is_spectrograph(instrument_type): @@ -540,5 +564,13 @@ def is_nres(instrument_type): def is_floyds(instrument_type): return 'FLOYDS' in instrument_type.upper() + @staticmethod + def is_active(instrument: dict) -> bool: + return instrument['state'].upper() != 'DISABLED' + + @staticmethod + def is_schedulable(instrument: dict) -> bool: + return instrument['state'] == 'SCHEDULABLE' + configdb = ConfigDB() diff --git a/observation_portal/common/test_data/configdb.json b/observation_portal/common/test_data/configdb.json index bb2c7b10..15b9563f 100644 --- a/observation_portal/common/test_data/configdb.json +++ b/observation_portal/common/test_data/configdb.json @@ -144,6 +144,252 @@ ] }, "__str__": "tst.doma.1m0a.xx01-xx01" + }, + { + "state": "DISABLED", + "code": "xx06", + "allow_self_guiding": true, + "autoguider_camera": { + "code": "xx07", + "camera_type": { + "code": "1M0-SCICAM-SBIG" + } + }, + "science_camera": { + "code": "xx06", + "camera_type": { + "code": "1M0-SCICAM-SBIG", + "name": "1M0-SCICAM-SBIG", + "default_mode": { + "binning": 2, + "readout": 14.5 + }, + "configuration_types": [ + "EXPOSE", + "BIAS", + "DARK", + "SCRIPT" + ], + "default_acceptability_threshold": 90, + "pixels_x": 1000, + "pixels_y": 1000, + "max_rois": 1, + "config_change_time": 0, + "acquire_processing_time": 0, + "acquire_exposure_time": 0, + "front_padding": 90, + "filter_change_time": 2, + "fixed_overhead_per_exposure": 1, + "mode_set": [ + { + "binning": 1, + "readout": 35.0 + }, + { + "binning": 2, + "readout": 14.5 + }, + { + "binning": 3, + "readout": 11.5 + } + ], + "mode_types": [ + { + "type": "readout", + "default": "1m0_sbig_2", + "modes": [ + { + "code": "1m0_sbig_1", + "overhead": 35.0, + "params": { + "binning": 1 + } + }, + { + "code": "1m0_sbig_2", + "overhead": 14.5, + "params": { + "binning": 2 + } + }, + { + "code": "1m0_sbig_3", + "overhead": 11.5, + "params": { + "binning": 3 + } + } + ] + }, + { + "type": "acquisition", + "default": "OFF", + "modes": [ + { + "code": "OFF", + "overhead": 0.0, + "params": {} + } + ] + }, + { + "type": "guiding", + "default": "OFF", + "modes": [ + { + "code": "OFF", + "overhead": 0.0, + "params": {} + }, + { + "code": "ON", + "overhead": 0.0, + "params": {} + } + ] + } + ] + }, + "filters": "air", + "optical_element_groups": [ + { + "type": "filters", + "element_change_overhead": 2, + "optical_elements": [ + { + "name": "Air", + "code": "air", + "schedulable": false + } + ] + } + ] + }, + "__str__": "tst.doma.1m0a.xx06-xx07" + }, + { + "state": "ENABLED", + "code": "xx08", + "allow_self_guiding": true, + "autoguider_camera": { + "code": "xx09", + "camera_type": { + "code": "1M0-SCICAM-SBIG" + } + }, + "science_camera": { + "code": "xx08", + "camera_type": { + "code": "1M0-SCICAM-SBIG", + "name": "1M0-SCICAM-SBIG", + "default_mode": { + "binning": 2, + "readout": 14.5 + }, + "configuration_types": [ + "EXPOSE", + "BIAS", + "DARK", + "SCRIPT" + ], + "default_acceptability_threshold": 90, + "pixels_x": 1000, + "pixels_y": 1000, + "max_rois": 1, + "config_change_time": 0, + "acquire_processing_time": 0, + "acquire_exposure_time": 0, + "front_padding": 90, + "filter_change_time": 2, + "fixed_overhead_per_exposure": 1, + "mode_set": [ + { + "binning": 1, + "readout": 35.0 + }, + { + "binning": 2, + "readout": 14.5 + }, + { + "binning": 3, + "readout": 11.5 + } + ], + "mode_types": [ + { + "type": "readout", + "default": "1m0_sbig_2", + "modes": [ + { + "code": "1m0_sbig_1", + "overhead": 35.0, + "params": { + "binning": 1 + } + }, + { + "code": "1m0_sbig_2", + "overhead": 14.5, + "params": { + "binning": 2 + } + }, + { + "code": "1m0_sbig_3", + "overhead": 11.5, + "params": { + "binning": 3 + } + } + ] + }, + { + "type": "acquisition", + "default": "OFF", + "modes": [ + { + "code": "OFF", + "overhead": 0.0, + "params": {} + } + ] + }, + { + "type": "guiding", + "default": "OFF", + "modes": [ + { + "code": "OFF", + "overhead": 0.0, + "params": {} + }, + { + "code": "ON", + "overhead": 0.0, + "params": {} + } + ] + } + ] + }, + "filters": "air", + "optical_element_groups": [ + { + "type": "filters", + "element_change_overhead": 2, + "optical_elements": [ + { + "name": "Air", + "code": "air", + "schedulable": false + } + ] + } + ] + }, + "__str__": "tst.doma.1m0a.xx08-xx09" } ] }, @@ -492,7 +738,6 @@ "mode_types": [ { "type": "readout", - "default": "1m0_nres_2", "modes": [ { "code": "1m0_nres_1", @@ -516,8 +761,19 @@ "modes": [ { "code": "WCS", - "overhead": 0.0, + "overhead": 400.0, "params": {} + }, + { + "code": "MYMODE", + "overhead": 400.0, + "params": { + "required_fields": [ + "field1", + "field2", + "field3" + ] + } } ] }, @@ -599,7 +855,6 @@ "mode_types": [ { "type": "readout", - "default": "2m0_floyds_1", "modes": [ { "code": "2m0_floyds_1", diff --git a/observation_portal/observations/serializers.py b/observation_portal/observations/serializers.py index cafcd137..de367ff7 100644 --- a/observation_portal/observations/serializers.py +++ b/observation_portal/observations/serializers.py @@ -190,7 +190,11 @@ def validate(self, data): if (configuration['guiding_config']['mode'] != GuidingConfig.OFF or configuration['acquisition_config']['mode'] != AcquisitionConfig.OFF): if not configuration.get('guide_camera_name', ''): - if 'self_guide' in configuration['extra_params'] and configuration['extra_params']['self_guide']: + if ( + 'extra_params' in configuration + and 'self_guide' in configuration['extra_params'] + and configuration['extra_params']['self_guide'] + ): configuration['guide_camera_name'] = configuration['instrument_name'] else: configuration['guide_camera_name'] = configdb.get_guider_for_instrument_name( diff --git a/observation_portal/proposals/templates/proposals/proposal_list.html b/observation_portal/proposals/templates/proposals/proposal_list.html index 61e14fa1..17667bba 100644 --- a/observation_portal/proposals/templates/proposals/proposal_list.html +++ b/observation_portal/proposals/templates/proposals/proposal_list.html @@ -1,5 +1,6 @@ {% extends 'base.html' %} {% load bootstrap4 %} +{% bootstrap_javascript jquery='full' %} {% block title %} Proposal List {% endblock %} {% block content %}
diff --git a/observation_portal/proposals/test_views.py b/observation_portal/proposals/test_views.py index b0efdaa8..3810af8f 100644 --- a/observation_portal/proposals/test_views.py +++ b/observation_portal/proposals/test_views.py @@ -121,6 +121,23 @@ def test_set_bad_limit(self): self.assertEqual(membership.time_limit, 0) self.assertContains(response, 'Please enter a valid time limit') + def test_set_bad_global_limit(self): + self.client.force_login(self.pi_user) + ci_users = mixer.cycle(5).blend(User) + mixer.cycle(5).blend(Profile, user=(ci_user for ci_user in ci_users)) + memberships = mixer.cycle(5).blend( + Membership, user=(c for c in ci_users), proposal=self.proposal, role=Membership.CI, time_limit=0 + ) + response = self.client.post( + reverse('proposals:membership-global', kwargs={'pk': self.proposal.id}), + data={'time_limit': ''}, + follow=True + ) + self.assertContains(response, 'Please enter a valid time limit') + for membership in memberships: + membership.refresh_from_db() + self.assertEqual(membership.time_limit, 0) + class TestProposalInvite(TestCase): def setUp(self): diff --git a/observation_portal/proposals/views.py b/observation_portal/proposals/views.py index c7923932..01e12e09 100644 --- a/observation_portal/proposals/views.py +++ b/observation_portal/proposals/views.py @@ -84,9 +84,13 @@ def post(self, request, **kwargs): proposal = request.user.membership_set.get(proposal=kwargs.get('pk'), role=Membership.PI).proposal except Membership.DoesNotExist: raise Http404 - time_limit = float(request.POST['time_limit']) * 3600 - proposal.membership_set.filter(role=Membership.CI).update(time_limit=time_limit) - messages.success(request, 'All CI time limits set to {0} hours'.format(time_limit / 3600)) + try: + time_limit = float(request.POST['time_limit']) * 3600 + except ValueError: + messages.error(request, 'Please enter a valid time limit') + else: + proposal.membership_set.filter(role=Membership.CI).update(time_limit=time_limit) + messages.success(request, 'All CI time limits set to {0} hours'.format(time_limit / 3600)) return HttpResponseRedirect(reverse('proposals:detail', kwargs={'pk': proposal.id})) diff --git a/observation_portal/requestgroups/duration_utils.py b/observation_portal/requestgroups/duration_utils.py index eeb1bd44..56364a43 100644 --- a/observation_portal/requestgroups/duration_utils.py +++ b/observation_portal/requestgroups/duration_utils.py @@ -157,12 +157,13 @@ def get_request_duration(request_dict): # Now add in optical element change time if the set of optical elements has changed for inst_config in configuration['instrument_configs']: + optical_elements = inst_config.get('optical_elements', {}) change_overhead = 0 - for oe_type, oe_value in inst_config['optical_elements'].items(): + for oe_type, oe_value in optical_elements.items(): if oe_type not in previous_optical_elements or oe_value != previous_optical_elements[oe_type]: if '{}s'.format(oe_type) in request_overheads['optical_element_change_overheads']: change_overhead = max(request_overheads['optical_element_change_overheads']['{}s'.format(oe_type)], change_overhead) - previous_optical_elements = inst_config['optical_elements'] + previous_optical_elements = optical_elements duration += change_overhead # Now add in the slew time between targets (configurations). Only Sidereal can be calculated based on position. @@ -187,7 +188,7 @@ def get_request_duration(request_dict): guide_optional = configuration['guiding_config']['optional'] if 'optional' in configuration['guiding_config'] \ else True if configuration['guiding_config']['mode'] != 'OFF' and not guide_optional: - if (configuration['guiding_config']['mode'] in request_overheads['guiding_overheads']): + if configuration['guiding_config']['mode'] in request_overheads['guiding_overheads']: duration += request_overheads['guiding_overheads'][configuration['guiding_config']['mode']] # TODO: find out if we need to have a configuration type change time for spectrographs? diff --git a/observation_portal/requestgroups/models.py b/observation_portal/requestgroups/models.py index ad485ead..3a863b96 100644 --- a/observation_portal/requestgroups/models.py +++ b/observation_portal/requestgroups/models.py @@ -654,7 +654,6 @@ def as_dict(self): class GuidingConfig(models.Model): - ON = 'ON' OFF = 'OFF' SERIALIZER_EXCLUDE = ('id', 'configuration') diff --git a/observation_portal/requestgroups/serializers.py b/observation_portal/requestgroups/serializers.py index 6c5e0ddf..a535536e 100644 --- a/observation_portal/requestgroups/serializers.py +++ b/observation_portal/requestgroups/serializers.py @@ -1,3 +1,7 @@ +import json +import logging +from json import JSONDecodeError + from rest_framework import serializers from django.utils.translation import ugettext as _ from django.core.exceptions import ObjectDoesNotExist @@ -5,9 +9,6 @@ from django.db import transaction from django.utils import timezone from django.core.validators import MinValueValidator, MaxValueValidator -from json import JSONDecodeError -import logging -import json from observation_portal.proposals.models import TimeAllocation, Membership from observation_portal.requestgroups.models import ( @@ -25,10 +26,83 @@ from datetime import timedelta from observation_portal.common.rise_set_utils import get_filtered_rise_set_intervals_by_site, get_largest_interval - logger = logging.getLogger(__name__) +class ModeValidationHelper: + """Class used to validate modes of different types""" + def __init__(self, mode_type, instrument_type, default_modes, modes): + self._mode_type = mode_type.lower() + self._instrument_type = instrument_type + self._default_modes = default_modes + self._modes = modes + self._mode_key = 'rotator_mode' if self._mode_type == 'rotator' else 'mode' + self._modes_by_code = {} + + def _possible_modes(self) -> list: + possible_modes = [] + if self._mode_type in self._default_modes: + possible_modes.append(self._default_modes[self._mode_type]) + elif self._mode_type in self._modes: + # There are modes to choose from. This would normally not happen since defaults should be set. + possible_modes.extend(self._modes[self._mode_type]['modes']) + return possible_modes + + def _unavailable_msg(self, config: dict) -> str: + if self._mode_type in self._modes: + if not config[self._mode_key].lower() in [m['code'].lower() for m in self._modes[self._mode_type]['modes']]: + return ( + f'{self._mode_type.capitalize()} mode {config[self._mode_key]} is not available for ' + f'instrument type {self._instrument_type}' + ) + return '' + + def _missing_fields_msg(self, config) -> str: + missing_fields = [] + mode = configdb.get_mode_with_code(self._instrument_type, config[self._mode_key], self._mode_type) + if 'required_fields' in mode.get('params', {}): + for field in mode['params']['required_fields']: + if 'extra_params' not in config or field not in config['extra_params']: + missing_fields.append(field) + if missing_fields: + return ( + f'{self._mode_type.capitalize()} Mode {mode["code"]} requires [{", ".join(missing_fields)}] ' + f'set in extra params' + ) + return '' + + def mode_is_not_set(self, config: dict) -> bool: + return self._mode_key not in config or not config[self._mode_key] + + def get_mode_to_set(self) -> dict: + """Choose a mode to set""" + mode = {'error': '', 'mode': {}} + possible_modes = self._possible_modes() + if len(possible_modes) == 1: + # There is only one mode to choose from, so set that. + mode['mode'] = possible_modes[0] + elif len(possible_modes) > 1: + # There are many possible modes, make the user choose. + mode['error'] = ( + f'Must set a {self._mode_type} mode, choose ' + f'from {", ".join([mode["code"] for mode in self._modes["guiding"]["modes"]])}' + ) + return mode + + def get_mode_error_msg(self, config: dict) -> str: + """Return an error message if there is a problem with the mode""" + if self._mode_type in self._modes: + # Check if the mode exists + unavailable_msg = self._unavailable_msg(config) + if unavailable_msg: + return unavailable_msg + # Check if there are any required params that are not set + missing_fields_msg = self._missing_fields_msg(config) + if missing_fields_msg: + return missing_fields_msg + return '' + + class CadenceSerializer(serializers.Serializer): start = serializers.DateTimeField() end = serializers.DateTimeField() @@ -84,8 +158,9 @@ def validate(self, data): data['bin_x'] = data['bin_y'] if 'bin_x' in data and 'bin_y' in data and data['bin_x'] != data['bin_y']: - raise serializers.ValidationError(_("Currently only square binnings are supported. Please submit with bin_x == bin_y")) - + raise serializers.ValidationError(_( + 'Currently only square binnings are supported. Please submit with bin_x == bin_y' + )) return data def to_representation(self, instance): @@ -151,8 +226,9 @@ class Meta: def validate_instrument_configs(self, value): # TODO: remove this check once we support multiple instrument configs if len(value) != 1: - raise serializers.ValidationError(_('Currently only a single instrument_config is supported. This restriction will be lifted in the future.')) - + raise serializers.ValidationError(_( + 'Currently only a single instrument_config is supported. This restriction will be lifted in the future.' + )) if [instrument_config.get('fill_window', False) for instrument_config in value].count(True) > 1: raise serializers.ValidationError(_('Only one instrument_config can have `fill_window` set')) return value @@ -167,23 +243,28 @@ def validate_instrument_type(self, value): return value def validate(self, data): - modes = configdb.get_modes_by_type(data['instrument_type']) - default_modes = configdb.get_default_modes_by_type(data['instrument_type']) - guiding_config = data['guiding_config'] - # Set defaults for guiding and acquisition modes if they are not set # TODO: Validate the guiding optical elements on the guiding instrument types - if 'mode' not in guiding_config: - if 'guiding' in default_modes: - guiding_config['mode'] = default_modes['guiding']['code'] + instrument_type = data['instrument_type'] + modes = configdb.get_modes_by_type(instrument_type) + default_modes = configdb.get_default_modes_by_type(instrument_type) + guiding_config = data['guiding_config'] + + # Validate the guide mode + guide_validation_helper = ModeValidationHelper('guiding', instrument_type, default_modes, modes) + if guide_validation_helper.mode_is_not_set(guiding_config): + guide_mode_to_set = guide_validation_helper.get_mode_to_set() + if guide_mode_to_set['error']: + raise serializers.ValidationError(_(guide_mode_to_set['error'])) + if guide_mode_to_set['mode']: + guiding_config['mode'] = guide_mode_to_set['mode']['code'] else: - # This should never happen if we have configdb filled out correctly guiding_config['mode'] = GuidingConfig.OFF - elif ('guiding' in modes - and guiding_config['mode'].lower() not in [gm['code'].lower() for gm in modes['guiding']['modes']]): - raise serializers.ValidationError(_("Guiding mode {} is not available for instrument type {}" - .format(guiding_config['mode'], data['instrument_type']))) - if configdb.is_spectrograph(data['instrument_type']) and data['type'] not in ['LAMP_FLAT', 'ARC']: + guide_mode_error_msg = guide_validation_helper.get_mode_error_msg(guiding_config) + if guide_mode_error_msg: + raise serializers.ValidationError(_(guide_mode_error_msg)) + + if configdb.is_spectrograph(instrument_type) and data['type'] not in ['LAMP_FLAT', 'ARC']: if 'optional' in guiding_config and guiding_config['optional']: raise serializers.ValidationError(_( "Guiding cannot be optional on spectrograph instruments for types that are not ARC or LAMP_FLAT." @@ -193,75 +274,84 @@ def validate(self, data): if data['type'] in ['LAMP_FLAT', 'ARC', 'AUTO_FOCUS', 'NRES_BIAS', 'NRES_DARK', 'BIAS', 'DARK', 'SCRIPT']: # These types of observations should only ever be set to guiding mode OFF, but the acquisition modes for # spectrographs won't necessarily have that mode. Force OFF here. - data['acquisition_config']['mode'] = 'OFF' + data['acquisition_config']['mode'] = AcquisitionConfig.OFF else: + # Validate acquire modes acquisition_config = data['acquisition_config'] - if 'mode' not in acquisition_config: - if 'acquisition' in default_modes: - acquisition_config['mode'] = default_modes['acquisition']['code'] - elif 'acquisition' in modes and acquisition_config['mode'] not in [am['code'] for am in modes['acquisition']['modes']]: - raise serializers.ValidationError(_("Acquisition mode {} is not available for instrument type {}" - .format(acquisition_config['mode'], data['instrument_type']))) - - # check for any required fields for acquisition - if acquisition_config['mode'] != 'OFF': - acquisition_mode = configdb.get_mode_with_code(data['instrument_type'], acquisition_config['mode'], - 'acquisition') - - if 'required_fields' in acquisition_mode['params']: - for field in acquisition_mode['params']['required_fields']: - if field not in acquisition_config['extra_params']: - raise serializers.ValidationError(_("Acquisition Mode {} required extra param of {} to be set" - .format(acquisition_mode['code'], field))) - - # Validate the optical elements, rotator and readout modes specified in the instrument configs - available_optical_elements = configdb.get_optical_elements(data['instrument_type']) + acquire_validation_helper = ModeValidationHelper('acquisition', instrument_type, default_modes, modes) + if acquire_validation_helper.mode_is_not_set(acquisition_config): + acquire_mode_to_set = acquire_validation_helper.get_mode_to_set() + if acquire_mode_to_set['error']: + raise serializers.ValidationError(_(acquire_mode_to_set['error'])) + if acquire_mode_to_set['mode']: + acquisition_config['mode'] = acquire_mode_to_set['mode']['code'] + else: + acquisition_config['mode'] = AcquisitionConfig.OFF + + acquire_mode_error_msg = acquire_validation_helper.get_mode_error_msg(acquisition_config) + if acquire_mode_error_msg: + raise serializers.ValidationError(_(acquire_mode_error_msg)) + + available_optical_elements = configdb.get_optical_elements(instrument_type) for instrument_config in data['instrument_configs']: - if ('mode' not in instrument_config or not instrument_config['mode']) and 'readout' in default_modes: + # Validate the readout mode and the binning. Readout modes and binning are tied + # together- If one is set, we can determine the other. + # TODO: Remove the binning checks when binnings are removed entirely + readout_validation_helper = ModeValidationHelper('readout', instrument_type, default_modes, modes) + if readout_validation_helper.mode_is_not_set(instrument_config): if 'bin_x' not in instrument_config and 'bin_y' not in instrument_config: - instrument_config['mode'] = default_modes['readout']['code'] - instrument_config['bin_x'] = default_modes['readout']['params']['binning'] - instrument_config['bin_y'] = instrument_config['bin_x'] + # Set the readout mode as well as the binning + readout_mode_to_set = readout_validation_helper.get_mode_to_set() + if readout_mode_to_set['error']: + raise serializers.ValidationError(_(readout_mode_to_set['error'])) + if readout_mode_to_set['mode']: + instrument_config['mode'] = readout_mode_to_set['mode']['code'] + instrument_config['bin_x'] = readout_mode_to_set['mode']['params']['binning'] + instrument_config['bin_y'] = readout_mode_to_set['mode']['params']['binning'] + elif 'bin_x' in instrument_config: + # A binning is set already - figure out what the readout mode should be from that try: - instrument_config['mode'] = configdb.get_readout_mode_with_binning(data['instrument_type'], - instrument_config['bin_x'])['code'] + instrument_config['mode'] = configdb.get_readout_mode_with_binning( + instrument_type, instrument_config['bin_x'] + )['code'] except ConfigDBException as cdbe: raise serializers.ValidationError(_(str(cdbe))) - else: - try: - readout_mode = configdb.get_mode_with_code(data['instrument_type'], - instrument_config['mode'], 'readout') - except ConfigDBException as cdbe: - raise serializers.ValidationError(_(str(cdbe))) + # A readout mode is set - validate the mode + readout_error_msg = readout_validation_helper.get_mode_error_msg(instrument_config) + if readout_error_msg: + raise serializers.ValidationError(_(readout_error_msg)) + + # At this point the readout mode that is set is valid. Now either set the binnings, or make + # sure that those that are set are ok + readout_mode = configdb.get_mode_with_code(instrument_type, instrument_config['mode'], 'readout') if 'bin_x' not in instrument_config: instrument_config['bin_x'] = readout_mode['params']['binning'] instrument_config['bin_y'] = readout_mode['params']['binning'] + elif instrument_config['bin_x'] != readout_mode['params']['binning']: - raise serializers.ValidationError(_("binning {} is not a valid binning on readout mode {} for instrument type {}" - .format(instrument_config['bin_x'], instrument_config['mode'], data['instrument_type']))) + raise serializers.ValidationError(_( + f'Binning {instrument_config["bin_x"]} is not a valid binning for readout mode ' + f'{instrument_config["mode"]} for instrument type {instrument_type}' + )) - # Validate the rotator modes if set in configdb + # Validate the rotator modes if 'rotator' in modes: - if ('rotator_mode' not in instrument_config or not instrument_config['rotator_mode'] - and 'rotator' in default_modes): - instrument_config['rotator_mode'] = default_modes['rotator']['code'] - - try: - rotator_mode = configdb.get_mode_with_code(data['instrument_type'], instrument_config['rotator_mode'], - 'rotator') - if 'required_fields' in rotator_mode['params']: - for field in rotator_mode['params']['required_fields']: - if 'extra_params' not in instrument_config or field not in instrument_config['extra_params']: - raise serializers.ValidationError( - _("Rotator Mode {} required extra param of {} to be set" - .format(rotator_mode['code'], field))) - except ConfigDBException as cdbe: - raise serializers.ValidationError(_(str(cdbe))) + rotator_mode_validation_helper = ModeValidationHelper('rotator', instrument_type, default_modes, modes) + if rotator_mode_validation_helper.mode_is_not_set(instrument_config): + rotator_mode_to_set = rotator_mode_validation_helper.get_mode_to_set() + if rotator_mode_to_set['error']: + raise serializers.ValidationError(_(rotator_mode_to_set['error'])) + if rotator_mode_to_set['mode']: + instrument_config['rotator_mode'] = rotator_mode_to_set['mode']['code'] + + rotator_error_msg = rotator_mode_validation_helper.get_mode_error_msg(instrument_config) + if rotator_error_msg: + raise serializers.ValidationError(_(rotator_error_msg)) # Check that the optical elements specified are valid in configdb - for oe_type, value in instrument_config['optical_elements'].items(): + for oe_type, value in instrument_config.get('optical_elements', {}).items(): plural_type = '{}s'.format(oe_type) if plural_type not in available_optical_elements: raise serializers.ValidationError(_("optical_element of type {} is not available on {} instruments" @@ -278,21 +368,17 @@ def validate(self, data): if data['type'].upper() not in observation_types_without_oe: for oe_type in available_optical_elements.keys(): singular_type = oe_type[:-1] if oe_type.endswith('s') else oe_type - if singular_type not in instrument_config['optical_elements']: - raise serializers.ValidationError( - _("must specify optical element of type {} for instrument type {}".format( - singular_type, data['instrument_type'] - )) - ) + if singular_type not in instrument_config.get('optical_elements', {}): + raise serializers.ValidationError(_( + f'Must set optical element of type {singular_type} for instrument type {instrument_type}' + )) # Validate any regions of interest if 'rois' in instrument_config: - max_rois = configdb.get_max_rois(data['instrument_type']) - ccd_size = configdb.get_ccd_size(data['instrument_type']) + max_rois = configdb.get_max_rois(instrument_type) + ccd_size = configdb.get_ccd_size(instrument_type) if len(instrument_config['rois']) > max_rois: raise serializers.ValidationError(_( - 'Instrument type {} supports up to {} regions of interest'.format( - data['instrument_type'], max_rois - ) + f'Instrument type {instrument_type} supports up to {max_rois} regions of interest' )) for roi in instrument_config['rois']: if 'x1' not in roi and 'x2' not in roi and 'y1' not in roi and 'y2' not in roi: @@ -315,23 +401,25 @@ def validate(self, data): if roi['x2'] > ccd_size['x'] or roi['y2'] > ccd_size['y']: raise serializers.ValidationError(_( 'Regions of interest for instrument type {} must be in range 0<=x<={} and 0<=y<={}'.format( - data['instrument_type'], ccd_size['x'], ccd_size['y'] + instrument_type, ccd_size['x'], ccd_size['y'] )) ) if data['type'] == 'SCRIPT': - if ('extra_params' not in data or 'script_name' not in data['extra_params'] - or not data['extra_params']['script_name']): - raise serializers.ValidationError( - _("Must specify a script_name in extra_params for SCRIPT configuration type") - ) + if ( + 'extra_params' not in data + or 'script_name' not in data['extra_params'] + or not data['extra_params']['script_name'] + ): + raise serializers.ValidationError(_( + 'Must specify a script_name in extra_params for SCRIPT configuration type' + )) # Validate the configuration type is available for the instrument requested - if data['type'] not in configdb.get_configuration_types(data['instrument_type']): - raise serializers.ValidationError(_("configuration type {} is not valid for instrument type {}").format( - data['type'], data['instrument_type'] + if data['type'] not in configdb.get_configuration_types(instrument_type): + raise serializers.ValidationError(_( + f'configuration type {data["type"]} is not valid for instrument type {instrument_type}' )) - return data @@ -360,12 +448,9 @@ def validate(self, data): enc_dict = {enc['code']: enc for enc in enc_set} if 'enclosure' in data: if data['enclosure'] not in enc_dict: - msg = _('Enclosure {} not valid. Valid choices: {}').format( - data['enclosure'], - ', '.join(enc_dict.keys()) - ) - raise serializers.ValidationError(msg) - + raise serializers.ValidationError(_( + f'Enclosure {data["enclosure"]} not valid. Valid choices: {", ".join(enc_dict.keys())}' + )) tel_set = enc_dict[data['enclosure']]['telescope_set'] tel_list = [tel['code'] for tel in tel_set] if 'telescope' in data and data['telescope'] not in tel_list: @@ -375,12 +460,10 @@ def validate(self, data): return data def to_representation(self, instance): - ''' + """ This method is overridden to remove blank fields from serialized output. We could put this into a subclassed ModelSerializer if we want it to apply to all our Serializers. - :param instance: - :return: - ''' + """ rep = super().to_representation(instance) return {key: val for key, val in rep.items() if val} @@ -396,7 +479,7 @@ def validate(self, data): if 'start' not in data: data['start'] = timezone.now() if data['end'] <= data['start']: - msg = _("Window end '{}' cannot be earlier than window start '{}'.").format(data['end'], data['start']) + msg = _(f"Window end '{data['end']}' cannot be earlier than window start '{data['start']}'") raise serializers.ValidationError(msg) if not get_semester_in(data['start'], data['end']): @@ -434,10 +517,15 @@ def validate_configurations(self, value): configuration['priority'] = i + 1 # TODO: Remove this once we support multiple targets/constraints if configuration['target'] != target: - raise serializers.ValidationError(_('Currently only a single target per Request is supported. This restriction will be lifted in the future.')) + raise serializers.ValidationError(_( + 'Currently only a single target per Request is supported. This restriction will be lifted in ' + 'the future.' + )) if configuration['constraints'] != constraints: - raise serializers.ValidationError(_('Currently only a single constraints per Request is supported. This restriction will be lifted in the future.')) - + raise serializers.ValidationError(_( + 'Currently only a single constraints per Request is supported. This restriction will be ' + 'lifted in the future.' + )) return value def validate_windows(self, value): @@ -513,8 +601,8 @@ def validate(self, data): raise serializers.ValidationError( ( 'According to the constraints of the request, the target is visible for a maximum of {0:.2f} ' - 'hours within the time window. This is less than the duration of your request {1:.2f} hours. Consider ' - 'expanding the time window or loosening the airmass or lunar separation constraints.' + 'hours within the time window. This is less than the duration of your request {1:.2f} hours. ' + 'Consider expanding the time window or loosening the airmass or lunar separation constraints.' ).format( largest_interval.total_seconds() / 3600.0, duration / 3600.0 diff --git a/observation_portal/requestgroups/templates/requestgroups/partials/requestgroup_header.html b/observation_portal/requestgroups/templates/requestgroups/partials/requestgroup_header.html index 1bde1a21..37c9df6c 100644 --- a/observation_portal/requestgroups/templates/requestgroups/partials/requestgroup_header.html +++ b/observation_portal/requestgroups/templates/requestgroups/partials/requestgroup_header.html @@ -1,4 +1,5 @@ -{% load i18n request_extras %} +{% load i18n bootstrap4 request_extras %} +{% bootstrap_javascript jquery='full' %}

{{ requestgroup.name }}
RequestGroup # {{ requestgroup.id }}

diff --git a/observation_portal/requestgroups/templates/requestgroups/requestgroup_list.html b/observation_portal/requestgroups/templates/requestgroups/requestgroup_list.html index 11c8a712..1b531fe3 100644 --- a/observation_portal/requestgroups/templates/requestgroups/requestgroup_list.html +++ b/observation_portal/requestgroups/templates/requestgroups/requestgroup_list.html @@ -1,6 +1,7 @@ {% extends 'index.html' %} {% load render_bundle from webpack_loader %} {% load i18n bootstrap4 request_extras humanize static %} +{% bootstrap_javascript jquery='full' %} {% block title %}Submitted Requests{% endblock %} {% block extra_css %} diff --git a/observation_portal/requestgroups/test/test_api.py b/observation_portal/requestgroups/test/test_api.py index cb584ea8..f22aa3be 100644 --- a/observation_portal/requestgroups/test/test_api.py +++ b/observation_portal/requestgroups/test/test_api.py @@ -312,7 +312,7 @@ def test_post_requestgroup_acquire_mode_brightest_no_radius(self): bad_data['requests'][0]['configurations'][0]['acquisition_config']['mode'] = 'BRIGHTEST' response = self.client.post(reverse('api:request_groups-list'), data=bad_data) self.assertEqual(response.status_code, 400) - self.assertIn('required extra param of acquire_radius to be set', str(response.content)) + self.assertIn('requires [acquire_radius] set in extra param', str(response.content)) def test_post_requestgroup_acquire_mode_brightest(self): good_data = self.generic_payload.copy() @@ -1260,6 +1260,52 @@ def test_guide_optional_allowed_for_arc(self): response = self.client.post(reverse('api:request_groups-list'), data=good_data) self.assertEqual(response.status_code, 201) + def test_guiding_mode_not_available(self): + bad_data = self.generic_payload.copy() + bad_data['requests'][0]['configurations'][0]['guiding_config']['mode'] = 'I_DONT_EXIST' + response = self.client.post(reverse('api:request_groups-list'), data=bad_data) + self.assertIn('Guiding mode I_DONT_EXIST is not available', str(response.content)) + + def test_readout_mode_many_options(self): + bad_data = self.generic_payload.copy() + bad_data['requests'][0]['configurations'][0]['instrument_type'] = '1M0-NRES-SCICAM' + bad_data['requests'][0]['configurations'][0]['type'] = 'NRES_SPECTRUM' + bad_data['requests'][0]['configurations'][0]['instrument_configs'][0]['optical_elements'] = {} + bad_data['requests'][0]['configurations'][0]['instrument_configs'][0]['mode'] = '' + del bad_data['requests'][0]['configurations'][0]['instrument_configs'][0]['bin_x'] + del bad_data['requests'][0]['configurations'][0]['instrument_configs'][0]['bin_y'] + response = self.client.post(reverse('api:request_groups-list'), data=bad_data) + self.assertIn('Must set a readout mode, choose from', str(response.content)) + + def test_instrument_type_does_not_have_readout_mode_then_mode_stays_blank(self): + good_data = self.generic_payload.copy() + response = self.client.post(reverse('api:request_groups-list'), data=good_data) + self.assertEqual(response.status_code, 201) + self.assertEqual(response.json()['requests'][0]['configurations'][0]['instrument_configs'][0]['rotator_mode'], '') + + def test_readout_mode_not_set_and_no_default_but_single_option_set(self): + good_data = self.generic_payload.copy() + good_data['requests'][0]['configurations'][0]['type'] = 'SPECTRUM' + good_data['requests'][0]['configurations'][0]['instrument_type'] = '2M0-FLOYDS-SCICAM' + good_data['requests'][0]['location']['telescope_class'] = '2m0' + del good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['mode'] + del good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['optical_elements']['filter'] + good_data['requests'][0]['configurations'][0]['instrument_configs'][0]['optical_elements']['slit'] = 'slit_1.6as' + response = self.client.post(reverse('api:request_groups-list'), data=good_data) + self.assertEqual(response.status_code, 201) + # The only readout mode for floyds is 2m0_floyds_1 + self.assertEqual(response.json()['requests'][0]['configurations'][0]['instrument_configs'][0]['mode'], '2m0_floyds_1') + + def test_many_missing_required_fields(self): + bad_data = self.generic_payload.copy() + bad_data['requests'][0]['configurations'][0]['instrument_type'] = '1M0-NRES-SCICAM' + bad_data['requests'][0]['configurations'][0]['type'] = 'NRES_SPECTRUM' + bad_data['requests'][0]['configurations'][0]['instrument_configs'][0]['optical_elements'] = {} + bad_data['requests'][0]['configurations'][0]['acquisition_config']['mode'] = 'MYMODE' + bad_data['requests'][0]['configurations'][0]['acquisition_config']['extra_params']['field1'] = 'some-arg' + response = self.client.post(reverse('api:request_groups-list'), data=bad_data) + self.assertIn('Acquisition Mode MYMODE requires [field2, field3] set', str(response.content)) + def test_invalid_filter_for_instrument(self): bad_data = self.generic_payload.copy() bad_data['requests'][0]['configurations'][0]['instrument_configs'][0]['optical_elements']['filter'] = 'magic' @@ -1278,7 +1324,7 @@ def test_filter_not_necessary_for_type(self): response = self.client.post(reverse('api:request_groups-list'), data=good_data) self.assertEqual(response.status_code, 201) - def test_args_required_for_script_type(self): + def test_script_name_required_for_script_type(self): bad_data = self.generic_payload.copy() bad_data['requests'][0]['configurations'][0]['type'] = 'SCRIPT' response = self.client.post(reverse('api:request_groups-list'), data=bad_data) @@ -1337,7 +1383,7 @@ def test_filter_necessary_for_type(self): del bad_data['requests'][0]['configurations'][0]['instrument_configs'][0]['optical_elements']['filter'] response = self.client.post(reverse('api:request_groups-list'), data=bad_data) self.assertEqual(response.status_code, 400) - self.assertIn('must specify optical element of type filter', str(response.content)) + self.assertIn('Must set optical element of type filter', str(response.content)) def test_invalid_spectra_slit_for_instrument(self): bad_data = self.generic_payload.copy() @@ -1364,7 +1410,7 @@ def test_invalid_binning_for_instrument_for_mode(self): bad_data['requests'][0]['configurations'][0]['instrument_configs'][0]['bin_x'] = 5 bad_data['requests'][0]['configurations'][0]['instrument_configs'][0]['bin_y'] = 5 response = self.client.post(reverse('api:request_groups-list'), data=bad_data) - self.assertIn('binning 5 is not a valid binning on readout mode 1m0_sbig_2', str(response.content)) + self.assertIn('Binning 5 is not a valid binning for readout mode 1m0_sbig_2', str(response.content)) self.assertEqual(response.status_code, 400) def test_readout_mode_sets_default_binning(self): @@ -1551,7 +1597,7 @@ def test_configuration_type_matches_instrument(self): self.assertIn('configuration type EXPOSE is not valid for instrument type 2M0-FLOYDS-SCICAM', str(response.content)) self.assertEqual(response.status_code, 400) - def test_acquirition_config_exposure_time_limits(self): + def test_acquisition_config_exposure_time_limits(self): bad_data = self.generic_payload.copy() bad_data['requests'][0]['configurations'][0]['acquisition_config']['exposure_time'] = -1 response = self.client.post(reverse('api:request_groups-list'), data=bad_data) diff --git a/observation_portal/sciapplications/admin.py b/observation_portal/sciapplications/admin.py index c1140d97..7f9bec51 100644 --- a/observation_portal/sciapplications/admin.py +++ b/observation_portal/sciapplications/admin.py @@ -82,8 +82,15 @@ def port(self, request, queryset): request, 'Application {} has no approved Time Allocations'.format(app.title), level='ERROR' ) return - app.send_approved_notification() self.message_user(request, 'Proposal {} successfully created.'.format(app.proposal)) + notification_sent = app.send_approved_notification() + if not notification_sent: + self.message_user( + request, + 'Email notifying PI of approval of proposal {} failed to send'.format(app.proposal), + level='ERROR' + ) + return admin.site.register(ScienceApplication, ScienceApplicationAdmin) diff --git a/observation_portal/sciapplications/models.py b/observation_portal/sciapplications/models.py index 60484bc2..bdfe72fd 100644 --- a/observation_portal/sciapplications/models.py +++ b/observation_portal/sciapplications/models.py @@ -1,3 +1,6 @@ +import io +import smtplib + from django.db import models from django.utils import timezone from django.contrib.auth.models import User @@ -7,7 +10,6 @@ from django.contrib.staticfiles import finders from weasyprint import HTML, CSS from PyPDF2 import PdfFileMerger -import io from observation_portal.accounts.tasks import send_mail from observation_portal.proposals.models import ( @@ -210,7 +212,16 @@ def send_approved_notification(self): 'semester_already_started': self.call.semester.start < timezone.now(), } ) - send_mail.send(subject, message, 'portal@lco.global', [self.proposal.pi.email]) + # Find the email to send the notification to. The proposal will have been created at this point, but the pi on + # the proposal might not be set yet if the pi has not a registered an account. In that case, use the email + # as set on the science application. + pi_email = self.proposal.pi.email if self.proposal.pi else self.pi + email_sent = True + try: + send_mail.send(subject, message, 'portal@lco.global', [pi_email]) + except smtplib.SMTPException: + email_sent = False + return email_sent class TimeRequest(models.Model): diff --git a/observation_portal/sciapplications/test_admin.py b/observation_portal/sciapplications/test_admin.py index af17e803..f1bfe1db 100644 --- a/observation_portal/sciapplications/test_admin.py +++ b/observation_portal/sciapplications/test_admin.py @@ -66,7 +66,7 @@ def test_port(self): for app in self.apps: self.assertEqual(ScienceApplication.objects.get(pk=app.id).status, ScienceApplication.PORTED) - def test_email_pi_on_successful_port(self): + def test_email_pi_on_successful_port_pi_is_submitter(self): app_id_to_port = self.apps[0].id ScienceApplication.objects.filter(pk=app_id_to_port).update(status=ScienceApplication.ACCEPTED) self.client.post( @@ -75,10 +75,55 @@ def test_email_pi_on_successful_port(self): follow=True ) self.assertEqual(len(mail.outbox), 1) + self.assertEqual(ScienceApplication.objects.get(pk=app_id_to_port).proposal.pi.email, self.user.email) self.assertEqual([ScienceApplication.objects.get(pk=app_id_to_port).proposal.pi.email], mail.outbox[0].to) self.assertIn(ScienceApplication.objects.get(pk=app_id_to_port).proposal.id, str(mail.outbox[0].message())) self.assertIn(ScienceApplication.objects.get(pk=app_id_to_port).call.semester.id, str(mail.outbox[0].message())) + def test_email_pi_on_successful_port_when_registered_pi_is_not_submitter(self): + other_user = mixer.blend(User) + app_id_to_port = self.apps[0].id + ScienceApplication.objects.filter(pk=app_id_to_port).update(status=ScienceApplication.ACCEPTED) + ScienceApplication.objects.filter(pk=app_id_to_port).update(pi=other_user.email) + self.client.post( + reverse('admin:sciapplications_scienceapplication_changelist'), + data={'action': 'port', '_selected_action': [str(app.pk) for app in self.apps]}, + follow=True + ) + # If the PI is registered but is not the submitter, they will receive two emails- one telling them + # their proposal has been approved, and another telling them them that they have been added to that proposal + # and can begin submitting requests. The approval email is second. + self.assertEqual(len(mail.outbox), 2) + self.assertEqual(ScienceApplication.objects.get(pk=app_id_to_port).proposal.pi.email, other_user.email) + # Added to proposal email + self.assertEqual([other_user.email], mail.outbox[0].to) + self.assertIn('You have been added to', str(mail.outbox[0].message())) + # Proposal approved email + self.assertEqual([other_user.email], mail.outbox[1].to) + self.assertIn(ScienceApplication.objects.get(pk=app_id_to_port).proposal.id, str(mail.outbox[1].message())) + self.assertIn(ScienceApplication.objects.get(pk=app_id_to_port).call.semester.id, str(mail.outbox[1].message())) + + def test_email_pi_on_successful_port_when_pi_is_not_registered(self): + non_registered_email = 'somenonexistantuser@email.com' + app_id_to_port = self.apps[0].id + ScienceApplication.objects.filter(pk=app_id_to_port).update(status=ScienceApplication.ACCEPTED) + ScienceApplication.objects.filter(pk=app_id_to_port).update(pi=non_registered_email) + self.client.post( + reverse('admin:sciapplications_scienceapplication_changelist'), + data={'action': 'port', '_selected_action': [str(app.pk) for app in self.apps]}, + follow=True + ) + # If the PI is not registered, then 2 emails will be sent out- one inviting them to register an account, and + # one letting them know their proposal has been approved. The accepted email is sent second. + self.assertEqual(len(mail.outbox), 2) + # Proposal invitation email + self.assertEqual([non_registered_email], mail.outbox[0].to) + self.assertIn('Please use the following link to register your account', str(mail.outbox[0].message())) + # Proposal approved email + self.assertEqual([non_registered_email], mail.outbox[1].to) + self.assertIn(ScienceApplication.objects.get(pk=app_id_to_port).proposal.id, str(mail.outbox[1].message())) + self.assertIn(ScienceApplication.objects.get(pk=app_id_to_port).call.semester.id, str(mail.outbox[1].message())) + def test_port_not_accepted(self): self.client.post( reverse('admin:sciapplications_scienceapplication_changelist'),