Moodle
  1. Moodle
  2. MDL-30170

Adding back a deleted MNet peer should undelete it

    Details

    • Testing Instructions:
      Hide

      1. Prepare two sites with MNet enabled
      2. At one site, add the other site as MNet peer host
      3. Delete the host
      4. TEST: make sure that the deleted host is listed below the table at the Manage peers page
      5. Click the name of the deleted host or try to re-add it as a new host
      6. TEST: When editing the deleted host record, make sure there is the radio selector highlighted and the instructions are clear on how to undelete the host.

      Show
      1. Prepare two sites with MNet enabled 2. At one site, add the other site as MNet peer host 3. Delete the host 4. TEST: make sure that the deleted host is listed below the table at the Manage peers page 5. Click the name of the deleted host or try to re-add it as a new host 6. TEST: When editing the deleted host record, make sure there is the radio selector highlighted and the instructions are clear on how to undelete the host.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30170-mnet-undelete

      Description

      When adding back a deleted peer, the peer should probably default to no longer being deleted

      I'd argue that, if a peer is added and it already exists as a deleted peer, then that peer should be undeleted automatically

        Gliffy Diagrams

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this and providing a solution. I'll ask Jerome to peer review your solution.

          Show
          Michael de Raadt added a comment - Thanks for reporting this and providing a solution. I'll ask Jerome to peer review your solution.
          Hide
          Jérôme Mouneyrac added a comment -

          I deleted a peer I had just created. When I tried to add it again, I saw an error message asking me to edit it. I edited it, and I saw an option to mark it undeleted. Then the peer was listed again.

          I would say not a bug. The usability could be improved, but I heard that Mnet is going to be removed from 2.3, so I don't think this usability will be improved for this case.

          Show
          Jérôme Mouneyrac added a comment - I deleted a peer I had just created. When I tried to add it again, I saw an error message asking me to edit it. I edited it, and I saw an option to mark it undeleted. Then the peer was listed again. I would say not a bug. The usability could be improved, but I heard that Mnet is going to be removed from 2.3, so I don't think this usability will be improved for this case.
          Hide
          Jérôme Mouneyrac added a comment -

          From reading the issue again, I think Andrew just suggested to change the Deleted field to 'Not deleted'. I understand why Andrew suggested to change it (considering the value as a default) and why the designer implemented this way (considering the value as a state).

          As I said as Mnet is going to be removed from 2.3 it's probably not going to be changed.
          Anyway, thanks Andrew for taking some time to write this report

          Show
          Jérôme Mouneyrac added a comment - From reading the issue again, I think Andrew just suggested to change the Deleted field to 'Not deleted'. I understand why Andrew suggested to change it (considering the value as a default) and why the designer implemented this way (considering the value as a state). As I said as Mnet is going to be removed from 2.3 it's probably not going to be changed. Anyway, thanks Andrew for taking some time to write this report
          Hide
          Jérôme Mouneyrac added a comment -

          Ah last info from the chat, Mnet is not going to be removed, it's going to be re-implemented And it's unsure for 2.3.

          Show
          Jérôme Mouneyrac added a comment - Ah last info from the chat, Mnet is not going to be removed, it's going to be re-implemented And it's unsure for 2.3.
          Hide
          Andrew Nicols added a comment -

          Jerome,

          Since MNet isn't going away, do you think that this should be integrated after all?

          I've not made any changes to the code, but your comments suggested that it /failed/ peer review because of MNet going away rather than my patch.

          Thanks,

          Andrew

          Show
          Andrew Nicols added a comment - Jerome, Since MNet isn't going away, do you think that this should be integrated after all? I've not made any changes to the code, but your comments suggested that it /failed/ peer review because of MNet going away rather than my patch. Thanks, Andrew
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Hi Andrew,
          from reading my second comment, I think I came to the conclusion that the original designer saw the value in a different way than the patch. So I would not submit it for integration. Mike is it ok to assign this issue to the next scrumm?
          cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - - edited Hi Andrew, from reading my second comment, I think I came to the conclusion that the original designer saw the value in a different way than the patch. So I would not submit it for integration. Mike is it ok to assign this issue to the next scrumm? cheers, Jerome
          Hide
          Andrew Nicols added a comment -

          Hi Michael,

          You weren't a watched on this when Jerome asked whether this could make the next scrum.

          Thanks,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Michael, You weren't a watched on this when Jerome asked whether this could make the next scrum. Thanks, Andrew
          Hide
          Michael de Raadt added a comment -

          Hi, Andrew.

          Thanks for adding me as a watcher.

          It doesn't look like there is development work to be done here, so adding it to the Scrum wouldn't help.

          What we need is a decision on the best approach. Unfortunately the original authors/maintainers have moved on.

          I'll see if we can get another opinion.

          Show
          Michael de Raadt added a comment - Hi, Andrew. Thanks for adding me as a watcher. It doesn't look like there is development work to be done here, so adding it to the Scrum wouldn't help. What we need is a decision on the best approach. Unfortunately the original authors/maintainers have moved on. I'll see if we can get another opinion.
          Hide
          David Mudrak added a comment -

          I just discussed the issue with Andrew at http://moodle.org/mod/cvsadmin/view.php?conversationid=9090#c320867 and we agreed a solution. I am going to do it.

          Show
          David Mudrak added a comment - I just discussed the issue with Andrew at http://moodle.org/mod/cvsadmin/view.php?conversationid=9090#c320867 and we agreed a solution. I am going to do it.
          Hide
          David Mudrak added a comment -

          Submitting for the integration. As agreed with Andrew, the essential issue here was that the radio switcher could be overlooked easily so the user (admin) did not know how to undelete the host actually. This and some other small usability issues should be fixed now.

          Show
          David Mudrak added a comment - Submitting for the integration. As agreed with Andrew, the essential issue here was that the radio switcher could be overlooked easily so the user (admin) did not know how to undelete the host actually. This and some other small usability issues should be fixed now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Note this has been also backported to 20_STABLE (after chat with David, thanks).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Note this has been also backported to 20_STABLE (after chat with David, thanks). Ciao
          Hide
          Rossiani Wijaya added a comment -

          This is working great.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working great. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

          Now... disconnect, relax and enjoy the next days, yay!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao
          Hide
          Mary Evans added a comment - - edited

          I've just found an error in theme/base/style/admin.css which was added by MDL-30170 23/11/2011

          I'm just about to fix it.

          MDL-42453

          Show
          Mary Evans added a comment - - edited I've just found an error in theme/base/style/admin.css which was added by MDL-30170 23/11/2011 I'm just about to fix it. MDL-42453

            People

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

              Dates

              • Created:
                Updated:
                Resolved: