Unique profile field

Found and removed a dsm() call.
Filled out the missing description for schema.

Please ignore above attachement.
Use this one instead for review.

I didn't realise there were very specific coding standards that I need to adhere to.
Ran through coder and fixed all problems.

Please ignore above.
Use the following attachment for review.

Git updated too.

Thanks for the review.

I've removed the outdated CVS $Id$ from all files. I've also removed the unwanted directory from the git repo.

Regarding hook_form_alter: I'd rather I keep it the way it is. Its a small function which checks three forms. If I were to use hook_form_FORM_ID_alter, I would have to end up defining at least two functions and still do if() checks inside the second one.

Have changed the comments to properly constructed sentences.

Have also changed the formatting so that its a little more readable and make it seem a little less complex :).

Updated to git.
Attached the same here.

Thanks again.

BTW Did you get a chance to test the functionality though?

This is a working module alliax. I've submitted it here for review so that I can put it up as a real project page once its approved.

You can download the latest zip file above, and use it. I would really appreciate it if you could provide feedback about it here once you have.

Thanks.

Currently, all existing profile fields remain untouched.
Uniqueness check is only performed on new field creation and existing field update.

For instance: say out of 1000 users, there are 4 with the same x value in a particular field that is designated as unique.

When any of the 4 users tries to update their profile information, they will not be able to do so with the value x.
If all 4 users update their profiles, the last one to do so won't be forced to enter a new value for the field containing x.

And of course no user will be allowed to create a new field with the same x value.

--

Perhaps an option can be provided after install where you can choose what you want to do with old data. Something like:
[]Auto-increment non-unique data for this field.
[]Leave data untouched.
[]...
This could be a per field or a global setting.
I don't think this should be too difficult to implement.

I did originally design the module thinking that one would generally want to install something like this pre-production. Because making a decision to make an existing field unique on a production site, means there is some major change in business logic itself. And I don't know whether the module should address that.

Having said that, if you think this would be a useful feature, do let me know and I could try to implement it.

> Simplest of all would be to lock the setting on fields with data, but that's a bit draconian.

I agree. Locking settings seems a bit over the top.

> Otherwise, you could run queries when a field is changed to unique and just warn the user that there is duplicate data, and show a list of the duplicates.

I think this is a very sensible suggestion. I am going to try and implement this.

Thanks.

I've implemented the feature that notifies the user if any existing values in a field are duplicate.
This notification is shown when a profile field's settings are updated and the user has turned on the option to make the field unique.
Have updated git.
http://drupalcode.org/sandbox/vrun/1173080.git/tree/ab724fa

Also find attached.

I am wondering though, how does this project actually get approved? I mean the module works, the code meets the coding standards, and the feature the module provides is currently not available. Do I need to do something more? I am a little unclear as to the next steps. Should I change the status to "reviewed and tested by the community"? Or someone else does that?

jordojuice, thanks for the prompt and helpful response. I will definitely try and review other modules as and when I find some time. Frankly, I had no clue that I could review modules.

Regarding the README.txt file, I understand from joachim's response that this isn't necessary, since this module is fairly straightforward to use. And, I assume anybody trying to use this module will at least read the description of the same, which delineates how the module functions. However, if this is a necessity let me know, and I will add a README.txt.

Also, I'm curious: what does the pdx-code-review tag that you attached signify?

joachim, thanks again for keeping this module on your radar. I look forward to further feedback. Already, I think the module is better for implementing your suggestion of notifying the user of duplicates.

Speaking of which, did you get a chance to test this new feature?
Awaiting your thoughts.

bump

Thanks joachim.

I don't know why it din't occur to me to use JOIN instead of two separate queries. I have fixed this:
http://drupalcode.org/sandbox/vrun/1173080.git/tree/577d5b6
___

Also, could unique_profile_field_check_existing_data() do its work with a COUNT ... GROUP BY ... query? Or would that not account for case differences (eg 'foo' / 'FOO')?

I remember doing something like:
SELECT value FROM profile_values WHERE fid = %d GROUP BY value HAVING ( COUNT(value) > 1 )
while designing it originally, and yes you are right -- the query above would consider "test", "TEST" and "TeSt" as the same value and return a count greater than 1. I remember reading somewhere, something about doing a GROUP BY hex(value), which would return a case sensitive match. But it seemed convoluted and would have been indecipherable when read by someone else.
___

unique_profile_field_get_value -- That would be better named unique_profile_field_field_is_unique() perhaps?
I use the get and set value naming combination as a convention that I find convenient in my head to deal with. I do realize that since the function returns a 0 or a 1 it could be used as a boolean check, and thus named. But I think I prefer the naming the way it is.

SELECT COUNT(value) AS field_value_occurrence_count FROM profile_values WHERE fid = %d GROUP BY value HAVING ( field_value_occurrence_count > 1 )

This will still return a case-insensitive match. I need it to consider "test" and "TEst" as different values. GROUP BY considers both as the same value and so returns a count greater than 1. Which is incorrect in this context. To make it case-sensitive I would need to do something like:

SELECT COUNT(value) AS field_value_occurrence_count FROM profile_values WHERE fid = %d GROUP BY hex(value) HAVING ( field_value_occurrence_count > 1 )

This will work correctly, since the hex value for "test" and "TEst" will be different, but will be the same for "test" and "test".
What I don't know is whether the above statement should be used considering its an unorthodox SQL statement, specifically the hex part.

I think you are mistaken, joachim.

For instance:
lower("TEST") will translate to "test", as will lower("TeSt"), lower("test") or lower("Test").

This would again end up leading to the same thing: Case insensitive match, and thus fields that are in fact unique will be considered as non-unique.

however hex("TEST") is different than hex("test"), which would lead to correct results.
Please see the attached image. The test table has the values "Test" and "test" in it, along with five occurrences or "100000", and some other random values. The first result in the image is incorrect. The second one is correct.

I could simply use HEX and put a comment explaining why.

> I would consider 'test' and 'TEST' to be the same, and therefore non-unique.

This would be acceptable in say a field that contains a name or something where the case doesn't matter.

But, what about say if the field contains unique registration codes which are entered by the user during registration and which are stored as a profile field. And say these codes are originally generated randomly by a script and mailed to the user. In this case, case-sensitive match is a must because otherwise two unique codes like uxfTT56 and UXFtt56 will end up being considered as the same, effectively disallowing registration of the later user. (This is in fact is a real use-case on one of my sites)

However during the course of this conversation, I did modify the function code slightly to try out with HEX (Find below). Functionally of course its no different. It doesn't even translate to lesser code. But perhaps is a bit clearer in terms of what its trying to do. What do you think? If it does make things clearer then I will update the existing function code.

/**
 * Checks existing data of a field when made unique, and issues a message if any existing duplicates are found.
 * No existing data is modified.
 * This is the only checking that is carried out on existing field data. 
 */
function unique_profile_field_check_existing_data($profile_field_id) {
  //Get all values from the given profile_field which have more than one occurrence, 
  //using GROUP BY HEX(value) to ensure a CASE-SENSITIVE match.
  $result = db_query('SELECT COUNT(value) AS occurrences, value 
                      FROM {profile_values} 
                      GROUP BY HEX(value) 
                      HAVING occurrences > 1 
                      WHERE fid = %d', $profile_field_id);
  $duplicates = '';                      
  while ($row = db_fetch_object($result)) {
    $duplicates = ($duplicates == '') ? "$row->value ($row->occurrences)" : ", $row->value ($row->occurrences)";    
  }
  // If no duplicates are found, exit checking existing data.
  if ($duplicates == '') {
    return;
  }
  // Get field title.
  $title = db_result(db_query('SELECT title FROM {profile_fields} WHERE fid = %d', $profile_field_id));
  // Notify user of duplicates that were found.
  drupal_set_message(t('The field %title contains data that is not unique. '.
                       'The following duplicate values were found: %duplicates. '.
                       'This is simply a notification. '.
                       'No existing data has been modified. '.
                       'Only new field values and updates on existing field values will be tested for uniqueness.', 
                       array('%title' => $title, '%duplicates' => $duplicates)));
}

>You might want to consider a setting for whether uniqueness is case-sensitive or not.
Good idea. Have implemented it. Updated git:
http://drupalcode.org/sandbox/vrun/1173080.git/tree/3de6792

>Also, a thought: are you checking for whitespace? Would 'John' and 'John ' count as different? Should they?
I haven't thought about this. Good point. I think I should simply trim the values.
So 'John ' == 'John' == ' John', but
'John' != 'J ohn' != 'Jo hn '

What do you think?
If agreed, will implement.
Currently am not even using trim().

Thanks.

Had inadvertently removed descriptions from schema in the install file in the last update.
Have fixed it.
http://drupalcode.org/sandbox/vrun/1173080.git/tree

bump

I am just hoping that someone will look into this. It's been 1.5 months since my last substantive post, and haven't yet received any feedback/review.

Done.

http://drupalcode.org/sandbox/vrun/1173080.git/tree/8a695744f69d47c3e16c...

Have implemented all the changes as recommended
http://drupalcode.org/sandbox/vrun/1173080.git/tree/7d17820

One question though:
I did remove the t() calls for db field descriptions as suggested, but why is it that a lot of other modules I went through do this?

@asrob: Thanks! I am glad it works for you.

@klausi: Thanks for the detailed and helpful review. Especially for the manual review.
Have updated as per your recommendations.
http://drupalcode.org/sandbox/vrun/1173080.git/tree/f2afd1e

• Cleared the master branch and updated README.txt for the same, as per http://drupal.org/node/1127732 .
• Fixed line ending and other code style issues through PAReview.sh
• unique_profile_field_validate(): I agree. It was inefficient. Have changed the function so that only the necessary data is fetched.
• unique_profile_field_get_value(): Agreed. Fixed.
• unique_profile_field_set_value(): Have changed the function to use drupal_write_record(). However due to http://echodittolabs.org/blog/2010/10/drupalwriterecord-can-return-false... , the conditional check is a bit strange.
• Checked one last time through coder on minor settings, and PAReview.sh and found no errors.

Thanks again.

@klausi: Thanks, this is great!

A big thanks to all the reviewers too.