Moodle
  1. Moodle
  2. MDL-39863

Some bad code shows up when doing a bulk delete of users.

    Details

    • Testing Instructions:
      Hide
      1. Log in as admin and turn debugging on.
      2. Go to Site administration ► Users ► Accounts ► Bulk user actions
      3. Select multiple users in available user list and add them to selection
      4. Select delete option in "With selected users" drop-down
      5. Make sure you don't see any error/notice.
      6. Repeat steps 2-3 with current user selected
      7. Make sure you see notice in red, with current user name not deleted.
      8. Repeat steps 2-3
      9. Select confirm option in "With selected users" drop-down
      10. Make sure you don't see any error/notice.
      Show
      Log in as admin and turn debugging on. Go to Site administration ► Users ► Accounts ► Bulk user actions Select multiple users in available user list and add them to selection Select delete option in "With selected users" drop-down Make sure you don't see any error/notice. Repeat steps 2-3 with current user selected Make sure you see notice in red, with current user name not deleted. Repeat steps 2-3 Select confirm option in "With selected users" drop-down Make sure you don't see any error/notice.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull 2.4 Branch:
      wip-mdl-39863-m24
    • Pull 2.5 Branch:
      wip-mdl-39863-m25
    • Pull Master Branch:
      wip-mdl-39863
    • Rank:
      50606

      Description

      This is not a bug, it is more of a "code cleanup" suggestion.

      For awhile (at least back to Moodle 2.4, and maybe earlier), when I do a bulk delete of users (with debug on), I get some error messages similar to what I have attached. Everything works fine, but I just thought that I would bring this to your attention just in case some other problem is occurring.

      Steps to reproduce:

      1. Log in as admin and turn debugging on
      2. Go to Site administration > Users > Accounts > Bulk user actions
      3. Select few users
      4. Select delete option and confirm.
        You will see following notice.
        You should really redirect before you start page output
        line 728 of /lib/outputrenderers.php: call to debugging()
        line 2540 of /lib/weblib.php: call to core_renderer->redirect_message()
        line 38 of /admin/user/user_bulk_delete.php: call to redirect()
        

        Activity

        Hide
        Helen Foster added a comment -

        Rick, thanks for your report. Please could you just edit this issue and copy and paste the debugging message into the issue description (or into a comment) so that it can be found in a search.

        Show
        Helen Foster added a comment - Rick, thanks for your report. Please could you just edit this issue and copy and paste the debugging message into the issue description (or into a comment) so that it can be found in a search.
        Hide
        Rick Jerz added a comment -

        I think that the debugging message is in my attachment at the bottom (isn't it?).

        Show
        Rick Jerz added a comment - I think that the debugging message is in my attachment at the bottom (isn't it?).
        Hide
        Helen Foster added a comment -

        Yes, it is, however text in images can't be found in a search.

        Show
        Helen Foster added a comment - Yes, it is, however text in images can't be found in a search.
        Hide
        Rajesh Taneja added a comment -

        Thanks Helen and Rick,

        I have added debug message in description.

        Show
        Rajesh Taneja added a comment - Thanks Helen and Rick, I have added debug message in description.
        Hide
        Rick Jerz added a comment -

        Helen, thanks for your comments. I didn't think about someone wanting to search when I provided my attachment. Sometimes it is good to see things exactly as they appear in a browser, but sometimes, as you pointed out, including the actual code is better. I will be more careful in the future so that my comments can be best utilized by the developers.

        Thanks again,

        Rick

        Show
        Rick Jerz added a comment - Helen, thanks for your comments. I didn't think about someone wanting to search when I provided my attachment. Sometimes it is good to see things exactly as they appear in a browser, but sometimes, as you pointed out, including the actual code is better. I will be more careful in the future so that my comments can be best utilized by the developers. Thanks again, Rick
        Hide
        Adrian Greeve added a comment -

        Hi Raj,

        Your patch looks good. My only suggestion would be to fix admin/user/user_bulk_confirm.php as it has the same problem (and update the testing instructions).

        Thanks.

        Show
        Adrian Greeve added a comment - Hi Raj, Your patch looks good. My only suggestion would be to fix admin/user/user_bulk_confirm.php as it has the same problem (and update the testing instructions). Thanks.
        Hide
        Rajesh Taneja added a comment -

        Thanks Adrian,

        Added fix for bulk_confirm, pushing it for integration.

        Show
        Rajesh Taneja added a comment - Thanks Adrian, Added fix for bulk_confirm, pushing it for integration.
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Dan Poltawski added a comment -

        Hi Raj,

        Sorry but this does not look like complete solution.

        If any of the conditions which happen to call $OUTPUT->notification() in the two files happen, then we will be outputing html with a div tags without first setting any headers.

        Show
        Dan Poltawski added a comment - Hi Raj, Sorry but this does not look like complete solution. If any of the conditions which happen to call $OUTPUT->notification() in the two files happen, then we will be outputing html with a div tags without first setting any headers.
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Rajesh Taneja added a comment -

        Thanks for pointing this Dan,

        I have fixed this by replacing redirect with confirmation page.
        Sending it back for your review.

        Show
        Rajesh Taneja added a comment - Thanks for pointing this Dan, I have fixed this by replacing redirect with confirmation page. Sending it back for your review.
        Hide
        Sam Hemelryk added a comment -

        Thanks Raj - this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks Raj - this has been integrated now.
        Hide
        Mark Nelson added a comment -

        Hey Raj, didn't see any notices and was able to delete a student successfully, but not my own account. Works as expected. Passing.

        Show
        Mark Nelson added a comment - Hey Raj, didn't see any notices and was able to delete a student successfully, but not my own account. Works as expected. Passing.
        Hide
        Dan Poltawski added a comment -

        Thanks for your contributions!

        _main:
        @ BB#0:
                push    {r7, lr}
                mov     r7, sp
                sub     sp, #4
                movw    r0, :lower16:(L_.str-(LPC0_0+4))
                movt    r0, :upper16:(L_.str-(LPC0_0+4))
        LPC0_0:
                add     r0, pc
                bl      _printf
                movs    r1, #0
                movt    r1, #0
                str     r0, [sp]                @ 4-byte Spill
                mov     r0, r1
                add     sp, #4
                pop     {r7, pc}
        
                .section        __TEXT,__cstring,cstring_literals
        L_.str:                                 @ @.str
                .asciz   "This code is now upstream!"
        
        Show
        Dan Poltawski added a comment - Thanks for your contributions! _main: @ BB#0: push {r7, lr} mov r7, sp sub sp, #4 movw r0, :lower16:(L_.str-(LPC0_0+4)) movt r0, :upper16:(L_.str-(LPC0_0+4)) LPC0_0: add r0, pc bl _printf movs r1, #0 movt r1, #0 str r0, [sp] @ 4- byte Spill mov r0, r1 add sp, #4 pop {r7, pc} .section __TEXT,__cstring,cstring_literals L_.str: @ @.str .asciz "This code is now upstream!"

          People

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

            Dates

            • Created:
              Updated:
              Resolved: