Enhance syncing of BILL accounts

teknologr/api/bill.py

@@ -1,92 +1,88 @@
 import requests
 import re
 import json
+import html
 from getenv import env
 
+# All BILL accounts are connected to a specific LDAP username.
+# Let's use this fact to connect Members to BILL accounts,
+# instead of storing the account number separately.
 
 class BILLException(Exception):
     pass
 
-
-class BILLAccountManager:
-    ERROR_ACCOUNT_DOES_NOT_EXIST = "BILL account does not exist"
-
-    def __init__(self):
-        self.api_url = env("BILL_API_URL")
-        self.user = env("BILL_API_USER")
-        self.password = env("BILL_API_PW")
-
-    def admin_url(self, bill_code):
-        if not self.api_url:
-            return ''
-        return f'{"/".join(self.api_url.split("/")[:-2])}/admin/userdata?id={bill_code}'
-
-    def __request(self, path):
-        try:
-            r = requests.post(self.api_url + path, auth=(self.user, self.password))
-        except:
-            raise BILLException("Could not connect to BILL server")
-
-        if r.status_code != 200:
-            raise BILLException(f"BILL returned status code {r.status_code}")
-
-        # Not a number, return as text
-        try:
-            number = int(r.text)
-        except ValueError:
-            return r.text
-
-        # A negative number means an error code
-        if number == -3:
-            raise BILLException(BILLAccountManager.ERROR_ACCOUNT_DOES_NOT_EXIST)
-        if number < 0:
-            raise BILLException(f"BILL returned error code: {number}")
-
-        return number
-
-    def create_bill_account(self, username):
-        if not re.search(r'^[A-Za-z0-9]+$', username):
-            raise BILLException("Can not create a BILL account using an LDAP username containing anything other than letters and numbers")
-
-        result = self.__request(f"add?type=user&id={username}")
-        if type(result) == int:
-            return result
+ERROR_NO_BILL = BILLException("BILL is not set up")
+ERROR_ACCOUNT_DOES_NOT_EXIST = BILLException("BILL account does not exist")
+
+def admin_url(bill_id):
+    api_url = env("BILL_API_URL")
+    if not api_url:
+        raise ERROR_NO_BILL
+    return f'{"/".join(api_url.split("/")[:-2])}/admin/userdata?id={bill_id}'
+
+def __request(path):
+    api_url = env("BILL_API_URL")
+    user = env("BILL_API_USER")
+    password = env("BILL_API_PW")
+    if not all([api_url, user, password]):
+        raise ERROR_NO_BILL
+
+    try:
+        r = requests.post(api_url + path, auth=(user, password))
+    except:
+        raise BILLException("Could not connect to BILL server")
+
+    if r.status_code != 200:
+        raise BILLException(f"BILL returned status code: {r.status_code}")
+
+    # The response can be anything, and it's up to the caller to parse it correctly.
+    # If not a number, return as text.
+    try:
+        number = int(r.text)
+    except ValueError:
+        # BILL uses HTML ecoding to represent non-ASCII characters
+        return html.unescape(r.text)
+
+    # A negative number means an error code
+    if number == -3:
+        raise ERROR_ACCOUNT_DOES_NOT_EXIST
+    if number < 0:
+        raise BILLException(f"BILL returned error code: {number}")
+
+    # A non-negative number is a valid response
+    return number
+
+def create_account(username):
+    if not re.search(r'^[A-Za-z0-9]+$', username):
+        raise BILLException("Can not create a BILL account using an LDAP username containing anything other than letters and numbers")
+
+    result = __request(f"add?type=user&id={username}")
+    if type(result) == int:
+        return result
+    raise BILLException(f"BILL returned error: {result}")
+
+def delete_account(username):
+    # If the BILL account does not exist all is ok
+    if not get_account(username):
+        return
+
+    result = __request(f"del?type=user&id={username}")
+    if result != 0:
         raise BILLException(f"BILL returned error: {result}")
 
