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 Master Branch:
      wip-MDL-40091-master

      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.

        Gliffy Diagrams

          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: