Hi Yuliya,

The changes for the main part of this change look good, but the award_criteria_course things caught my eye:

1/ In award_criteria_course::get_options(), I don't really undetand the logic around $param, $this->param and the courseid. I mean it looks like there sometimes could be a courseid coming from the $PAGE->course->id and sometimes from the $this->params. The array_shift vs explicit array setting looks strange. Could you explain a bit whats going on there because i've looked through it a few times and its still confusing me. Ideally, I think the course id should always passed from parameters and not taken form the $PAGE global as that might have side effects.

2/ Again in award_criteria_course::get_options() it seems like its missing an $param = array() definition too.

thanks,
Dan

Hi Dan,

Course criteria can be added only to course badges, so there should be no problem with taking $PAGE->course->id. I changed it so that id is passed with the form if you think that it would be better. The problem was that when I was creating an element I needed to know what course it was. I also fixed some errors that would happen if a course gets deleted.

Criteria are saved in form:
badge -> criteria -> array(crit1, crit2, crit3). Each criteria except for a single course completion can have multiple parameters (like courses, roles, activities), so they are stored in form crit1 = array(param1, param2, param3, ...). Course criteria can have only one parameter - this course, so through array_shift() I was getting this first parameter. I changed that to reset() now.

Thanks, sending to integration.

Some comments:

1) you can move all the files in the area together. Not sure if that's interesting in this case or the number of badges per course is always small. For your consideration.

2) I've seen that, after moving the 'badgeimage' files... you proceed to update the 'badge' record with the new information (type, status...) but it has surprised me that you are also storing the file->id into badge->image. Is there any use of that information? I hope it's only being used as sort of bool flag (although it's mandatory so shouldn't be necessary), because files should not be accessed by id ever. More if the id is stored "externally" (in the badges table).

3) course->fullname needs to processed by format_string() always (to support multilang contents mainly). Note that exactly the same applies to activity->name (although there aren't occurrences in this patch).

4) finally, although apart from this issue, I'm not really sure about the make_categories_list() call and the HUGE list being the best candidate for picking a list of course. I can imagine sites with thousands of courses running under problems to get that form (courseset) defined. Some search alternative or filtering option may be necessary (as said, unrelated with this issue).

999) Although I've not idea about badges at all... I've failed to find any backup/restore support about them. While it may be not critical right now... I think it's a MUST once people start using / sharing / moving 2.5 courses with badges information. Please consider adding it ASAP. Structures don't seem to be very complex. Again, in another issue. I just discovered it because I use backup code to learn about the DB schema, hehe. And I did not find anything.

So, I'm reopening this because of 1-2-3 (mainly because of 3, with 1-2 for your consideration). Otherwise, it looks perfect, I think. Thanks!

Edited: typos.

Note that MDL-39538 has been integrated, so the ::make_category_list() call is not needed here anymore.

1) Will have a look into that. Implemented the first solution that I found, really.

2) File id is never used to access a badge image file. Now that I think about it, it is not really needed. I guess it was in initial design when I was not too familiar with Moodle File API. I can file a bug to remove it later, if you would prefer that.

3) Will do.

4) Need to think about that.

999) Yeah, it's on my todo list (I noticed that recently as well). Haven't got around adding that.

Thanks for your comments!

Hi, the new patch looks better, really. Just 1 tiny detail for confirmation. Is this line/change correct (cancel name, continue string):

https://github.com/totara/openbadges/commit/7deff81f80834afc1b84a8d2b65da9d0f6aa7f7f#L1R114

Of course, it would be great to have separate issues for:

A) Take rid of badge->image
B) Review all uses of both course and activity names and ensure they are processed by format_string()
C) Backup / restore support

Awaiting for confirmation about that line change before proceeding with integration.

Yep, line change is correct. It was supposed to be cancel in first place.

I already created issue MDL-39558 for backup and restore. Will do the same for the rest.

Super, thanks... proceeding with this.

Integrated, thanks!

Works perfectly

Did you think this day was not going to arrive ever?

Your patience has been rewarded, yay, sent upstream, thanks!

Closing...ciao