-    def delete_bill_account(self, bill_code):
-        info = self.get_account_by_code(bill_code)
-
-        # If the BILL account does not exist all is ok
-        if not info:
-            return
-
-        result = self.__request(f"del?type=user&acc={bill_code}")
-
-        if result != 0:
-            raise BILLException(f"BILL returned error: {result}")
-
-    def get_account_by_username(self, username):
-        '''
-        Get the info for a certain BILL account. Returns None if the account does not exist.
-        '''
-        try:
-            result = self.__request(f"get?type=user&id={username}")
-            return json.loads(result)
-        except BILLException as e:
-            s = str(e)
-            if s == BILLAccountManager.ERROR_ACCOUNT_DOES_NOT_EXIST:
-                return None
-            raise e
-
-    def get_account_by_code(self, bill_code):
-        '''
-        Get the info for a certain BILL account. Returns None if the account does not exist.
-        '''
-        try:
-            result = self.__request(f"get?type=user&acc={bill_code}")
-            return json.loads(result)
-        except BILLException as e:
-            s = str(e)
-            if s == BILLAccountManager.ERROR_ACCOUNT_DOES_NOT_EXIST:
-                return None
-            raise e
+def get_account(username):
+    '''
+    Get the info for the BILL account connected to a certain LDAP username.
+    Returns None if the account does not exist.
+    '''
+    try:
+        result = __request(f"get?type=user&id={username}")
+        info = json.loads(result)
+        info['acc'] = int(info['acc'])
+        # Balance is a float, but leave it as a string to avoid having to format it later
+        # info['balance'] = float(info['balance'])
+        return info
+    except BILLException as e:
+        if e == ERROR_ACCOUNT_DOES_NOT_EXIST:
+            return None
+        raise e

teknologr/api/filters.py

@@ -131,6 +131,7 @@ class MemberFilter(BaseFilter):
 
     # Staff only filters
     STAFF_ONLY = Member.STAFF_ONLY_FIELDS
+    STAFF_ONLY.remove('bill_code') # No longer cached, so can not filter on it
     birth_date = django_filters.DateFromToRangeFilter(
         label='Född mellan',
     )
@@ -156,9 +157,6 @@ class MemberFilter(BaseFilter):
     username = CharFilterWithKeywords(
         label='Användarnamn',
     )
-    bill_code = CharFilterWithKeywords(
-        label='BILL-konto',
-    )
 
     def includes_hidable_field(self):
         ''' Check if the current query includes filtering on any hidable Member fields '''
@@ -332,7 +330,6 @@ class ApplicantFilter(MemberFilter):
     graduated_year = None
     comment = None
     dead = None
-    bill_code = None
     created = None
     modified = None
     n_functionaries = None

teknologr/api/serializers.py

@@ -53,6 +53,10 @@ class Meta:
     def to_representation(self, instance):
         data = super().to_representation(instance)
 
+        # Hide bill_code by default, because it is no longer cached in Member
+        if 'bill_code' in data:
+            data.pop('bill_code')
+
         hide = not self.is_staff and not instance.show_contact_info()
         if hide:
             for field in Member.HIDABLE_FIELDS:
@@ -61,6 +65,12 @@ def to_representation(self, instance):
         # Add the actual related objects if detail view
         # XXX: Do we need to prefetch all related objects here? It's now done earlier, by the caller...
         if self.detail:
+            if self.is_staff:
+                # Fetch and add BILL id on detail view only
+                # XXX: 'bill_code' is the wrong term
+                bill_info = instance.get_bill_info() or {}
+                data['bill_code'] = bill_info.get('acc')
+
             data['decorations'] = [{
                 'decoration': {'id': do.decoration.id, 'name': do.decoration.name},
                 'acquired': do.acquired,

teknologr/api/tests.py

@@ -283,7 +283,8 @@ def test_post_for_superuser(self):
     'allow_studentbladet': bool,
     'comment': str,
     'username': (str, None),
-    'bill_code': (str, None),
+    # No longer cached in Member, only fetched for detail view
+    # 'bill_code': (str, None),
 }
 MEMBER_DETAIL = {
     'functionaries': [{
@@ -326,6 +327,7 @@ def test_post_for_superuser(self):
         'begin_date': str,
         'end_date': (str, None),
     }],
+    'bill_code': (int, None),
 }
 
 class MembersAPITest(BaseAPITest, GetAllMethodTests, PostMethodTests):

teknologr/api/tests_filter.py

@@ -33,7 +33,7 @@ def setUp(self):
             allow_publish_info=True,
             comment='Dummy',
             username='dummyd1',
-            bill_code=42,
+            # bill_code=42,
         )
 
     def login_user(self):
@@ -847,6 +847,7 @@ def setUp(self):
         )
 
 
+'''
 class MemberFilterBillCodeTest(BaseAPITest, TestCases):
     def setUp(self):
         super().setUp()
@@ -872,3 +873,4 @@ def setUp(self):
             allow_publish_info=True,
             dead=False,
         )
+'''

