Moodle
  1. Moodle
  2. MDL-26384

Click 'turn editing on' on tag/index.php will jump to tag/search.php

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.1, 2.1.4, 2.2.1, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Tags
    • Labels:
    • Testing Instructions:
      Hide
      1. Login as Admin and goto sitepages>tags from Navigation
      2. If you don't have tags, add a few by clicking on manage tags link
      3. Now visit the tags page something like /moodle/tag/index.php?tag=moodle where moodle is the tagname (Make sure you access the page using tag=tagname not id=tagid)
      4. Click on "turn editing on"
      5. Make sure Editing is on now and you are on the same page.
      6. Turn Editing off
      7. Make sure Editing is off now and you are on the same page
      Show
      Login as Admin and goto sitepages>tags from Navigation If you don't have tags, add a few by clicking on manage tags link Now visit the tags page something like /moodle/tag/index.php?tag=moodle where moodle is the tagname (Make sure you access the page using tag=tagname not id=tagid) Click on "turn editing on" Make sure Editing is on now and you are on the same page. Turn Editing off Make sure Editing is off now and you are on the same page
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-26384-master

      Description

      Click 'turn editing on' on tag/index.php will jump to tag/search.php

        Gliffy Diagrams

          Activity

          Hide
          Ankit Agarwal added a comment -

          up for review!
          Thanks

          Show
          Ankit Agarwal added a comment - up for review! Thanks
          Hide
          Andrew Davis added a comment -

          I'm not sure if there is really a single best way but I have an alternative for you to consider. There's nothing wrong with what you have done so feel free to ignore me and put this up for integration

          My issue is that both $tag->id and $tagid contain the same value and both are being used.

          if ($tagname) {
              $tag = tag_get('name', $tagname, '*');
              $tagid = $tag->id;
          } else if ($tagid) {
              $tag = tag_get('id', $tagid, '*');
          }
          

          Instead you could do this

          if ($tagname) {
              $tag = tag_get('name', $tagname, '*');
          } else if ($tagid) {
              $tag = tag_get('id', $tagid, '*');
          }
          unset($tagid);
          

          and change any later references to $tagid to $tag->id. There's only 1. The parameters go into the database. Objects come out. From then on you refer to the objects rather than the data received from the client.

          Show
          Andrew Davis added a comment - I'm not sure if there is really a single best way but I have an alternative for you to consider. There's nothing wrong with what you have done so feel free to ignore me and put this up for integration My issue is that both $tag->id and $tagid contain the same value and both are being used. if ($tagname) { $tag = tag_get('name', $tagname, '*'); $tagid = $tag->id; } else if ($tagid) { $tag = tag_get('id', $tagid, '*'); } Instead you could do this if ($tagname) { $tag = tag_get('name', $tagname, '*'); } else if ($tagid) { $tag = tag_get('id', $tagid, '*'); } unset($tagid); and change any later references to $tagid to $tag->id. There's only 1. The parameters go into the database. Objects come out. From then on you refer to the objects rather than the data received from the client.
          Hide
          Andrew Davis added a comment -

          Looks good now

          Show
          Andrew Davis added a comment - Looks good now
          Hide
          Ankit Agarwal added a comment -

          Thanks Andrew for the review!
          Sending for integration!

          Show
          Ankit Agarwal added a comment - Thanks Andrew for the review! Sending for integration!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          PS: Anyway, always I see those edit on/off buttons, I think they should be GET actions and not POST ones. Often they lead to borked navigation (going back) and, after all they are not saving anything.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! PS: Anyway, always I see those edit on/off buttons, I think they should be GET actions and not POST ones. Often they lead to borked navigation (going back) and, after all they are not saving anything.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passed under 21_STABLE and master (so 22_STABLE assumed), while reviewing.

          Show
          Eloy Lafuente (stronk7) added a comment - Passed under 21_STABLE and master (so 22_STABLE assumed), while reviewing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers.

          Many thanks & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers. Many thanks & ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: