#136: Make improvements as per PR for this issue

common/backend.py

@@ -97,32 +97,33 @@ def count_with_filters(self, session_id, entity_type, filters):
         pass
 
     @abstractmethod
-    def get_with_id(self, session_id, entity_type, id):
+    def get_with_id(self, session_id, entity_type, id_):
         """
         Gets the entity matching the given ID for the given entity type
         :param session_id: The session id of the requesting user
         :param entity_type: The type of entity
-        :param id: the id of the record to find
+        :param id_: the id of the record to find
         :return: the entity retrieved
         """
         pass
 
     @abstractmethod
-    def delete_with_id(self, session_id, entity_type, id):
+    def delete_with_id(self, session_id, entity_type, id_):
         """
         Deletes the row matching the given ID for the given entity type
         :param session_id: The session id of the requesting user
         :param table: the table to be searched
-        :param id: the id of the record to delete
+        :param id_: the id of the record to delete
         """
         pass
 
     @abstractmethod
-    def update_with_id(self, session_id, entity_type, id, data):
+    def update_with_id(self, session_id, entity_type, id_, data):
         """
         Updates the row matching the given ID for the given entity type
         :param session_id: The session id of the requesting user
         :param entity_type: The type of entity
+        :param id_: the id of the record to update
         :param data: The dictionary that the entity should be updated with
         :return: The updated entity.
         """

common/database_backend.py

@@ -67,18 +67,18 @@ def count_with_filters(self, session_id, table, filters):
 
     @requires_session_id
     @queries_records
-    def get_with_id(self, session_id, table, id):
-        return get_row_by_id(table, id).to_dict()
+    def get_with_id(self, session_id, table, id_):
+        return get_row_by_id(table, id_).to_dict()
 
     @requires_session_id
     @queries_records
-    def delete_with_id(self, session_id, table, id):
-        return delete_row_by_id(table, id)
+    def delete_with_id(self, session_id, table, id_):
+        return delete_row_by_id(table, id_)
 
     @requires_session_id
     @queries_records
-    def update_with_id(self, session_id, table, id, data):
-        return update_row_from_id(table, id, data)
+    def update_with_id(self, session_id, table, id_, data):
+        return update_row_from_id(table, id_, data)
 
     @requires_session_id
     @queries_records

common/database_helpers.py

@@ -68,7 +68,7 @@ def commit_changes(self):
         """
         Commits all changes to the database and closes the session
         """
-        log.info(f" Commiting changes to {self.table}")
+        log.info(" Committing changes to %s", self.table)
         self.session.commit()
 
 
@@ -146,7 +146,7 @@ def __init__(self, table, row, new_values):
         self.new_values = new_values
 
     def execute_query(self):
-        log.info(f" Updating row in {self.table}")
+        log.info(" Updating row in %s", self.table)
         self.row.update_from_dict(self.new_values)
         self.session.add(self.row)
         self.commit_changes()
@@ -159,7 +159,7 @@ def __init__(self, table, row):
         self.row = row
 
     def execute_query(self):
-        log.info(f" Deleting row {self.row} from {self.table.__tablename__}")
+        log.info(" Deleting row %s from %s", self.row, self.table.__tablename__)
         self.session.delete(self.row)
         self.commit_changes()
 
@@ -394,40 +394,40 @@ def create_rows_from_json(table, data):
     return create_row_from_json(table, data)
 
 
-def get_row_by_id(table, id):
+def get_row_by_id(table, id_):
     """
     Gets the row matching the given ID from the given table, raises MissingRecordError if it can not be found
     :param table: the table to be searched
-    :param id: the id of the record to find
+    :param id_: the id of the record to find
     :return: the record retrieved
     """
     with ReadQuery(table) as read_query:
-        log.info(f" Querying {table.__tablename__} for record with ID: {id}")
-        where_filter = WhereFilter("ID", id, "eq")
+        log.info(" Querying %s for record with ID: %d", table.__tablename__, id_)
+        where_filter = WhereFilter("ID", id_, "eq")
         where_filter.apply_filter(read_query)
         return read_query.get_single_result()
 
 
