Moodle
  1. Moodle
  2. MDL-40572

Update badges assertion to use the new spec

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5.1, 2.6
    • Fix Version/s: 2.6
    • Component/s: Badges
    • Labels:
    • Testing Instructions:
      Hide

      Part 1

      Run unit tests and make sure they are all passing.

      Part 2

      To test, you need at least one badge issued from Moodle, and your web site to be accessible online (because we are testing hosted assertions).

      1. From my badges page go to your issued badge. URL should be in form of badges/badge.php?hash=XXXXXXXXXXX

      2. Manually change URL to look like badges/assertion.php?b=XXXXXXXXX where XXXXXXXXX is the issued badge unique hash. If you changed it correctly, you should see a JSON string that starts with '{"uid":"XXXXXXXXX"'...

      3. Copy either this entire JSON string or just assertion URL. It works both ways.

      4. Go to http://validator.openbadges.org

      5. Paste what you copied into a textarea and click "Check Validity".

      6. After validation is done, you should see a message "Valid", "Spec Version: 1.0.0" similar to attached image

      Show
      Part 1 Run unit tests and make sure they are all passing. Part 2 To test, you need at least one badge issued from Moodle, and your web site to be accessible online (because we are testing hosted assertions). 1. From my badges page go to your issued badge. URL should be in form of badges/badge.php?hash=XXXXXXXXXXX 2. Manually change URL to look like badges/assertion.php?b=XXXXXXXXX where XXXXXXXXX is the issued badge unique hash. If you changed it correctly, you should see a JSON string that starts with ' {"uid":"XXXXXXXXX"' ... 3. Copy either this entire JSON string or just assertion URL. It works both ways. 4. Go to http://validator.openbadges.org 5. Paste what you copied into a textarea and click "Check Validity". 6. After validation is done, you should see a message "Valid", "Spec Version: 1.0.0" similar to attached image
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-40572_master
    • Rank:
      51400

      Description

      Need to update the code to use the new assertion specification https://github.com/mozilla/openbadges/wiki/Assertions

        Issue Links

          Activity

          Hide
          Yuliya Bozhko added a comment -

          First attempt to rewrite badge assertions using new spec!

          For more information see:
          https://github.com/mozilla/openbadges/wiki/Assertion-Specification-Changes
          https://github.com/mozilla/openbadges/wiki/Assertions

          There are quite substantial changes in the new specification. Namely, assertions are now separated into three parts - info about issued badge, info about badge class and info about issuer. Each part is a JSON string itself and should be accessible via public URL.

          Changes/new things:

          1. I moved assertion into a different class, which hopefully will make it easier to add signed assertion a bit later, and do other changes.
          2. badges_get_issued_badge_info() function is now deprecated and I marked it accordingly.
          3. URL of the issued badges and issued badge assertion did not change, so there should be no problems with earlier issued badges. Access to all locally issued badges will now generate new assertion.
          4. Added URL parameters to assertion.php to separate badge class and issuer information.
          5. There are some changes to rendering of issued badges, but only in the way information is retrieved, not in user interface or any methods parameters.

          There are no changes in external badges.

          I tested new assertions through http://validator.openbadges.org and they worked without any issues.

          And finally, this is too bigger change for 2.5.3, so I submitted only master branch version.

          Please let me know what you think

          Show
          Yuliya Bozhko added a comment - First attempt to rewrite badge assertions using new spec! For more information see: https://github.com/mozilla/openbadges/wiki/Assertion-Specification-Changes https://github.com/mozilla/openbadges/wiki/Assertions There are quite substantial changes in the new specification. Namely, assertions are now separated into three parts - info about issued badge, info about badge class and info about issuer. Each part is a JSON string itself and should be accessible via public URL. Changes/new things: I moved assertion into a different class, which hopefully will make it easier to add signed assertion a bit later, and do other changes. badges_get_issued_badge_info() function is now deprecated and I marked it accordingly. URL of the issued badges and issued badge assertion did not change, so there should be no problems with earlier issued badges. Access to all locally issued badges will now generate new assertion. Added URL parameters to assertion.php to separate badge class and issuer information. There are some changes to rendering of issued badges, but only in the way information is retrieved, not in user interface or any methods parameters. There are no changes in external badges. I tested new assertions through http://validator.openbadges.org and they worked without any issues. And finally, this is too bigger change for 2.5.3, so I submitted only master branch version. Please let me know what you think
          Hide
          Dan Poltawski added a comment -

          1. I don't like the class name 'Assertion', its too generic. But since this is for 2.6 only, you could take advantage of http://docs.moodle.org/dev/Automatic_class_loading and create a core_badges_assertion class and get that loaded without inclusion.

          2. Could we unit test this? (I know it gets complicated when connecting to the external providers, but any unit testing which could be doing would be great)

          3. The debugging notice about badges_get_issued_badge_info() should be set to DEBUG_DEVELOPER only. (Gah, we really should change the default for this function, i've created MDL-41803)

          4.

          $badgeclass = optional_param('badgeclass', false, PARAM_BOOL); // Setting to generate Badge Class.
          $issuer = optional_param('issuer', false, PARAM_BOOL); // Setting to generate Issuer.
          
          • Maybe these params would be better are a single 'action' param with different options since the code itself does not allow both actions to happen (if you set issuer to true and badgeclass to true then only badgeclass will happen).
          • Trivial comment, but I think its clearer to say 'Generates badge class if true'.
          Show
          Dan Poltawski added a comment - 1. I don't like the class name 'Assertion', its too generic. But since this is for 2.6 only, you could take advantage of http://docs.moodle.org/dev/Automatic_class_loading and create a core_badges_assertion class and get that loaded without inclusion. 2. Could we unit test this? (I know it gets complicated when connecting to the external providers, but any unit testing which could be doing would be great) 3. The debugging notice about badges_get_issued_badge_info() should be set to DEBUG_DEVELOPER only. (Gah, we really should change the default for this function, i've created MDL-41803 ) 4. $badgeclass = optional_param('badgeclass', false , PARAM_BOOL); // Setting to generate Badge Class . $issuer = optional_param('issuer', false , PARAM_BOOL); // Setting to generate Issuer. Maybe these params would be better are a single 'action' param with different options since the code itself does not allow both actions to happen (if you set issuer to true and badgeclass to true then only badgeclass will happen). Trivial comment, but I think its clearer to say 'Generates badge class if true'.
          Hide
          Yuliya Bozhko added a comment -

          Thanks, Dan! I will have a look at the comments this week.

          Not sure what kind of unit test I can do here, but will try add some.

          Show
          Yuliya Bozhko added a comment - Thanks, Dan! I will have a look at the comments this week. Not sure what kind of unit test I can do here, but will try add some.
          Hide
          Yuliya Bozhko added a comment - - edited

          I think I've got all of your comments, Dan.

          Not sure about the unit test. Open Badges Validator doesn't allow remote request to validate assertions. So I created a format string with to run with unit test method assertStringMatchesFormat().

          Adding testing instructions now.

          Show
          Yuliya Bozhko added a comment - - edited I think I've got all of your comments, Dan. Not sure about the unit test. Open Badges Validator doesn't allow remote request to validate assertions. So I created a format string with to run with unit test method assertStringMatchesFormat(). Adding testing instructions now.
          Hide
          Dan Poltawski added a comment -

          Thanks Yuliya,

          Final thing is to move the badges deprecated function to lib/deprecatedlib.php
          http://docs.moodle.org/dev/Deprecation#Moodle_Core_deprecation_process

          Show
          Dan Poltawski added a comment - Thanks Yuliya, Final thing is to move the badges deprecated function to lib/deprecatedlib.php http://docs.moodle.org/dev/Deprecation#Moodle_Core_deprecation_process
          Hide
          Yuliya Bozhko added a comment -

          Done! Please let me know I need to squash commits.

          Show
          Yuliya Bozhko added a comment - Done! Please let me know I need to squash commits.
          Hide
          Dan Poltawski added a comment -

          Hi Yuliya,

          Yes - please squash the commits.

          Oh and oops.. for some reason I thought about this when reviewing last week but didn't mention it.. If you wanted to you could convert the class to use the core\badges namespace - which might be useful if you do more classes in that namespace in the future. But its up to you if you don't want to change it.

          Show
          Dan Poltawski added a comment - Hi Yuliya, Yes - please squash the commits. Oh and oops.. for some reason I thought about this when reviewing last week but didn't mention it.. If you wanted to you could convert the class to use the core\badges namespace - which might be useful if you do more classes in that namespace in the future. But its up to you if you don't want to change it.
          Hide
          Yuliya Bozhko added a comment -

          No problem. Are there any docs on namespaces to have a look at? I might do that if there is not too much work to update classes. We have a pretty busy schedule this week...

          Show
          Yuliya Bozhko added a comment - No problem. Are there any docs on namespaces to have a look at? I might do that if there is not too much work to update classes. We have a pretty busy schedule this week...
          Hide
          Yuliya Bozhko added a comment -

          Commits squashed and branch rebased.

          Decided not to switch to namespaces just yet. Hope it's ok.

          Show
          Yuliya Bozhko added a comment - Commits squashed and branch rebased. Decided not to switch to namespaces just yet. Hope it's ok.
          Hide
          Dan Poltawski added a comment -

          all good for me

          Show
          Dan Poltawski added a comment - all good for me
          Hide
          Sam Hemelryk added a comment -

          Ok this has been integrated now. I had a chat with Yuliya as there was one thing I noted that needs to happen.
          It's not crucial to have it in this week, but is crucial to have it sorted before the release.
          We've agreed to open a new issue to look at this later today/during the week.

          Many thanks
          SAm

          Show
          Sam Hemelryk added a comment - Ok this has been integrated now. I had a chat with Yuliya as there was one thing I noted that needs to happen. It's not crucial to have it in this week, but is crucial to have it sorted before the release. We've agreed to open a new issue to look at this later today/during the week. Many thanks SAm
          Hide
          Sam Hemelryk added a comment -

          Linking to MDL-42072 to see output and header managed better.

          Show
          Sam Hemelryk added a comment - Linking to MDL-42072 to see output and header managed better.
          Hide
          Jérôme Mouneyrac added a comment -

          It was fun to test Thanks Yuliya.

          Show
          Jérôme Mouneyrac added a comment - It was fun to test Thanks Yuliya.
          Hide
          Dan Poltawski added a comment -

          You did it!

          Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

          Show
          Dan Poltawski added a comment - You did it! Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: