Uploaded image for project: 'OpenOLAT'
  1. OpenOLAT
  2. OO-809

RS when importing a list of users to a group by email address

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed (View Workflow)
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 9.0.4
    • Fix Version/s: 9.0.6
    • Component/s: Group
    • Labels:
      None
    • Environment:

      DB: Postgresql

      Description

      When using the group wizard to import a list of users a RS occurs if you don't use MySQL and the list contains at least 2 email addresses.

      The cause is that the DB function lower() (UserManagerImpl: findIdentitiesByEmail()) must not contain a list of operands (array). The operand must be a single string.

      A solution can be the first patch which converts the array of email addresses to lower case. So the "lower()" in the DB query can be omitted. Assuming to check a list of emails, the operator "=" is not suitable, the operator has to be "in".

      While investigating the whole thing, I encountered a problem with case sensitivity. A group admin can type usernames and email addresses also with capital characters. Email addresses are always case insensitive while usernames are lower case per definition. This case is not respected in the code (ImportMemberOverviewIdentitiesController: loadModel()).

      The second patch considers that institutinal identifiers are case sensitive, user names are lower case and email addresses are case insensitive. I removed the code for checking the validity of an email address, because this is done in findIdentitiesByEmail().

      Patch 1:
      diff -r 00114a4a4cbb src/main/java/org/olat/user/UserManagerImpl.java
      — a/src/main/java/org/olat/user/UserManagerImpl.java Mon Oct 07 14:19:09 2013 +0200
      +++ b/src/main/java/org/olat/user/UserManagerImpl.java Wed Oct 09 14:34:21 2013 +0200
      @@ -204,14 +204,16 @@
      @Override
      public List<Identity> findIdentitiesByEmail(List<String> emailList) {
      List<String> emails = new ArrayList<String>(emailList);

      • for(Iterator<String> emailIt=emails.iterator(); emailIt.hasNext(); ) {
      • String email = emailIt.next();
        + for (int i=0; i<emails.size(); i++)
        Unknown macro: {+ String email = emails.get(i).toLowerCase(); if (!MailHelper.isValidEmailAddress(email)) { - emailIt.remove(); + emails.remove(i); logWarn("Invalid email address: " + email, null); }+ else { + emails.set(i, email); + } }
      • if(emails.isEmpty())

        { return Collections.emptyList(); }

        @@ -225,7 +227,7 @@
        if(mysql)

        { emailSb.append(" user.properties['").append(UserConstants.EMAIL).append("'] in (:emails) "); }

        else

        { - emailSb.append(" lower(user.properties['").append(UserConstants.EMAIL).append("']) = lower(:emails)"); + emailSb.append(" lower(user.properties['").append(UserConstants.EMAIL).append("']) in (:emails)"); }

      List<Identity> identities = dbInstance.getCurrentEntityManager()
      @@ -237,7 +239,7 @@
      if(mysql)

      { institutionalSb.append(" user.properties['").append(UserConstants.INSTITUTIONALEMAIL).append("'] in (:emails) "); }

      else

      { - institutionalSb.append(" lower(user.properties['").append(UserConstants.INSTITUTIONALEMAIL).append("']) = lower(:emails)"); + institutionalSb.append(" lower(user.properties['").append(UserConstants.INSTITUTIONALEMAIL).append("']) in (:emails)"); }

      if(!identities.isEmpty())

      { institutionalSb.append(" and identity not in (:identities) "); Patch 2: diff -r 00114a4a4cbb src/main/java/org/olat/course/member/wizard/ImportMemberOverviewIdentitiesController.java --- a/src/main/java/org/olat/course/member/wizard/ImportMemberOverviewIdentitiesController.java Mon Oct 07 14:19:09 2013 +0200 +++ b/src/main/java/org/olat/course/member/wizard/ImportMemberOverviewIdentitiesController.java Wed Oct 09 14:35:24 2013 +0200 @@ -160,40 +160,7 @@ }

      }

      • //search by names
      • List<Identity> identities = securityManager.findIdentitiesByName(identList);
      • for(Identity identity:identities) {
      • identList.remove(identity.getName());
      • if (!PersistenceHelper.containsPersistable(oks, identity)
      • && !securityManager.isIdentityInSecurityGroup(identity, anonymousSecGroup)) { - oks.add(identity); - }
        - }
        -
        - //search by email
        - List<String> emails = new ArrayList<String>();
        - for(String ident:identList) {
        - if(MailHelper.isValidEmailAddress(ident)) { - emails.add(ident); - }
        - }
        - List<Identity> mailIdentities = userManager.findIdentitiesByEmail(emails);
        - for(Identity identity:mailIdentities) {
        - String email = identity.getUser().getProperty(UserConstants.EMAIL, null);
        - if(email != null) { - identList.remove(email); - }
        - String institutEmail = identity.getUser().getProperty(UserConstants.INSTITUTIONALEMAIL, null);
        - if(institutEmail != null) { - identList.remove(institutEmail); - }
        - if (!PersistenceHelper.containsPersistable(oks, identity)
        - && !securityManager.isIdentityInSecurityGroup(identity, anonymousSecGroup)) {- oks.add(identity);- }
      • }
      • //search by institutionalUserIdentifier
        + //search by institutionalUserIdentifier, case sensitive
        List<Identity> institutIdentities = securityManager.findIdentitiesByNumber(identList);
        for(Identity identity:institutIdentities) { String userIdent = identity.getUser().getProperty(UserConstants.INSTITUTIONALUSERIDENTIFIER, null); @@ -205,8 +172,39 @@ oks.add(identity); }

        }
        + // make a lowercase copy of identList for processing username and email
        + List<String> identListLowercase = new ArrayList<String>();
        + for (String ident:identList)

        { + identListLowercase.add(ident.toLowerCase()); + }

        + //search by names, must be lower case
        + List<Identity> identities = securityManager.findIdentitiesByName(identListLowercase);
        + for(Identity identity:identities)

        Unknown macro: {+ identListLowercase.remove(identity.getName());+ if (!PersistenceHelper.containsPersistable(oks, identity)+ && !securityManager.isIdentityInSecurityGroup(identity, anonymousSecGroup)) { + oks.add(identity); + }
        + }

        - notfounds.addAll(identList);
        + //search by email, case insensitive
        + List<Identity> mailIdentities = userManager.findIdentitiesByEmail(identListLowercase);
        + for(Identity identity:mailIdentities) {
        + String email = identity.getUser().getProperty(UserConstants.EMAIL, null);
        + if(email != null) { + identListLowercase.remove(email); + }
        + String institutEmail = identity.getUser().getProperty(UserConstants.INSTITUTIONALEMAIL, null);
        + if(institutEmail != null) { + identListLowercase.remove(institutEmail); + }
        + if (!PersistenceHelper.containsPersistable(oks, identity)
        + && !securityManager.isIdentityInSecurityGroup(identity, anonymousSecGroup)) {+ oks.add(identity);+ }+ }

        +
        + notfounds.addAll(identListLowercase);
        }

      public boolean validate() {

        Attachments

          Activity

            People

            Assignee:
            srosse Stéphane Rossé
            Reporter:
            stephan Stephan Clemenz
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 1 hour
                1h