-def delete_row_by_id(table, id):
+def delete_row_by_id(table, id_):
     """
     Deletes the row matching the given ID from the given table, raises MissingRecordError if it can not be found
     :param table: the table to be searched
-    :param id: the id of the record to delete
+    :param id_: the id of the record to delete
     """
-    log.info(f" Deleting row from {table.__tablename__} with ID: {id}")
-    row = get_row_by_id(table, id)
+    log.info(" Deleting row from %s with ID: %d", table.__tablename__, id_)
+    row = get_row_by_id(table, id_)
     with DeleteQuery(table, row) as delete_query:
         delete_query.execute_query()
 
 
-def update_row_from_id(table, id, new_values):
+def update_row_from_id(table, id_, new_values):
     """
     Updates a record in a table
     :param table: The table the record is in
-    :param id: The id of the record
+    :param id_: The id of the record
     :param new_values: A JSON string containing what columns are to be updated
     """
-    row = get_row_by_id(table, id)
+    row = get_row_by_id(table, id_)
     with UpdateQuery(table, row, new_values) as update_query:
         update_query.execute_query()
 
@@ -496,7 +496,7 @@ def get_first_filtered_row(table, filters):
     :param filters: the filter to be applied to the query
     :return: the first row matching the filter
     """
-    log.info(f" Getting first filtered row for {table.__tablename__}")
+    log.info(" Getting first filtered row for %s", table.__tablename__)
     return get_rows_by_filter(table, filters)[0]
 
 
@@ -508,7 +508,7 @@ def get_filtered_row_count(table, filters):
     :return: int: the count of the rows
     """
 
-    log.info(f" getting count for {table.__tablename__}")
+    log.info(" getting count for %s", table.__tablename__)
     with CountQuery(table) as count_query:
         filter_handler = FilterOrderHandler()
         filter_handler.add_filters(filters)
@@ -523,7 +523,7 @@ def patch_entities(table, json_list):
     :param json_list: the list of updated values or a dictionary
     :return: The list of updated rows.
     """
-    log.info(f" Patching entities in {table.__tablename__}")
+    log.info(" Patching entities in %s", table.__tablename__)
     results = []
     if type(json_list) is dict:
         for key in json_list:

common/python_icat_backend.py

@@ -80,18 +80,18 @@ def count_with_filters(self, session_id, table, filters):
 
     @requires_session_id
     @queries_records
-    def get_with_id(self, session_id, table, id):
-        return get_entity_by_id(self.client, table.__name__, id, True)
+    def get_with_id(self, session_id, table, id_):
+        return get_entity_by_id(self.client, table.__name__, id_, True)
 
     @requires_session_id
     @queries_records
-    def delete_with_id(self, session_id, table, id):
-        return delete_entity_by_id(self.client, table.__name__, id)
+    def delete_with_id(self, session_id, table, id_):
+        return delete_entity_by_id(self.client, table.__name__, id_)
 
     @requires_session_id
     @queries_records
-    def update_with_id(self, session_id, table, id, data):
-        return update_entity_by_id(self.client, table.__name__, id, data)
+    def update_with_id(self, session_id, table, id_, data):
+        return update_entity_by_id(self.client, table.__name__, id_, data)
 
     @requires_session_id
     @queries_records

common/python_icat_helpers.py

@@ -25,7 +25,7 @@ def wrapper_requires_session(*args, **kwargs):
             client = args[0].client
             # Find out if session has expired
             session_time = client.getRemainingMinutes()
-            log.info(f"Session time: {session_time}")
+            log.info("Session time: %d", session_time)
             if session_time < 0:
                 raise AuthenticationError("Forbidden")
             else:
@@ -127,8 +127,9 @@ def execute_icat_query(client, query, return_json_formattable=False):
             dict_result = result.as_dict()
             for key, value in dict_result.items():
                 # Convert datetime objects to strings so they can be JSON serialisable
-                if isinstance(dict_result[key], datetime):
-                    dict_result[key] = str(dict_result[key])
+                if isinstance(value, datetime):
+                    # Remove timezone data which isn't utilised in ICAT and convert to ISO format
+                    dict_result[key] = value.replace(tzinfo=None).isoformat(' ')
 
             data.append(dict_result)
         return data
@@ -206,8 +207,8 @@ def str_to_date_object(icat_attribute, data):
         `accepted_date_format`
     """
 
-    log.debug(f"ICAT Attribute: {icat_attribute}, Type: {type(icat_attribute)}")
-    log.debug(f"Data: {data}, Type: {type(data)}")
+    log.debug("ICAT Attribute: %s, Type: %s", icat_attribute, type(icat_attribute))
+    log.debug("Data: %s, Type: %s", data, type(data))
 
     accepted_date_format = "%Y-%m-%d %H:%M:%S"
 
@@ -246,16 +247,16 @@ def update_attributes(object, dictionary):
     object.update()
 
 
-def get_entity_by_id(client, table_name, id, return_json_formattable_data):
+def get_entity_by_id(client, table_name, id_, return_json_formattable_data):
     """
     Gets a record of a given ID from the specified entity
 
     :param client: ICAT client containing an authenticated user
     :type client: :class:`icat.client.Client`
     :param table_name: Table name to extract which entity to use
     :type table_name: :class:`str`
-    :param id: ID number of the entity to retrieve
-    :type id: :class:`int`
+    :param id_: ID number of the entity to retrieve
+    :type id_: :class:`int`
     :param return_json_formattable_data: Flag to determine whether the data should be returned as a
         list of data ready to be converted straight to JSON (i.e. if the data will be used as a
         response for an API call) or whether to leave the data in a Python ICAT format
@@ -265,57 +266,57 @@ def get_entity_by_id(client, table_name, id, return_json_formattable_data):
     """
 
     # Set query condition for the selected ID
-    id_condition = create_condition('id', '=', id)
+    id_condition = create_condition('id', '=', id_)
 
     selected_entity_name = get_python_icat_entity_name(client, table_name)
 
     id_query = construct_icat_query(client, selected_entity_name, conditions=id_condition, includes="1")
     entity_by_id_data = execute_icat_query(client, id_query, return_json_formattable_data)
 
-    if entity_by_id_data == []:
+    if not entity_by_id_data:
         # Cannot find any data matching the given ID
         raise MissingRecordError("No result found")
+    else: 
+        return entity_by_id_data[0]
 
-    return entity_by_id_data
 
-
-def delete_entity_by_id(client, table_name, id):
+def delete_entity_by_id(client, table_name, id_):
     """
     Deletes a record of a given ID of the specified entity
 
     :param client: ICAT client containing an authenticated user
     :type client: :class:`icat.client.Client`
     :param table_name: Table name to extract which entity to delete
     :type table_name: :class:`str`
-    :param id: ID number of the entity to delete
-    :type id: :class:`int`
+    :param id_: ID number of the entity to delete
+    :type id_: :class:`int`
     """
 
-    entity_id_data = get_entity_by_id(client, table_name, id, False)
-    client.delete(entity_id_data[0])
+    entity_id_data = get_entity_by_id(client, table_name, id_, False)
+    client.delete(entity_id_data)
 
 
-def update_entity_by_id(client, table_name, id, new_data):
+def update_entity_by_id(client, table_name, id_, new_data):
     """
     Gets a record of a given ID of the specified entity
 
     :param client: ICAT client containing an authenticated user
     :type client: :class:`icat.client.Client`
     :param table_name: Table name to extract which entity to use
     :type table_name: :class:`str`
-    :param id: ID number of the entity to retrieve
-    :type id: :class:`int`
+    :param id_: ID number of the entity to retrieve
+    :type id_: :class:`int`
     :param new_data: JSON from request body providing new data to update the record with the
         specified ID
     :return: The updated record of the specified ID from the given entity
     """
 
-    entity_id_data = get_entity_by_id(client, table_name, id, False)
+    entity_id_data = get_entity_by_id(client, table_name, id_, False)
     # There will only ever be one record associated with a single ID - if a record with the
     # specified ID cannot be found, it'll be picked up by the MissingRecordError in 
     # get_entity_by_id()
-    update_attributes(entity_id_data[0], new_data)
+    update_attributes(entity_id_data, new_data)
 
     # The record is re-obtained from Python ICAT (rather than using entity_id_data) to show to the
     # user whether the change has actually been applied
-    return get_entity_by_id(client, table_name, id, True)
+    return get_entity_by_id(client, table_name, id_, True)

src/main.py

@@ -57,7 +57,7 @@ def handle_error(e):
     get_id_endpoint_resource = get_id_endpoint(
         entity_name, endpoints[entity_name])
     api.add_resource(get_id_endpoint_resource,
-                     f"/{entity_name.lower()}/<int:id>")
+                     f"/{entity_name.lower()}/<int:id_>")
     spec.path(resource=get_id_endpoint_resource, api=api)
 
     get_count_endpoint_resource = get_count_endpoint(

src/resources/entities/entity_endpoint.py

@@ -149,8 +149,8 @@ def get_id_endpoint(name, table):
     """
     class EndpointWithID(Resource):
 
-        def get(self, id):
-            return backend.get_with_id(get_session_id_from_auth_header(), table, id), 200
+        def get(self, id_):
+            return backend.get_with_id(get_session_id_from_auth_header(), table, id_), 200
 
         get.__doc__ = f"""
             ---
@@ -182,9 +182,9 @@ def get(self, id):
                     description: No such record - Unable to find a record in the database
             """
 
-        def delete(self, id):
+        def delete(self, id_):
             backend.delete_with_id(
-                get_session_id_from_auth_header(), table, id)
+                get_session_id_from_auth_header(), table, id_)
             return "", 204
 
         delete.__doc__ = f"""
@@ -213,10 +213,10 @@ def delete(self, id):
                     description: No such record - Unable to find a record in the database
             """
 
-        def patch(self, id):
+        def patch(self, id_):
             session_id = get_session_id_from_auth_header()
-            backend.update_with_id(session_id, table, id, request.json)
-            return backend.get_with_id(session_id, table, id), 200
+            backend.update_with_id(session_id, table, id_, request.json)
+            return backend.get_with_id(session_id, table, id_), 200
 
         patch.__doc__ = f"""
             ---

src/resources/table_endpoints/table_endpoints.py

@@ -7,7 +7,7 @@
 
 
 class InstrumentsFacilityCycles(Resource):
-    def get(self, id):
+    def get(self, id_):
         """
         ---
         summary: Get an Instrument's FacilityCycles 
@@ -45,11 +45,11 @@ def get(self, id):
             404:
                 description: No such record - Unable to find a record in the database
         """
-        return backend.get_facility_cycles_for_instrument(get_session_id_from_auth_header(), id, get_filters_from_query_string()), 200
+        return backend.get_facility_cycles_for_instrument(get_session_id_from_auth_header(), id_, get_filters_from_query_string()), 200
 
 
 class InstrumentsFacilityCyclesCount(Resource):
-    def get(self, id):
+    def get(self, id_):
         """
         ---
         summary: Count an Instrument's FacilityCycles 
@@ -81,7 +81,7 @@ def get(self, id):
             404:
                 description: No such record - Unable to find a record in the database
         """
-        return backend.get_facility_cycles_for_instrument_count(get_session_id_from_auth_header(), id, get_filters_from_query_string()), 200
+        return backend.get_facility_cycles_for_instrument_count(get_session_id_from_auth_header(), id_, get_filters_from_query_string()), 200
 
 
 class InstrumentsFacilityCyclesInvestigations(Resource):