DATA

1) About table course_modules_availability_field 

a) It's more than one word so name should be plural (from code guidelines). Yes I'm aware lots of existing tables do this wrong.

b) If using integer constants, the comment for the 'operator' field should list all values, not just say that some values are used. (Intention is that by looking at the xmldb doc page alongside the table data, you can more or less understand what it means without having to look at the code.) Better still, why not change it to a char field using meaningful keywords like 'contains' so that it's self-documenting? (There isn't a big performance penalty for char fields, if you're not going to join on them.)

c) Regarding the field 'field', multiple-meaning-based-on-content fields are best avoided if possible as they can make SQL more complicated. I would suggest two fields:

* userfield - name of field in user table (char, may be null)
* customfieldid - id of field in user_info_field table (unsigned int 10, may be null)

So basically, one or other of these will be null (but not both).

d) 'value' should have 'not null' set to true.


BACKUP/RESTORE

2) How does this work with custom user fields (like '3' or whatever) when restoring between different servers that might have different fields set up or in a different order? (Hint: It doesn't.) Could this be improved? I'm not sure... Could you check how custom user fields are represented when it backs up user - i.e. maybe it backs up the name as well as the id, and then ignores it on restore if there isn't one with the name matching the id? Maybe?

3) You do the thing with set_backup_ids_record but I don't think this is necessary, is it? Can't you just insert it directly at that point? Aside from the user fields issue, the only id stored in this data which has changed on restore is the coursemoduleid and you already fixed that one so there is no need to defer it, this just makes backup slower for no reason I think.


LIB

4) Query to get condition fields in conditionlib 176-

+                if (is_numeric($condition->field)) {
+                    if ($field = $DB->get_record('user_info_field', array('id'=>$condition->field))) {
+                        $fieldname = $field->name;

WHOOP WHOOP! It's the performance problem alert! You're doing a query inside a loop. That's rarely a good look.

Also of interest, the query above could have been get_records because it didn't use a join. But, actually the correct answer to this problem is to do a join. You should be able to simultaneously query the records and LEFT JOIN with the user_info_field table to get the names where applicable.

This will be easier if you change the data structure as recommended above.

Possibly if you do that you can maybe sort it out so that it returns the correct fields (note: usually better to specify fields rather than using *) to put into the $cm->conditionsfield array directly without having to manually make a new object. You can maybe even do the whole thing with get_records_sql.

5) get_condition_user_field_operators, get_condition_user_fields: should these be defined as static?

6) get_condition_user_field_operators: the operators should definitely be constants (as noted I would recommend using strings e.g. 'contains', but still define as constants either way, within the class e.g. const OP_CONTAINS = 'contains').

7) Logic around line 600 where it checks for each operator. Could you split this into a new private function please, test_operator($operator, $uservalue, $value) - this will make it easier to understand. (Plus using the constants as above.)

8) Logic around 850 where it's getting the user field cache. Could you change this to use get_records_sql instead of recordset. That way you can just set the array to the result directly instead of using that loop. And it will always be set (to empty array) so you don't need the condition below.

9) QUESTION - is there any value in supporting $grabthelot here? Really we might as well grab all the user's custom fields, mightn't we? Not like there are probably that many. It's a bit different from grabbing data for only one activity vs. hundreds. I might be wrong though...

10) get_cached_user_profile_field claims it returns a string, but there are points where it returns false. Suggest returning string '' instead so that it is correct, and updating phpdoc to add 'or empty string if not found'.

11) get_cached_user_profile_field last part:

if ($user = $DB->get_record('user', array('id' => $userid), $fieldid)) {

a) I think you should change this to remove the if statement and use MUST_EXIST. If you call this with an invalid user id, that's surely an error.

b) Surely this should get get_field, not do get_record and then split out the one required field...


FORM

12) I think you haven't prevented them having multiple conditions on the same field, but as far as I can see this won't work because field name is used as key in several arrays in conditionlib. Either:

a) Add validation in this form to ensure that you only have one condition on one field.
or
b) Change the conditionlib code so that the key in those arrays is id or something, and the value is an object containing field/value pairs.


MODINFOLIB

13) QUESTION - This looks fine but I don't see how it retrieves this data? I guess I was expecting to see the code (either here or in course/lib.php probably) which was going to actually fill the conditionfields array for all the cms on the course in one query or whatever? Could you confirm this part actually works i.e. the data does actually get cached in modinfo.


LANG STRINGS

14) Restriction

$string['requires_user_field_restriction'] = 'Only available to users who\'s user field \'{$a->field}\' {$a->operator} \'{$a->value}\'.';

who's is a minor grammar error. Also the format doesn't match other strings of this type. I suggest:

Not available unless your <strong>{$a->field}</strong> {$a->operator} <strong>{$a->value}</strong>.

E.g. this would often say things like 'Not available unless your department = Science.' which is quite clear...

15) Help

$string['userfield_help'] = 'This is an attribute associated with each individual user';

Sorry but this is no help at all :) Suggest the following:

You can restrict access based on any field from the user&rsquo;s profile.

(key point here is that it mentions the profile so users know where this data comes from. they can probably guess anyway but.)


TRIVIA

NOTE: With regard to trivial changes, there is much existing code in these files that doesn't follow the code guidelines. Definitely don't change this - my general policy is that all lines you have modified should also be corrected, even/especially if this makes it 'inconsistent' within a file.

16) Since you're changing the last line in backup_stepslib could you add a newline at end of file (otherwise git shows that stupid message).

17) restore_stepslib has a 'foreach(' [missing space]; also is there a line longer than 100 characters there, if so could you wrap it please.

18) moodleform_mod has a '$num=0' and others like this (missing spaces around = sign), also missing space before bracket in foreach. Also needs space after comma, space around ? and : and + and => operators, and I prefer space around . operator too. Finally there is one instance of: ($details->value) - best not to use brackets around a single variable name like that.

19) conditionlib has some of the same problems.

20) phpdoc in conditionlib should be changed a bit:
a) Don't bother with @global, delete these (some are wrong anyway, the syntax for this is horrible)
b) '@return array' needs a description, like '@return array Associative array from operator constant to display name'
c) This is me being anal, but please could you make the descriptions (first part of phpdoc) so that they use an active verb ('Returns...' 'Sets...' etc) and end in a full stop. Everything else (@param, @return, etc) should not end in a full stop and is already correct, it's just the descriptions.

21) conditionlib array variables weird exception - when you define an array like on line 261 $userfields = ..., the indent for that array is only FOUR characters not like a normal wrapped line which is eight.

22) get_condition_user_fields - please change the first part so it loops around getting the names by calling the existing get_user_field_name function (in mooodlelib) - then change that function to add the exceptions you've already handled where it is different (aimid, etc). Instead of duplicating the logic.

23) conditionlib 324 has only 4 spaces indent, should be 8.