teknologr/api/views.py

@@ -15,7 +15,7 @@
 from api.serializers import *
 from api.filters import *
 from api.ldap import LDAPAccountManager, LDAPError_to_string
-from api.bill import BILLAccountManager, BILLException
+import api.bill as bill
 from api.utils import assert_public_member_fields
 from api.mailutils import mailNewPassword, mailNewAccount
 from members.models import GroupMembership, Member, Group
@@ -272,7 +272,7 @@ def post(self, request, member_id):
             return HttpResponse('Username or password field missing', status=400)
 
         if member.username:
-            return HttpResponse('Member already has an LDAP account', status=400)
+            return HttpResponse('Member already has a username', status=400)
 
         if Member.objects.filter(username=username).exists():
             return HttpResponse(f'Username "{username}" is already taken', status=400)
@@ -299,8 +299,9 @@ def delete(self, request, member_id):
         member = get_object_or_404(Member, id=member_id)
 
         if not member.username:
-            return HttpResponse('Member has no LDAP account', status=400)
-        if member.bill_code:
+            return HttpResponse('Member has no username', status=400)
+
+        if member.get_bill_info():
             return HttpResponse('BILL account must be deleted first', status=400)
 
         try:
@@ -380,58 +381,44 @@ def change_ldap_password(request, member_id):
 class BILLAccountView(APIView):
     def get(self, request, member_id):
         member = get_object_or_404(Member, id=member_id)
+
+        if not member.username:
+            return Response({'detail': 'username missing'}, status=404)
+
         try:
-            account = BILLAccountManager().get_account_by_code(member.bill_code)
-        except Exception as e:
-            return Response({'detail': str(e)}, status=500)
-        if not account:
-            return Response({'detail': 'Could not find BILL account.'}, status=404)
-        return Response(account)
+            info = bill.get_account(member.username)
+            if info:
+                return Response(info)
+        except bill.BILLException as e:
+            return Response({'detail': str(e)}, status=404)
+        return Response({'detail': 'No BILL account found'}, status=404)
 
     def post(self, request, member_id):
         member = get_object_or_404(Member, id=member_id)
 
-        if member.bill_code:
-            return HttpResponse('Member already has a BILL account', status=400)
         if not member.username:
-            return HttpResponse('LDAP account missing', status=400)
+            return HttpResponse('username missing', status=400)
 
-        bm = BILLAccountManager()
-        bill_code = None
-
-        # Check if there already is a BILL account with this LDAP name
+        # Check if the Member already has a BILL account before creating a new one
         try:
-            bill_code = bm.get_account_by_username(member.username).get('acc')
-        except:
-            pass
-
-        # If not, create a new BILL account
-        if not bill_code:
-            try:
-                bill_code = bm.create_bill_account(member.username)
-            except BILLException as e:
-                return HttpResponse(str(e), status=400)
-
-        member.bill_code = bill_code
-        member.save()
+            if not bill.get_account(member.username):
+                bill.create_account(member.username)
+        except bill.BILLException as e:
+            return HttpResponse(str(e), status=400)
 
         return HttpResponse(status=200)
 
     def delete(self, request, member_id):
         member = get_object_or_404(Member, id=member_id)
 
-        if not member.bill_code:
-            return HttpResponse('Member has no BILL account', status=400)
+        if not member.username:
+            return HttpResponse('username missing', status=400)
 
-        bm = BILLAccountManager()
         try:
-            bm.delete_bill_account(member.bill_code)
-        except BILLException as e:
+            bill.delete_account(member.username)
+        except bill.BILLException as e:
             return HttpResponse(str(e), status=400)
 
-        member.bill_code = None
-        member.save()
-
         return HttpResponse(status=200)
 
 

teknologr/katalogen/templates/profile.html

@@ -40,14 +40,19 @@ <h5>Övrig privat information</h5>
           <tr>
             <th>BILL-konto</th>
             <td class="text-right">
-              <span class="monospace">{{ member.bill_code|default:'' }}</span>
+              <span class="monospace">{{ bill.acc }}</span>
+            </td>
+          </tr>
+          <tr>
+            <th>BILL-smeknamn</th>
+            <td class="text-right">{{ bill.nick }}
             </td>
           </tr>
           <tr>
             <th>BILL-saldo</th>
             <td class="text-right">
-              {% if bill_balance %}
-                {{ bill_balance }} €
+              {% if bill.balance %}
+                {{ bill.balance }} €
               {% endif %}
             </td>
           </tr>