Open Badges Integration

Thanks for raising this issue, Simon.

The final specs are now complete and available on github for review:

https://github.com/totara/openbadges-docs

We will be starting development very soon so any feedback would be appreciated.

Following discussions with Martin at the hackfest we'll be implementing this with the intention of it going into core (presumably 2.5) rather than as a local plugin.

We aim to have the Moodle part (Phase 1) complete by mid January 2013.

Does someone have permission to assign this bug to Yuliya (ybozhko) as I don't seem to be able to?

Yuliya is only listed in the tracker as a normal user. You will need to ping Michael to ask him to add Yuliya to the developers group in Jira.

Hi,
i've just quickly browsed through the design spec and noticed:

• criteria and aggregation methods design seem to be duplicating what /completion has in master. (related --> MDL-36419, i've added Aaron as watcher here). Creation of criteria might actually be rephrased as creating instances of those criteria classes which seems to be what completion does anyway iirc.
• if these cannot be normalised in /completion (or using rubrics - http://docs.moodle.org/dev/Advanced_grading_API), then some further normalisation about criterias ?

i think MDL-36419 might lend a hand in achieving Open Badges integration.

Hi Simon,

Just wondering if you could update the bug here in the same way as the forums.

Sure will do!

I've added a link to the git repository.

We are currently in alpha which means we're testing the functionality, adding docs and tests, etc. After an internal code review and a few more small improvements we'll be pushing to review with HQ.

Documentation for this project is available at: http://docs.moodle.org/dev/OpenBadges_User_Documentation

Fundamental question after skimming the code. Is openbadges 'core' enough to have its own top level directory/subsystem like this.

Not sure, but it ended up there for consistency with things like course completion and because there didn't seem to be any other place that fitted particularly well. It doesn't make sense as a block, there are site wide badges that sit outside a course level so an activity didn't make sense. It's not really a report. Any other suggestions?

I'm not really a fan of the way all the core bits have code spread around anyway, what I'd really like to see is a plugin type called 'core' so you could have core/*/ folders with all the code relating to each component one place, and it would save a load of conflicts in lib/db/ but that's a different topic

Assigning to Yuliya

I've been playing with it here, and I like it... great start Yuliya and everyone!

I have no problem with /badges, especially as there are portions that will be linked to from the outside world. I'd whinge about the plural "badges" but others have sneaked in plurals now (blocks and notes and files) so it's too late to worry about it.

A couple of minor UI gripes to keep you going (haven't looked at code yet):

M1) /badges/badge.php?hash=cc577479388d31499883f57c2b538cf67fd93545 has no header or nav. I get that you might not always want that if someone's coming from outside but at least if I'm logged in then it should have them, otherwise it feels like a dead end.

M2) It needs to be clearer somewhere that backpack stuff won't work if the site is not accessible from outside world (eg firewall or local site). As it is all I get is this error from Backpack: "We have encountered the following problem: could not get assertion: unreachable" which would probably confuse students.

M3) After creating a badge you get a screen that tells you three times "Criteria for this badge have not been set up yet." Seems overkill.

M4) This may be an open badges flaw, but it seems you can make up anything you like as the badge issuer and it is shown. For example I made a fake badge posing as www.uwa.edu.au. If people start using badges for anything significant then this seems like a disaster waiting to happen. It's like phishing web sites all over again. Look at my CV full of real University degrees! Should we enforce the Moodle site's address as the issuer and also double-check the email, or force the admin's email? I know it's open source and they can get around that but still, let's not make it trivial.

M5) Once issued is it true that criteria can never be changed? I guess that makes sense.

M6) The "General Settings" in admin menu is usually the first item in a group and should be called "Badges settings" so search results make more sense... see example of other admin menus.

M7) Site badges are not assignable at course level? My first intuition was that they should be. If this is by design (and I can guess it was) then please make this clearer on the site badges pages.

Thank you very much for your feedback, Martin!

M1 and M6 I agree with completely and already fixed those in the latest commit yesterday.

M2. Agree but not sure what the best way to achieve that. The message you described is sent by Mozilla backpack when badge is pushed using their API. As I initially thought, it is administrator's job to turn off the options that might not be available to users due to the environment. Is there an easy way to determine if a web site is available externally?

M3. I only count two references Anyway, will have a look at this one and make sure it is fixed.

M4. We raised this issue with open badges project https://github.com/mozilla/openbadges/issues/496. They are saying that having an issuer URL that differs from the assertion URL isn't necessarily a sign of cheating. I see no reason not to enforce the use of web site URL as issuer origin, but I still think that issuer name and contact should be editable. If it's a course, a teacher might want to put their name (email) instead of administrator's. Maybe we can give a badge creator options to choose either a default issuer name or their own? What do you think?

M5. We want to make sure that all users complete the same requirements to earn a badge. If we allowed badges requirements to be modified all the time, we would most likely end up with users having the same badge for meeting completely different requirements. So, we lock criteria once a badge was issued at least once. In case criteria need to be changed, a user can duplicate a badge and delete (really, archive) the old one.

M7. Do you mean manual award of badges? I will have a look at clarifying this on a site badges page.

Thanks Yuliya.

M2. I would think a short message on the page before the admin has set anything up would do the trick. eg something like "Your site must be accessible from the internet if you want to allow users to store their badges in external backpack sites." Docs too.

M3. I counted the button as another one.

M4. I'm surprised they've not thought that through, without something there badges will be relegated to trivial things only. Until they do something there (and I can only see that being based on a central issuing agency with a process similar to SSL certs), I think Moodle should enforce the URL to be $CFG->wwwroot. I agree it would be fine to just pre-fill name and contact with site defaults and let them edit if they want to.

M7. Yeah it could be just in the help or a short text somewhere to say that "These badges can only be assigned to users for site-level activities. Course-level badges must be created at course level." Now I understand it it totally makes sense how it is, but I think something small like that will aid understanding quicker.

Regarding M4, the open badges team have just added "Signed assertions" (https://github.com/mozilla/openbadges/wiki/New-Assertion-Specification#signed-badges) which uses a cryptographic signature instead of an assertion URL. It's only just come out so we haven't implemented it yet but we definitely will soon.

Of course it still doesn't prevent someone setting up their own issuer and issuing themselves badges, but it should be possible to examine the public key URL for a signed badge to check it belongs to the organisation you expect (and I'd expect it to be straightforward to verify a badge via the backpack).

With Open Badges being a relatively new project it doesn't have everything in place yet but the open badges team are very responsive so I expect it will all be in place before too long.

Simon

Just a note about timing here: to have any chance of getting the next 2.5 release this has to be fully submitted for integration by Sunday (the end of this month).

I'll see if I can get someone to peer review the code before then, so if you're going to push changes to it still please do it asap. Thanks!

Thanks, Martin! I will try to push last changes today. Everything seems to be there, just polishing some minor stuff around external connections and adding badges to backpacks.

Hi Guys,

I'm just having my first itteration going through the code looking at it without really understanding the wider picture, just reading the code as its written on the page, if you see what i mean.

I'm really pleased to see code quality is looking really good and also in Moodle style. Anyway, noting down all my non- /badges/ comments quickly (sorry, I didn't order these well, I will do for the rest of the files):

D1. A major thing I think is 'un-moodley' is the use of the badge->context. Almost everywhere in Moodle this would be a Moodle context, but in openbadges it is a 'context level' (which isn't a moodle context contextlevel either). Really its just unfortunate naming, but Moodle contexts are really so funamental and core that I think we should stop that confusion.

D2. The XMLDB schema has previous/next fields in it, but the recent xmldb edtior doesn't produce them because we've removed them from 2.5 and they should be removed.

D3. test.php is left in there

D4. The badges lang file should be ordered alphabetically rather than grouped in sections. It helps prevent people introducing duplicate lang strings.

D5. This lang string should use a placeholder {$fordb->name = get_string('copyof', 'badges') . $this->name;}

D6. get_users_bages() Looks like its doing search sql even when no search terms are supplied. I think that will be bad for the query performance and it would be good to avoid it

D7. I saw instances where you called print_error and then some code afterwards. IIRC print_error will throw an exception and halt execution, so that doesn't make sense. It may be more explict to thorw an exception.

D8. In a few places I saw shortened variable names, thats not moodle style, better to spell it out fully e.g. '$compl' instead of '$completions'. Also underscoresd.

D9. I think that you are sending notifications out with your own email_to_user code. Is this because of a limitation of the the messaging system? All notification should go through the messaging system and direct email calls aren't really allowed.

D10. Some of the badgelib.php functions not in classes struck me that perhaps they could have a name with more 'namespacing' to prevent collisisons. e.g. calculate_messages_schedule(), get_badges() and bake().

D11. in bagdgelib.php::review_all_criteria a comment explaining whats going on when BADGE_TYPE_SITE would be beneficial

D12. get_backpack_settings() has the wrong phpdoc

D13. profile_display_badges Just noting that there is a MDL issue open which I think is moving the profile page away from a table.

D14. DESCRIPTION in phpdoc in blocks/badges/db/access.php

D15. The whitespace is messed up around the comments in badgelib BADGE_STATUS_ACTIVE_LOCKED

action.php

D16. You are doing navigation_node::override_active_url in many of the files - should these nodes be properly defined in the navigation rather than this? I'm surprised Martin didn't mention that actually. I thought we were moving away from tabs in favour of navigation.

D17. The use of the HTTP_REFFER is generally discouraged as its quite fragile and it looks a bit fragile in this case too. Could we pass a returnurl instead?

D18. confirm_sesskey() in a few places I would prefer to be require_sesskey() inside the if statement, just because it ends the execuation and it shouldn't be wrong in normal circumstances. Thats just my preference though. ()

assertion.php

D19. Should page ever be restricted?

award.php

D20. Issuing by role is really non-conventional, is there a reason capabilites don't work in this situation? Actually this is related to Martin's comment M5. What happens if a somebody changes a role or makes a new 'teacher role' which all teachers now use?

D21. $currenturl urly should be set properly. You should know what your current rul is?

cron.php

D22. My gut feeling is that badge_review_cron() sql would perform better with JOIN on course rather than subselect. It'd be good to use the constant there rather than raw balue too.

edit.php

D23. Looks like its missing a capability check?

D16 "I thought we were moving away from tabs in favour of navigation." It's fine to keep tabs in some places, but single-level only. The number of people I've seen who delete the nav block altogether because they hate it ....

Hey Dan,

Thanks for all the feedback, really useful!

Yuliya will be on to these tomorrow, but I thought I'd comment on a few:

D1. I agree, I think we could use badge_type or type for this property. The constants are called BADGE_TYPE_* so that would be consistent.

D17. Agreed, but in MDL-35991 Petr mentions "In any case we should not be passing URLs in GET parameters because some "security" hacks might strip them, there were a few reports already…". Is there a preferred alternative?

D20. Issuing by role is really non-conventional, is there a reason capabilities don't work in this situation?

The use case is a course creator who wants to allow students to issue each other a particular badge (perhaps there's an element of peer-review or just to allow students to reward each other in some way). But they still want to restrict issuing of a different badge within the same course. If awarding is controlled by a capability, you can't let the students award one badge but not another. Another example might be a badge that must be awarded by multiple roles (like teacher AND assessor, but not by two teachers).

D20b. What happens if a somebody changes a role or makes a new 'teacher role' which all teachers now use?

In that situation they would need to clone the badge, modify the criteria for the clone, enable the clone and disable the original. It is a bit more work for the badge creator but the reason it is needed is because of our "no changing badge criteria once issued" rule, not because we're using roles instead of capabilities.

We've had a lot of discussion about the whole "no changing badge criteria" rule. It would be nice if it wasn't required but at this stage we've kept it due to concerns about fairness. It would be weird for different users to receive the same badge for completing different criteria. By cloning the badge it gives the course creator the chance to 'version' their badges and provides a permanent record of the criteria for each badge.

D17. Agreed, but in MDL-35991 Petr mentions "In any case we should not be passing URLs in GET parameters because some "security" hacks might strip them, there were a few reports already…". Is there a preferred alternative?

Passing a PARAM_LOCALURL around. There is a moodle_url function out_as_local_url() which will give you one for passing around - quiz uses this and some places.

Note that Petr may not agree with me. I don't think he thinks its possible to pass any urls around, but I don't see any better solutions (or solutions from him). The problem is overly agressive firewall/filtering things which don't like urls encoded in the url, but I think if we just pass around the path part it keeps most of them at bay. I wouldn't be surprised if the same firewall/filtering products also strip out the referrer header too, so I don't think thats an improvement.

D17. Okay makes sense. The only other solution I can think of is passing a string or integer code for each possible destination then handling it like this:

switch ($urlcode} {
case 'manage':
    redirect($CFG->wwwroot . '/url/to/manage/page';
    break;
case 'view':
    redirect($CFG->wwwroot . '/url/to/view/page';
    break;
case 'home':
default:
    redirect($CFG->wwwroot);
    break;
}

It's no good if the desired destination is dynamic though (like if you want to return back where you came from).

PARAM_LOCALURL works for me.

Tim had problems with localurl in quiz recently, woraround was to base64 encode the parameter.

Hi Dan,

Great to see so much feedback I think I fixed almost everything, except these:

D8. It is difficult to catch those variable names right now, I am fixing them when I come across any

D9. We need to send badge images attached to emails. I tried to use message_send, but as far as I remember it had no support for attachments. Can you recommend any other way?

D13. Thanks for letting us know. I couldn't find an issue in Moodle tracker. Can you give us a link please?

D16. Not sure how to do this. I override navigation because course level badges and site level badges should be active in their own navigation area, but they still share the same files (index.php, view.php, etc.). What I wanted to do is when, for example, I edit a course badge I want course navigation to be active on 'Manage badges' node. I couldn't find another way of doing that.

D19. assertion.php should not be restricted. This is a public assertion URL that has to be accessible by anyone to validate badges. badge.php is a similar public page with only difference that when user is logged in they can see navigation menu.

D20. I think Simon explained it, so I haven't done anything for this one.

D22. I am currently running some tests to check which query will perform faster. Already moved from IN to EXISTS

All changes are up in github repo already.

(Sorry, catching up)

mybadges.php
D24. Looks like this file is lacking sesskey protection from csrf.

D25. The folder /utils/ feels a bit to me like it should be lib to be moodley. I don't feel too strongly about this though.

utils/backpacklib.php
D26. OpenBadgesBackpackHandler::curl_request would be better if it was using Moodles curl class in filelib (I am no fan of that class, but it does handle proxy servers etc). It looks like this won't take into considering Moodle's proxy configuraiton settings. Same for check_backpack_accessibility

D27. award_criteria_social.php is empty

D28. It's great, music to our ears that you've got some behat and unit tests, it'd be great to have a lot more too

D29. Just fyi really. We'd need to have the commits squashed won a bit and including this issue number in order to integrate them (I know its still work in progress) http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages

Anyway, as I said a the outset, I think this is looking pretty good. I'm sure i've missed other things on this first run through, but considering the amount of code the list of issues is very small. Thanks a lot!

D9. We need to send badge images attached to emails. I tried to use message_send, but as far as I remember it had no support for attachments. Can you recommend any other way?

No, I don't think so. Hmm.

D13. Thanks for letting us know. I couldn't find an issue in Moodle tracker. Can you give us a link please?

The saga which seems to be: MDL-35875

D16. Not sure how to do this. I override navigation because course level badges and site level badges should be active in their own navigation area, but they still share the same files (index.php, view.php, etc.). What I wanted to do is when, for example, I edit a course badge I want course navigation to be active on 'Manage badges' node. I couldn't find another way of doing that.

Yeah, I see. I think I might be asking for the wrong thing. Need to check with Sam.

D19. assertion.php should not be restricted.

How about if openbadges is disabled

Thanks a lot for the fast response!

D19. Done. Although users who had earned the badges before openbadges feature was disabled will not be able to verify them any more.
D24. Done.
D25. Done.
D26. Done. Good point. Never thought that Moodle had its own curl class. Using it now, but it looks like all curl requests are doing much more than I really need for badges.
D27. Done. It was one of the fututre dev idea. Removed for now.
D28. I am going to write more tests soon. I stopped half way through writing behat tests as there was no filepicker support at that time. I think MDL-38184 resolved that issue couple weeks ago, so I will need to get back to that as well.
D29. Will do.

Hi Dan,

I created a new branch on https://github.com/totara/openbadges and put all the commits in one. I updated pull master branch in the tracker.

Please let me know if there are any problems, as I am submitting code to Moodle for the first time and not sure if I am doing everything right

Thanks!

It looks to me there's probably been enough peer review so I'm submitting this for final integration review.

I notice there is still a README.md in there which isn't relevant for integration

Fixed that. Let me know if everything is ok. We have 4 day Easter weekend starting tomorrow, but I will make sure to check if there are any issues.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

Thanks!

I have rebased against master, and updated the user profile to use the new approach from MDL-35875.

The user profile does look a bit odd (the way that profile fields reflow around the user photos on the left) but it happens with all fields not just badges so I am assuming it is intentional.

This is a big feature for Moodle - thanks for working on it.

Some things I spotted while reviewing this code:

Warnings in lib/navigationlib.php during install need empty() check:

-        if ($CFG->enablebadges && $iscurrentuser &&
+        if (!empty($CFG->enablebadges) && $iscurrentuser &&

-        if ($CFG->enablebadges && has_capability('moodle/badges:viewbadges', $this->page->context)) {
+        if (!empty($CFG->enablebadges) && has_capability('moodle/badges:viewbadges', $this->page->context)) {

2. Include non https include:

+ if ($CFG->badges_allowexternalbackpack)

This will produce insecure content warnings on sites with https.

3. Missing require_sesskey in "badges/action.php" if ($copy) section

4. Typo in language file (accesed -> accessed)

5. Missing require_sesskey in "badges/index.php" activate/deactivate actions

6. New svg icons need SVG fix.

7. Include for badges in admin/settings/badges.php should be inside the if for performance.

8. There is no 8.

Although there are a bunch of points here - I fixed most of them in integration.

The only one I didn't fix was 2. I'll add a new issue for that.

Final note: Yay unit tests!

I just committed an additional commit fixing the difference between xmldb generated install.xml file and the one that was there.

Must've been manually editing the xml file, the shame! :-P

BTW my +1 to have this on by default.

Quick question, what do you need in terms of testing instructions for a big feature like this?

So far we have these docs:

http://docs.moodle.org/dev/OpenBadges_User_Documentation

Plus some of the original specs:

http://docs.moodle.org/dev/openbadges
https://github.com/totara/openbadges-docs

Is that enough? If not do you have any good examples of testing instructions for a big feature we could use as a starting point?

Thanks for coming up with http://docs.moodle.org/dev/OpenBadges_User_Documentation - very helpful

I could use it to write a few QA tests if you like.

I have a question. After reading the user docs and the openbadges spec at dev, I wasn't certain what "complete an activity" means. Specifically, can "completion" be restricted to "successful completion," i.e. earning a passing grade at an activity? The regular activity completion criteria don't allow that to be specified-- only "student must receive a grade to complete this activity." Is the information on http://docs.moodle.org/23/en/Activity_completion_settings what is needed here, i.e. the activity needs to be edited in the gradebook and a "Grade to pass" value set? It might be helpful to include a reference and link to this page, if so.

@Helen: Just wanted to let you know that I updated user documentation screenshots to reflect the latest changes. If you need any more information or something appears unclear, please let me know.

Hi,

Sorry I'm late here, glad to see that new features comes with feature files, just a couple of things:

• The features should be tagged according to the frankenstyle component name (http://docs.moodle.org/dev/Frankenstyle) I don't know if badges is a new core subsystem
• The fixtures like 'badge.png' can be placed in tests/fixtures/ like other fixtures (see lib/tests/fixtures/) so if in future phpunit tests can also use them if necessary
• Features or scenarios that includes files uploads should be tagged as @_only_local as they will cause a failure in CI servers or systems using a remote Selenium server (http://docs.moodle.org/dev/Acceptance_testing#Tests_filters) I've just added tagging info to http://docs.moodle.org/dev/Acceptance_testing#Writing_features (last point)

The scenarios contents looks perfect

IMO there is no need to add an extra commit only for this, I can include this changes in https://tracker.moodle.org/browse/MDL-38613 before the 2.5 release

I've updated the frankenstyle doc. Should be core_badges as it is a new core subsystem

We are still looking - but the behat tests for this appear to be failing with a session related issue. David will post some more info soon (and I'll verify this again tomorrow).

Hi,

This is one of the apache logs I see:

[Wed Apr 03 17:02:37 2013] [error] [client 192.168.100.174] Cannot read session record, referer: http://192.168.100.174/master/badges/newbadge.php?type=1
[Wed Apr 03 17:02:37 2013] [error] [client 192.168.100.174] Default exception handler: Unsupported redirect detected, script execution terminated Debug: \nError code: redirecterrordetected\n* line 2422 of /lib/weblib.php: moodle_exception thrown\n* line 2867 of /lib/moodlelib.php: call to redirect()\n* line 56 of /repository/repository_ajax.php: call to require_login()\n, referer: http://192.168.100.174/master/badges/newbadge.php?type=1
[Wed Apr 03 17:02:37 2013] [error] [client 192.168.100.174] Cannot read session record, referer: http://192.168.100.174/master/badges/newbadge.php?type=1
[Wed Apr 03 17:02:37 2013] [error] [client 192.168.100.174] Default exception handler: Incorrect sesskey submitted, form not accepted! Debug: \nError code: invalidsesskey\n* line 476 of /lib/setuplib.php: moodle_exception thrown\n* line 33 of /lib/ajax/setuserpref.php: call to print_error()\n, referer: http://192.168.100.174/master/badges/newbadge.php?type=1

Running one single tests is ok, the problem comes after the first scenario involving a new badge finishes and begins the next, if I comment the lines involving the new badge form all works as expected, unfortunately this does not test much...

I've reproduced the problem in the normal moodle site:

1. Go to site administration -> badges -> Add a new badge
2. Fill the form uploading a image file
3. Cancel the form with the cancel button
4. Return to the same Add a new badge page
5. Try to select a file
6. It takes a lot of time returning a Timed out while waiting for session lock.<br />Wait for your current requests to finish and try again later. at the end

Removing qa_test_required label as David informs me that this new feature came with behat tests - brilliant!

David, it took me some time, but I found an issue It seems that ajax.php script is locking a session when getting request from curl takes some time. Adding session_get_instance()->write_close(); after page context is set up fixes it for me.

Yuliya, if you can add a commit on the top of your branch for this fix then we can pull it in, thanks

Thanks Yuliya,

I integrated a patch with that change in it to fix the behat tests.

I was presented with the following error when I tried to upgrade an instance using Oracle.

Debug info: ORA-01408: such column list already indexed
CREATE INDEX a_badgcrit_bad2_ix ON a_badge_criteria (badgeid)
Error code: ddlexecuteerror
Stack trace:

    line 432 of \lib\dml\moodle_database.php: ddl_change_structure_exception thrown
    line 272 of \lib\dml\oci_native_moodle_database.php: call to moodle_database->query_end()
    line 897 of \lib\dml\oci_native_moodle_database.php: call to oci_native_moodle_database->query_end()
    line 88 of \lib\ddl\database_manager.php: call to oci_native_moodle_database->change_database_structure()
    line 77 of \lib\ddl\database_manager.php: call to database_manager->execute_sql()
    line 436 of \lib\ddl\database_manager.php: call to database_manager->execute_sql_arr()
    line 1852 of \lib\db\upgrade.php: call to database_manager->create_table()
    line 1530 of \lib\upgradelib.php: call to xmldb_main_upgrade()
    line 292 of \admin\index.php: call to upgrade_core()

This prevented the upgrade from proceeding.

Commenting out a few index commands in lib/db/upgrade.php allow the upgrade to proceed.

$table->add_index('critid', XMLDB_INDEX_NOTUNIQUE, array('critid'));

$table->add_index('badgeid', XMLDB_INDEX_NOTUNIQUE, array('badgeid'));

$table->add_index('userid', XMLDB_INDEX_NOTUNIQUE, array('userid'));

It looks like (Oracle at least) creates indexes for key fields. (Perhaps some DB expert could confirm that).

That certainly looks like the issue, but according to the link below, Oracle doesn't automatically add an index to foreign keys:

http://stackoverflow.com/questions/4127206/do-i-need-to-create-indexes-on-foreign-keys

Also I wonder why the index on userid on L1896 doesn't break it when the one on badgeid the line before does (both have foreign keys)?

We'll look into this some more tomorrow.

There is some interesting code in lib/ddl/sql_generator.php, getAddKeySQL() function. Comment says:

// If we aren't creating the keys OR if the key is XMLDB_KEY_FOREIGN (not underlying index generated
// automatically by the RDBMS) create the underlying (created by us) index (if doesn't exists)

which seems to suggest Moodle is automatically creating an index on all foreign keys (which then causes Oracle to die when we try to add indexes too).

All the Moodle DB drivers do create the indexes for all PKs/FKs automatically. Else, it's a bug in the Moodle DB driver.

In the other side, it should behave the same under all DBs (the failure) and it seems different drivers are not breaking under the same circumstances. Surely that should be considered a bug too.

I also had to remove (even more) indexes and keys from the DB setup for the unit tests so they would work under Oracle.

Here's a big smelly dump...

    <TABLE NAME="badge" COMMENT="Defines badge">
      <FIELDS>
        <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
        <FIELD NAME="name" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="description" TYPE="text" NOTNULL="false" SEQUENCE="false"/>
        <FIELD NAME="image" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="timecreated" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
        <FIELD NAME="timemodified" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
        <FIELD NAME="usercreated" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="usermodified" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="issuername" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="issuerurl" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="issuercontact" TYPE="char" LENGTH="255" NOTNULL="false" SEQUENCE="false"/>
        <FIELD NAME="expiredate" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
        <FIELD NAME="expireperiod" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
        <FIELD NAME="type" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="1" SEQUENCE="false" COMMENT="1 = site, 2 = course"/>
        <FIELD NAME="courseid" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
        <FIELD NAME="message" TYPE="text" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="messagesubject" TYPE="text" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="attachment" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="1" SEQUENCE="false" COMMENT="Attach baked badge for download"/>
        <FIELD NAME="notification" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="1" SEQUENCE="false" COMMENT="Message when badge is awarded"/>
        <FIELD NAME="status" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Badge status: 0 = inactive, 1 = active, 2 = active+locked, 3 = inactive+locked, 4 = archived"/>
        <FIELD NAME="nextcron" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
      </FIELDS>
      <KEYS>
        <KEY NAME="primary" TYPE="primary" FIELDS="id"/>
        <KEY NAME="fk_courseid" TYPE="foreign" FIELDS="courseid" REFTABLE="course" REFFIELDS="id"/>
        <KEY NAME="fk_usermodified" TYPE="foreign" FIELDS="usermodified" REFTABLE="user" REFFIELDS="id"/>
        <KEY NAME="fk_usercreated" TYPE="foreign" FIELDS="usercreated" REFTABLE="user" REFFIELDS="id"/>
      </KEYS>
      <INDEXES>
        <INDEX NAME="type" UNIQUE="false" FIELDS="type"/>
      </INDEXES>
    </TABLE>
    <TABLE NAME="badge_criteria" COMMENT="Defines criteria for issuing badges">
      <FIELDS>
        <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
        <FIELD NAME="badgeid" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
        <FIELD NAME="criteriatype" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false" COMMENT="The criteria type we are aggregating"/>
        <FIELD NAME="method" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="1" SEQUENCE="false" COMMENT="1 = all, 2 = any"/>
      </FIELDS>
      <KEYS>
        <KEY NAME="primary" TYPE="primary" FIELDS="id"/>
        <KEY NAME="fk_badgeid" TYPE="foreign" FIELDS="badgeid" REFTABLE="badge" REFFIELDS="id"/>
      </KEYS>
<!--      <INDEXES>
        <INDEX NAME="badgeid" UNIQUE="false" FIELDS="badgeid"/>
        <INDEX NAME="criteriatype" UNIQUE="false" FIELDS="criteriatype"/>
        <INDEX NAME="badgecriteriatype" UNIQUE="true" FIELDS="badgeid, criteriatype"/>
      </INDEXES>
-->    </TABLE>
    <TABLE NAME="badge_criteria_param" COMMENT="Defines parameters for badges criteria">
      <FIELDS>
        <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
        <FIELD NAME="critid" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="name" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="value" TYPE="char" LENGTH="255" NOTNULL="false" SEQUENCE="false"/>
      </FIELDS>
      <KEYS>
        <KEY NAME="primary" TYPE="primary" FIELDS="id"/>
        <KEY NAME="fk_critid" TYPE="foreign" FIELDS="critid" REFTABLE="badge_criteria" REFFIELDS="id"/>
      </KEYS>
      <!--
      <INDEXES>
        <INDEX NAME="critid" UNIQUE="false" FIELDS="critid"/>
      </INDEXES>
    -->
    </TABLE>
    <TABLE NAME="badge_issued" COMMENT="Defines issued badges">
      <FIELDS>
        <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
        <FIELD NAME="badgeid" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
        <FIELD NAME="userid" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
        <FIELD NAME="uniquehash" TYPE="text" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="dateissued" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
        <FIELD NAME="dateexpire" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
        <FIELD NAME="visible" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
        <FIELD NAME="issuernotified" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
      </FIELDS>
      <KEYS>
        <KEY NAME="primary" TYPE="primary" FIELDS="id"/>
        <KEY NAME="fk_badgeid" TYPE="foreign" FIELDS="badgeid" REFTABLE="badge" REFFIELDS="id"/>
        <KEY NAME="fk_userid" TYPE="foreign" FIELDS="userid" REFTABLE="user" REFFIELDS="id"/>
      </KEYS>
<!--      <INDEXES>
        <INDEX NAME="badgeid" UNIQUE="false" FIELDS="badgeid"/>
        <INDEX NAME="userid" UNIQUE="false" FIELDS="userid"/>
        <INDEX NAME="badgeuser" UNIQUE="true" FIELDS="badgeid, userid"/>
      </INDEXES>
-->    </TABLE>
    <TABLE NAME="badge_criteria_met" COMMENT="Defines criteria that were met for an issued badge">
      <FIELDS>
        <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
        <FIELD NAME="issuedid" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/>
        <FIELD NAME="critid" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="userid" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="datemet" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
      </FIELDS>
      <KEYS>
        <KEY NAME="primary" TYPE="primary" FIELDS="id"/>
        <KEY NAME="fk_critid" TYPE="foreign" FIELDS="critid" REFTABLE="badge_criteria" REFFIELDS="id"/>
        <KEY NAME="fk_userid" TYPE="foreign" FIELDS="userid" REFTABLE="user" REFFIELDS="id"/>
        <KEY NAME="fk_issuedid" TYPE="foreign" FIELDS="issuedid" REFTABLE="badge_issued" REFFIELDS="id"/>
      </KEYS>
    </TABLE>
    <TABLE NAME="badge_manual_award" COMMENT="Track manual award criteria for badges">
      <FIELDS>
        <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
        <FIELD NAME="badgeid" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="recipientid" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="issuerid" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="issuerrole" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="datemet" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
      </FIELDS>
      <KEYS>
        <KEY NAME="primary" TYPE="primary" FIELDS="id"/>
<!--        <KEY NAME="fk_badgeid" TYPE="foreign" FIELDS="id" REFTABLE="badge" REFFIELDS="id"/>
        <KEY NAME="fk_recipientid" TYPE="foreign" FIELDS="recipientid" REFTABLE="user" REFFIELDS="id"/>
        <KEY NAME="fk_issuerid" TYPE="foreign" FIELDS="issuerid" REFTABLE="user" REFFIELDS="id"/>
        <KEY NAME="fk_issuerrole" TYPE="foreign" FIELDS="issuerrole" REFTABLE="role" REFFIELDS="id"/>
-->      </KEYS>
    </TABLE>
    <TABLE NAME="badge_backpack" COMMENT="Defines settings for connecting external backpack">
      <FIELDS>
        <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
        <FIELD NAME="userid" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
        <FIELD NAME="email" TYPE="char" LENGTH="100" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="backpackurl" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="backpackuid" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="backpackgid" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false"/>
        <FIELD NAME="autosync" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
        <FIELD NAME="password" TYPE="char" LENGTH="50" NOTNULL="false" SEQUENCE="false"/>
      </FIELDS>
      <KEYS>
        <KEY NAME="primary" TYPE="primary" FIELDS="id"/>
        <KEY NAME="fk_userid" TYPE="foreign" FIELDS="userid" REFTABLE="user" REFFIELDS="id"/>
      </KEYS>
      <!--
      <INDEXES>
        <INDEX NAME="userid" UNIQUE="false" FIELDS="userid"/>
      </INDEXES>
    -->
    </TABLE>
  </TABLES>

Thanks, Michael. I don't really have any experience with using Oracle, but I am trying to set up an environment right now and will try test if anything else is broken. Would you like me to submit a patch with any fixes I make (including install and upgrade scripts that were already mentioned here)?

Hi,

I'm working on a patch for the oracle issue. Seems advantageous to try and fix this before it goes into moodle.git.

WIP:
https://github.com/danpoltawski/moodle/compare/a8fdb36...openbadges-fixes

I have verified upgrade and clean install now, so I think that this can be pulled:
git pull git://github.com/danpoltawski/moodle.git openbadges-fixes

Thanks very much Dan - the patches look good to me.

In the other side, it should behave the same under all DBs (the failure) and it seems different drivers are not breaking under the same circumstances. Surely that should be considered a bug too.

I've created MDL-38972 for this DB driver problem.

Thanks Dan - the oracle fixes have been integrated to master now.

Hi Guys,

Really great feature. The interface is easy to use and the code looks great as well, well done. Thanks for this contribution.

There was an issue where I received the following error:

"Debug info:
Error code: missingparam
Stack trace:

line 476 of /lib/setuplib.php: moodle_exception thrown
line 532 of /lib/moodlelib.php: call to print_error()
line 1033 of /lib/sessionlib.php: call to required_param()
line 1044 of /lib/sessionlib.php: call to confirm_sesskey()
line 127 of /badges/index.php: call to require_sesskey()"

However, to continue testing without delay I created a quick fix which can be found at, please pull this: https://github.com/markn86/moodle/commit/c51776543

Other than that I did not experience any other issues! Thanks!

Thanks Mark - good spotting. I have pulled in that fix for now.

Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking.

line 1289 of \lib\changes.php: call to debugging()
line 281 of \lib\are.php: call to moodleform->detectMissingThanks()
line 202 of \lib\now.php: call to moodleform->_is_poor_form()
line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

I'm getting this Error message after updating Moodle (master) on my localhost server.
Is this something to do with MDL-35073 (Open badges integration)?

 Notice: Undefined property: stdClass::$enablebadges in C:\wamp\www\moodle\lib\navigationlib.php on line 2463
Call Stack
#	Time	Memory	Function	Location
1	0.0030	815560	{main}( )	..\view.php:0
2	0.4500	44661872	core_renderer->header( )	..\view.php:240
3	0.4791	45491168	core_renderer->render_page_layout( )	..\outputrenderers.php:768
4	0.4796	45606256	include( 'C:\wamp\www\moodle\theme\afterburner\layout\default.php' )	..\outputrenderers.php:838
5	0.4796	45606600	moodle_page->has_navbar( )	..\default.php:4
6	0.4800	45613192	navbar->has_items( )	..\pagelib.php:766
7	0.4803	45627464	global_navigation->initialise( )	..\navigationlib.php:2943
8	0.5413	49458992	global_navigation->add_course_essentials( )	..\navigationlib.php:1151

( ! ) Notice: Undefined property: stdClass::$enablebadges in C:\wamp\www\moodle\lib\badgeslib.php on line 894
Call Stack
#	Time	Memory	Function	Location
1	0.0030	815560	{main}( )	..\view.php:0
2	0.4500	44661872	core_renderer->header( )	..\view.php:240
3	0.4791	45491168	core_renderer->render_page_layout( )	..\outputrenderers.php:768
4	0.4796	45606256	include( 'C:\wamp\www\moodle\theme\afterburner\layout\default.php' )	..\outputrenderers.php:838
5	0.7921	57001920	block_manager->region_has_content( )	..\default.php:6
6	0.7921	57001920	block_manager->ensure_content_created( )	..\blocklib.php:353
7	0.7922	57002128	block_manager->create_block_contents( )	..\blocklib.php:1003
8	0.7922	57002720	block_base->get_content_for_output( )	..\blocklib.php:951
9	0.7923	57004856	block_base->formatted_contents( )	..\moodleblock.class.php:232
10	0.7923	57004856	block_settings->get_content( )	..\moodleblock.class.php:284
11	0.7937	57071080	moodle_page->__get( )	..\pagelib.php:0
12	0.7937	57071168	moodle_page->magic_get_settingsnav( )	..\pagelib.php:717
13	0.7938	57077584	settings_navigation->initialise( )	..\pagelib.php:701
14	0.7938	57077712	settings_navigation->load_course_settings( )	..\navigationlib.php:3179
15	0.8260	59299472	badges_add_course_navigation( )	..\navigationlib.php:3548

Yes it is Mary, thanks for the report. I've created MDL-38997 and will push a fix.

Hi,

We just noticed a bad regression from this where the merge reintroduced the course reports in navigation, reversing the work of MDL-31983 - oops!

I've created MDL-39015 and going to do an urgent fix!

Re-adding the qa_test_required label, as the behat tests don't cover all functionality, and it does no harm to have some test duplication, especially with a new feature like Open Badges which a lot of people will be keen to try out!

Removing qa_test_required label as I have added MDLQA-5257,MDLQA-5258,MDLQA-5260, MDLQA-5261,MDLQA-5262,MDLQA-5263, MDLQA-5264 -which I think should keep testers happy.

Removing docs_required as badges are documented here http://docs.moodle.org/25/en/Badges (I suspect dev_docs required should be removed too but I will leave that to a dev to make that decision

Fred found a problem with the icons in this, which is being tracked in MDL-40442.