Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-26384

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

          Attachments

            Activity

            Hide
            ankit_frenz Ankit Agarwal added a comment -

            up for review!
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - up for review! Thanks
            Hide
            andyjdavis 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
            andyjdavis 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
            andyjdavis Andrew Davis added a comment -

            Looks good now

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

            Thanks Andrew for the review!
            Sending for integration!

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Andrew for the review! Sending for integration!
            Hide
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

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

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Passed under 21_STABLE and master (so 22_STABLE assumed), while reviewing.
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  12/Mar/12