Moodle
  1. Moodle
  2. MDL-40091

Book revision number not updated on edit chapter content

    Details

    • Testing Instructions:
      Hide

      Test 1:
      1 - Locate an existing book resource or create one.
      2 - Note down the course module id from the url: /mod/book/view.php?id=[cmid]
      3 - Connect to the database and execute the following query: select b.* from mdl_course_modules m inner join mdl_book b on b.id = m.instance where module = 3 and m.id = [cmid]
      4 - Note down the value of the column "revision".
      5 - Turn editing on and edit the content or the title of any chapter in the book resource.
      6 - Save your changes and execute the query in step 3.
      7 - The expected result, revision number incremented by one.

      Show
      Test 1: 1 - Locate an existing book resource or create one. 2 - Note down the course module id from the url: /mod/book/view.php?id= [cmid] 3 - Connect to the database and execute the following query: select b.* from mdl_course_modules m inner join mdl_book b on b.id = m.instance where module = 3 and m.id = [cmid] 4 - Note down the value of the column "revision". 5 - Turn editing on and edit the content or the title of any chapter in the book resource. 6 - Save your changes and execute the query in step 3. 7 - The expected result, revision number incremented by one.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      wip-MDL-40091-MOODLE_24_STABLE
    • Pull 2.5 Branch:
      wip-MDL-40091-MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-40091-master
    • Rank:
      50821

      Description

      The revision number does not change when editing a chapter in an existing book resource. However the revision number do change for the following actions:

      • Adding a new chapter.
      • Removing an existing chapter.
      • Hide/Un-hide an existing chapter.
      • Updating the book general information.

        Activity

        Hide
        Dan Marsden added a comment -

        Thanks Mohamed - bouncing this up for peer review (note to Peer reviewer - I haven't looked at/checked Mohamed's patch)

        Show
        Dan Marsden added a comment - Thanks Mohamed - bouncing this up for peer review (note to Peer reviewer - I haven't looked at/checked Mohamed's patch)
        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this, Mohamed, and thanks for working on it, Dan.

        Show
        Michael de Raadt added a comment - Thanks for reporting this, Mohamed, and thanks for working on it, Dan.
        Hide
        Adrian Greeve added a comment -

        Hello,

        I think that the patch looks okay. Just a couple of comments:

        1. I noticed that the version of the master branch that I pulled down doesn't contain the checks that the other branches do.
        2. Is there a reason that we don't want to change the revision number if the chapter becomes a sub-chapter?

        Thanks.

        Show
        Adrian Greeve added a comment - Hello, I think that the patch looks okay. Just a couple of comments: I noticed that the version of the master branch that I pulled down doesn't contain the checks that the other branches do. Is there a reason that we don't want to change the revision number if the chapter becomes a sub-chapter? Thanks.
        Hide
        Dan Marsden added a comment -

        thanks for the speedy review Adrian! - Mohamed are you able to tidy up the master branch to include the check?
        https://github.com/satrun77/moodle/commit/wip-MDL-40091-master

        also - can you please take a look at Adrian's 2nd point above?

        Show
        Dan Marsden added a comment - thanks for the speedy review Adrian! - Mohamed are you able to tidy up the master branch to include the check? https://github.com/satrun77/moodle/commit/wip-MDL-40091-master also - can you please take a look at Adrian's 2nd point above?
        Hide
        Mohamed Alsharaf added a comment -

        Thanks for the review Adrian.

        I will fix these 2 issues today.

        Show
        Mohamed Alsharaf added a comment - Thanks for the review Adrian. I will fix these 2 issues today.
        Hide
        Mohamed Alsharaf added a comment -

        I have fixed the issues in comment 11/Jun/13 6:53 PM

        Show
        Mohamed Alsharaf added a comment - I have fixed the issues in comment 11/Jun/13 6:53 PM
        Hide
        Adrian Greeve added a comment -

        Hi,

        I've had another look at the code.
        I don't think that we need the if statement to check to see if anything has been changed when editing the book chapters.
        Reasons for this are:

        1. If we decide to add additional fields to the chapter information then we would also have to remember to change this if statement to accommodate those changes.
        2. Other parts of code don't do the same rigorous testing of the fields before increasing the revision number, and if we decided that this was the behaviour that we wanted, we should also change those areas.
        3. After talking with Ankit (book module maintainer) he is happy to have the revision number increase even in the user clicks submit without anything being changed.
          Please remove the if statement (or provide a convincing argument to keep them in) and then re-submit.

        Sorry to hold up you submission. I appreciate your hard work on this issue.

        Show
        Adrian Greeve added a comment - Hi, I've had another look at the code. I don't think that we need the if statement to check to see if anything has been changed when editing the book chapters. Reasons for this are: If we decide to add additional fields to the chapter information then we would also have to remember to change this if statement to accommodate those changes. Other parts of code don't do the same rigorous testing of the fields before increasing the revision number, and if we decided that this was the behaviour that we wanted, we should also change those areas. After talking with Ankit (book module maintainer) he is happy to have the revision number increase even in the user clicks submit without anything being changed. Please remove the if statement (or provide a convincing argument to keep them in) and then re-submit. Sorry to hold up you submission. I appreciate your hard work on this issue.
        Hide
        Mohamed Alsharaf added a comment -

        @Adrian it is ok. I have updated the patch.

        Show
        Mohamed Alsharaf added a comment - @Adrian it is ok. I have updated the patch.
        Hide
        Adrian Greeve added a comment -

        @Mohamed,

        This looks fine.
        If you could possibly just tidy up your commit message (MDL-XXXX component: commit message), then you are ready to go for integration.

        Thanks.

        Show
        Adrian Greeve added a comment - @Mohamed, This looks fine. If you could possibly just tidy up your commit message (MDL-XXXX component: commit message), then you are ready to go for integration. Thanks.
        Hide
        Mohamed Alsharaf added a comment -

        @Adrian I have changed the commit message.

        Thanks

        Show
        Mohamed Alsharaf added a comment - @Adrian I have changed the commit message. Thanks
        Hide
        Dan Marsden added a comment -

        Great work Mohamed - thanks for the help reviewing this Adrian.

        Show
        Dan Marsden added a comment - Great work Mohamed - thanks for the help reviewing this Adrian.
        Hide
        Sam Hemelryk added a comment -

        Thanks guys, this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks guys, this has been integrated now
        Hide
        Michael de Raadt added a comment -

        Test result: Success!

        I assume (after reading the comments and talking to Adrian) that "Test 2" is no longer relevant. The revision number did update regardless of whether there was a change to the saved content.

        Tested in 2.3, 2.4, 2.5 and master.

        Show
        Michael de Raadt added a comment - Test result: Success! I assume (after reading the comments and talking to Adrian) that "Test 2" is no longer relevant. The revision number did update regardless of whether there was a change to the saved content. Tested in 2.3, 2.4, 2.5 and master.
        Hide
        Mohamed Alsharaf added a comment -

        @Michael thanks for testing my patch. I will remove test 2 it is to relevant.

        Show
        Mohamed Alsharaf added a comment - @Michael thanks for testing my patch. I will remove test 2 it is to relevant.
        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:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: