global: several improvements

* Rewrites error detection for request and response message.
* Improves logging.
* Ensures that the sequence number is present in the message if the selfchek
  terminal sends it.
* Adds sequence number and checksum in dumped message.
* Adds cached property to extension.
* Fixes circulation date parsing.

Co-Authored-by: Lauren-D <[email protected]>

invenio_sip2/actions/actions.py

@@ -23,7 +23,8 @@
 
 from invenio_sip2.api import Message
 
-from ..actions.base import Action, check_selfcheck_authentication
+from ..actions.base import Action
+from ..decorators import add_sequence_number, check_selfcheck_authentication
 from ..errors import SelfcheckCirculationError
 from ..handlers import authorize_patron_handler, checkin_handler, \
     checkout_handler, enable_patron_handler, hold_handler, item_handler, \
@@ -39,6 +40,7 @@
 class SelfCheckLogin(Action):
     """Action to selfcheck login."""
 
+    @add_sequence_number
     def execute(self, message, **kwargs):
         """Execute action."""
         selfcheck_login = message.get_field_value('login_uid')
@@ -71,6 +73,7 @@ class AutomatedCirculationSystemStatus(Action):
     """Action to get status from automated circulation system."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client):
         """Execute action."""
         # TODO : calculate system status from remote app
@@ -109,11 +112,17 @@ def execute(self, message, client):
 class RequestResend(Action):
     """Action to resend last message."""
 
-    def __init__(self, command):
+    def __init__(self, command, message):
         """Init action object."""
         self.command = command
+        self.message = message
         self.validate_action()
 
+    def __str__(self):
+        """String representation of Action class."""
+        return f'{self.__class__.__name__}() message:{self.message}, ' \
+               f'request:{self.command}'
+
     @check_selfcheck_authentication
     def execute(self, message, client):
         """Execute action."""
@@ -127,6 +136,7 @@ class PatronEnable(Action):
     """Action to enable patron on automated circulation system."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client):
         """Execute action."""
         patron_id = message.get_field_value('patron_id')
@@ -181,6 +191,7 @@ class PatronStatus(Action):
     """Action to get patron status from automated circulation system."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client):
         """Execute action."""
         patron_id = message.get_field_value('patron_id')
@@ -209,6 +220,7 @@ class PatronInformation(Action):
     """Action to get patron information from automated circulation system."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client):
         """Execute action."""
         patron_id = message.get_field_value('patron_id')
@@ -272,6 +284,7 @@ class EndPatronSession(Action):
     """Action to end patron session on automated circulation system."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client):
         """Execute action."""
         client.clear_patron_session()
@@ -293,6 +306,7 @@ class ItemInformation(Action):
     """Action to get item information from automated circulation system."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client):
         """Execute action."""
         patron_session = client.get_current_patron_session()
@@ -336,6 +350,7 @@ class BlockPatron(Action):
     """Action to block patron."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client, **kwargs):
         """Execute action."""
         # TODO: implements action
@@ -346,6 +361,7 @@ class Checkin(Action):
     """Action to checkin an item."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client, **kwargs):
         """Execute checkin action."""
         patron_session = client.get_current_patron_session()
@@ -397,6 +413,7 @@ class Checkout(Action):
     """Action to checkout an item."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client, **kwargs):
         """Execute checkout action."""
         patron_session = client.get_current_patron_session()
@@ -450,6 +467,7 @@ class FeePaid(Action):
     """Action to paid fee."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client, **kwargs):
         """Execute action."""
         # TODO: implements action
@@ -460,6 +478,7 @@ class Hold(Action):
     """Action to hold an item."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client, **kwargs):
         """Execute hold action."""
         patron_session = client.get_current_patron_session()
@@ -506,6 +525,7 @@ class Renew(Action):
     """Action to renew an item."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client, **kwargs):
         """Execute checkout action."""
         patron_session = client.get_current_patron_session()
@@ -556,6 +576,7 @@ class RenewAll(Action):
     """Action to renew all items."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client, **kwargs):
         """Execute action."""
         # TODO: implements action
@@ -566,6 +587,7 @@ class ItemStatusUpdate(Action):
     """Action to update item status."""
 
     @check_selfcheck_authentication
+    @add_sequence_number
     def execute(self, message, client, **kwargs):
         """Execute action."""
         # TODO: implements action

invenio_sip2/actions/base.py

@@ -19,30 +19,16 @@
 
 from __future__ import absolute_import, print_function
 
-from functools import wraps
-
 from ..api import Message
 from ..proxies import current_sip2 as acs_system
 
 
-def check_selfcheck_authentication(func):
-    """Decorator to check authentication of selfcheck client."""
-
-    @wraps(func)
-    def inner(*args, **kwargs):
-        client = kwargs.pop('client')
-        # TODO: maybe we can always call remote api to authenticate client
-        if client and client.is_authenticated:
-            return func(*args, client)
-
-    return inner
-
-
 class Action(object):
     """An action object used for automated circulation system."""
 
-    def __init__(self, command, response, **kwargs):
+    def __init__(self, command, response, message, **kwargs):
         """Init action object."""
+        self.message = message
         self.command = command
         self.response_type = acs_system.sip2_message_types.get_by_command(
             response
@@ -87,3 +73,8 @@ def prepare_message_response(self, **kwargs):
     def execute(self, **kwargs):
         """Execute actions."""
         raise NotImplementedError()
+
+    def __str__(self):
+        """String representation of Action class."""
+        return f'{self.__class__.__name__}() message:{self.message}, ' \
+               f'request:{self.command}, response:{self.response_type.command}'

invenio_sip2/api.py

@@ -27,6 +27,7 @@
 from .helpers import MessageTypeFixedField, MessageTypeVariableField
 from .models import SelfcheckLanguage
 from .proxies import current_sip2 as acs_system
+from .utils import generate_checksum
 
 
 def preprocess_field_value(func):
@@ -60,7 +61,7 @@ def __init__(self, field=None, field_value=''):
 
     def __str__(self):
         """String representation of FieldMessage object."""
-        return self.field.field_id + (self.field_value or '') + '|'
+        return self.field.field_id + (self.field_value or '')
 
 
 class FixedFieldMessage(FieldMessage):
@@ -78,6 +79,9 @@ def __init__(self, **kwargs):
         """Constructor."""
         self.variable_fields = []
         self.fixed_fields = []
+        self.checksum = None
+        self.sequence_number = None
+        self.line_terminator = acs_system.line_terminator
 
         for key, value in kwargs.items():
             setattr(self, key, value)
@@ -97,11 +101,20 @@ def __str__(self):
         new_message = self.command
 
         for fixed_field in self.fixed_fields:
-            new_message = new_message + str(fixed_field)
-
-        for variable_field in self.variable_fields:
-            new_message = new_message + str(variable_field)
-
+            new_message += str(fixed_field)
+
+        for idx, variable_field in enumerate(self.variable_fields):
+            if idx != 0:
+                new_message += '|'
+            new_message += str(variable_field)
+
+        if acs_system.is_error_detection_enabled:
+            if self.sequence_number:
+                new_message += f'AY{self.sequence_number}'
+            if not self.checksum:
+                self.checksum = generate_checksum(new_message)
+            new_message += f'AZ{self.checksum}'
+        new_message += self.line_terminator
         self.message_text = new_message
         return self.message_text
 
@@ -130,42 +143,37 @@ def summary(self):
         """Shortcut for sip2 summary."""
         return self.get_fixed_field_value('summary')
 
-    @property
-    def sequence_number(self):
-        """Get the sequence number."""
-        return self.sequence
-
     def _parse_request(self):
         """Parse the request sended by the selfcheck."""
-        if current_app.config.get('SIP2_ERROR_DETECTION'):
-            # extract sequence number and checksum
-            self.sequence = self.message_text[-7:-6]
-            self.checksum = self.message_text[-4:]
-            self.variable_fields.append(FieldMessage(
-                MessageTypeVariableField.find_by_field_id('AY'),
-                self.sequence))
-            self.variable_fields.append(FieldMessage(
-                MessageTypeVariableField.find_by_field_id('AZ'),
-                self.checksum))
-
-        # get remaining parts of the message
-        txt = self.message_text[2:-9]
+        txt = self.message_text[2:]
+        # try to extract sequence number and checksum
+        error_txt = txt[-9:]
+        field_sequence_id = error_txt[:2]
+        field_checksum_id = error_txt[-6:-4]
+        if field_sequence_id == 'AY':
+            # process sequence_number
+            self.sequence_number = error_txt[2:3]
+        if field_checksum_id == 'AZ':
+            self.checksum = error_txt[-4:]
+        if acs_system.is_error_detection_enabled and self.sequence_number \
+                and self.checksum:
+            txt = txt[:-9]
 
         # extract fixed fields from request
         for fixed_field in self.message_type.fixed_fields:
             # get fixed field value
             value = txt[:fixed_field.length]
             self.fixed_fields.append(FixedFieldMessage(fixed_field, value))
             txt = txt[fixed_field.length:]
-
         if not txt:
             return
 
-        # extract variable fields from request
         for part in filter(None, txt.split('|')):
-            field = MessageTypeVariableField.find_by_field_id(part[:2])
+            field_id = part[:2]
+            field_value = part[2:]
+            field = MessageTypeVariableField.find_by_field_id(field_id)
             if field is not None:
-                self.variable_fields.append(FieldMessage(field, part[2:]))
+                self.variable_fields.append(FieldMessage(field, field_value))
 
     def get_fixed_field_by_name(self, field_name):
         """Get the FixedFieldMessage object by field name."""
@@ -243,14 +251,14 @@ def add_fixed_field(self, field, field_value):
 
     def dumps(self):
         """Dumps message as dict."""
-        data = {}
+        data = {
+            '_sip2': str(self),
+            'message_type': {
+                'command': self.message_type.command,
+                'label': self.message_type.label
+            }}
         # TODO: `_sip2` field is only use in backend. Try to mask it on
         #       view or logging
-        data['_sip2'] = str(self)
-        data['message_type'] = {
-            'command': self.message_type.command,
-            'label': self.message_type.label
-        }
         for fixed_field in self.fixed_fields:
             data[fixed_field.field.field_id] = fixed_field.field_value
 
@@ -265,4 +273,8 @@ def dumps(self):
                 else:
                     data[variable_field.field.name] = \
                         variable_field.field_value
+        if self.sequence_number:
+            data['sequence_number'] = self.sequence_number
+        if self.checksum:
+            data['checksum'] = self.checksum
         return data

invenio_sip2/config.py

@@ -221,26 +221,27 @@
 
 # Define message action.
 SIP2_MESSAGE_ACTIONS = {
-    '01': dict(response='24', action=BlockPatron),
-    '09': dict(message="checkin", response='10', action=Checkin),
-    '11': dict(message="checkout", response='12', action=Checkout),
-    '15': dict(response='16', action=Hold),
-    '17': dict(message="item_information", response='18',
+    '01': dict(message='block_patron', response='24', action=BlockPatron),
+    '09': dict(message='checkin', response='10', action=Checkin),
+    '11': dict(message='checkout', response='12', action=Checkout),
+    '15': dict(message='hold', response='16', action=Hold),
+    '17': dict(message='item_information', response='18',
                action=ItemInformation),
-    '19': dict(response='20', action=ItemStatusUpdate),
-    '23': dict(message="patron_request_status", response='24',
+    '19': dict(message='item_status_update', response='20',
+               action=ItemStatusUpdate),
+    '23': dict(message='patron_request_status', response='24',
                action=PatronStatus),
-    '25': dict(message="patron_enable", response='26', action=PatronEnable),
-    '29': dict(response='30', action=Renew),
-    '35': dict(message="end_patron_session", response='36',
+    '25': dict(message='patron_enable', response='26', action=PatronEnable),
+    '29': dict(message='renew', response='30', action=Renew),
+    '35': dict(message='end_patron_session', response='36',
                action=EndPatronSession),
-    '37': dict(response='38', action=FeePaid),
-    '63': dict(message="patron_information", response='64',
+    '37': dict(message='fee_paid', response='38', action=FeePaid),
+    '63': dict(message='patron_information', response='64',
                action=PatronInformation),
-    '65': dict(response='66', action=RenewAll),
-    '93': dict(message="login", response='94', action=SelfCheckLogin),
-    '97': dict(action=RequestResend),
-    '99': dict(message="sc_status", response='98',
+    '65': dict(message='renew_all', response='66', action=RenewAll),
+    '93': dict(message='login', response='94', action=SelfCheckLogin),
+    '97': dict(message='request_resend', action=RequestResend),
+    '99': dict(message='sc_status', response='98',
                action=AutomatedCirculationSystemStatus),
 }