patrons: fix patron creation and modification

* Fixes patron creation with an existing user account, closes rero#1454.
* Ensures that when the patron updates his/her email in the RERO ID, the
  modification is synched to the patron record, closes rero#1458.
* Makes the email required if the communication channel is email, closes rero#1455.

Co-Authored-by: Johnny Mariéthoz <Johnny.Mariethoz@rero.ch>

rero_ils/config.py

@@ -2080,6 +2080,10 @@ def _(x):
 #: Allow users to login without first confirming their email address.
 SECURITY_LOGIN_WITHOUT_CONFIRMATION = False
 
+#: TODO: remove this when the email is sent only if the user has an email
+# address
+SECURITY_SEND_PASSWORD_CHANGE_EMAIL = False
+
 # Misc
 INDEXER_REPLACE_REFS = True
 INDEXER_RECORD_TO_INDEX = 'rero_ils.modules.indexer_utils.record_to_index'

rero_ils/modules/patrons/api.py

@@ -32,7 +32,6 @@
 from invenio_userprofiles.models import UserProfile
 from sqlalchemy.orm.exc import NoResultFound
 from werkzeug.local import LocalProxy
-from werkzeug.utils import cached_property
 
 from .models import PatronIdentifier, PatronMetadata
 from .utils import get_patron_from_arguments
@@ -173,12 +172,7 @@ def create(cls, data, id_=None, delete_pid=False,
         data = trim_barcode_for_record(data=data)
         # synchronize the rero id user profile data
         user = cls.sync_user_and_profile(data)
-
         try:
-            # for a fresh created user
-            if user:
-                # link by id
-                data.setdefault('user_id', user.id)
             record = super(Patron, cls).create(
                 data, id_, delete_pid, dbcommit, reindex, **kwargs)
             record._update_roles()
@@ -197,7 +191,8 @@ def update(self, data, dbcommit=False, reindex=False):
         # remove spaces
         data = trim_barcode_for_record(data=data)
         # synchronize the rero id user profile data
-        self.sync_user_and_profile(dict(self, **data))
+        data = dict(self, **data)
+        self.sync_user_and_profile(data)
         super(Patron, self).update(data, dbcommit, reindex)
         self._update_roles()
         return self
@@ -226,6 +221,17 @@ def update_from_profile(cls, profile):
                     value = getattr(profile, field)
                     if value not in [None, '']:
                         patron[field] = value
+            # update the email
+            if profile.user.email != patron.get('email'):
+                # the email is not defined or removed in the user profile
+                if not profile.user.email:
+                    try:
+                        del patron['email']
+                    except KeyError:
+                        pass
+                else:
+                    # the email has been updated in the user profile
+                    patron['email'] = profile.user.email
             super(Patron, patron).update(dict(patron), True, True)
 
     @classmethod
@@ -238,15 +244,15 @@ def _get_user_by_data(cls, data):
         :return: a patron object or None.
         """
         user = None
-        if data.get('user_id'):
-            user = User.query.filter_by(id=data.get('user_id')).first()
         if not user and data.get('username'):
             try:
                 user = UserProfile.get_by_username(data.get('username')).user
             except NoResultFound:
                 user = None
         if not user and data.get('email'):
             user = User.query.filter_by(email=data.get('email')).first()
+        if not user and not data.get('user_id'):
+            user = User.query.filter_by(id=data.get('user_id')).first()
         return user
 
     @classmethod
@@ -255,9 +261,10 @@ def sync_user_and_profile(cls, data):
 
         :param data - dict representing the patron data
         """
+        created = False
         # start a session to be able to rollback if the data are not valid
+        user = cls._get_user_by_data(data)
         with db.session.begin_nested():
-            user = cls._get_user_by_data(data)
             # need to create the user
             if not user:
                 birth_date = data.get('birth_date')
@@ -270,24 +277,31 @@ def sync_user_and_profile(cls, data):
                     password=hash_password(birth_date),
                     profile=dict(), active=True)
                 db.session.add(user)
+                created = True
             else:
                 if user.email != data.get('email'):
                     user.email = data.get('email')
             # update all common fields
+            if user.profile is None:
+                user.profile = UserProfile(user_id=user.id)
+            profile = user.profile
             for field in cls.profile_fields:
                 # date field need conversion
                 if field == 'birth_date':
                     setattr(
-                        user.profile, field,
+                        profile, field,
                         datetime.strptime(data.get(field), '%Y-%m-%d'))
                 else:
-                    setattr(user.profile, field, data.get(field, ''))
+                    setattr(profile, field, data.get(field, ''))
             db.session.merge(user)
-            if not data.get('user_id'):
+            data['user_id'] = user.id
+            if created:
                 # the fresh created user
                 return user
 
-    @cached_property
+    # TODO: use cached property one we found how to invalidate the cache when
+    #       the user change
+    @property
     def user(self):
         """Invenio user of a patron."""
         user = _datastore.find_user(id=self.get('user_id'))

rero_ils/modules/patrons/jsonschemas/patrons/patron-v0.0.1.json

@@ -80,7 +80,7 @@
       "minLength": 6,
       "form": {
         "expressionProperties": {
-          "templateOptions.required": "field.parent.model.roles.some(v => (v === 'librarian' || v === 'system_librarian'))"
+          "templateOptions.required": "field.parent.model.roles.some(v => (v === 'librarian' || v === 'system_librarian')) || field.parent.model.patron.communication_channel === 'email'"
         },
         "validation": {
           "validators": {

tests/ui/patrons/test_patrons_api.py

@@ -25,6 +25,7 @@
 
 import pytest
 from invenio_accounts.models import User
+from invenio_accounts.testutils import create_test_user
 from invenio_userprofiles import UserProfile
 from jsonschema.exceptions import ValidationError
 
@@ -167,45 +168,51 @@ def test_patron_create(app, roles, lib_martigny, librarian_martigny_data_tmp,
 def test_patron_create_without_email(app, roles, patron_type_children_martigny,
                                      patron_martigny_data_tmp, mailbox):
     """Test Patron creation without an email."""
-    del patron_martigny_data_tmp['email']
+    patron_martigny_data_tmp = deepcopy(patron_martigny_data_tmp)
 
     # no data has been created
     mailbox.clear()
-    # assert User.query.count() == 0
-    # assert UserProfile.query.count() == 0
 
+    # create a patron without email
+    del patron_martigny_data_tmp['email']
     ptrn = Patron.create(
         patron_martigny_data_tmp,
         dbcommit=True,
         delete_pid=True
     )
+    # user has been created
     user = User.query.filter_by(id=ptrn.get('user_id')).first()
     assert user
     assert not user.email
     assert user == ptrn.user
     assert user.active
     assert len(mailbox) == 0
 
+    # add an email of a non existing user
     patron_martigny_data_tmp['email'] = 'test@test.ch'
     ptrn.replace(
         data=patron_martigny_data_tmp,
         dbcommit=True
     )
+    # the user remains the same
     assert user == ptrn.user
     assert user.email == patron_martigny_data_tmp['email']
     assert user.active
     assert len(mailbox) == 0
 
+    # update with a new email in the system
     patron_martigny_data_tmp['email'] = 'test@test1.ch'
     ptrn.replace(
         data=patron_martigny_data_tmp,
         dbcommit=True
     )
+    # the user remains the same
     assert user == ptrn.user
     assert user.email == patron_martigny_data_tmp['email']
     assert user.active
     assert len(mailbox) == 0
 
+    # remove the email
     del patron_martigny_data_tmp['email']
     ptrn.replace(
         data=patron_martigny_data_tmp,
@@ -216,6 +223,26 @@ def test_patron_create_without_email(app, roles, patron_type_children_martigny,
     assert user.active
     assert len(mailbox) == 0
 
+    # create a new invenio user in the system
+    rero_id_user = create_test_user(email='reroid@test.com', active=True)
+
+    # update the patron with the email of the freshed create invenio user
+    patron_martigny_data_tmp['email'] = 'reroid@test.com'
+    patron_martigny_data_tmp['username'] = 'reroid'
+    ptrn.replace(
+        data=patron_martigny_data_tmp,
+        dbcommit=True
+    )
+    # the user linked with the patron has been changed
+    assert rero_id_user == ptrn.user
+    # the username is updated on both user profile and patron
+    assert rero_id_user.profile.username == ptrn.get('username') == 'reroid'
+
+    # clean up created users
+    ds = app.extensions['invenio-accounts'].datastore
+    ds.delete_user(user)
+    ds.delete_user(rero_id_user)
+
 
 def test_patron_organisation_pid(org_martigny, patron_martigny_no_email,
                                  librarian_martigny_no_email):