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
    • Rank:
      15971

      Description

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

        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: