Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: DEV backlog
    • Fix Version/s: 2.1
    • Component/s: Lesson
    • Labels:
      None
    • Rank:
      1231

      Issue Links

        Activity

        Hide
        Aparup Banerjee added a comment -

        1) process_lesson() php_doc mentions MOD/CHOICE
        2) process_lesson_pages() -> process_lesson_page() as we're processing just the one page. also in get_path()
        3) process_lesson_answers() -> process_lesson_page_answer() as we're processing a single answer thats within a page. also that call to write_xml needs to be checked, i'm not sure about providing the 4th arg hardcoded.

        have you tested your xml file with the unit test ? (described in http://docs.moodle.org/en/Development:Backup_1.9_conversion_for_developers#Setting-up_the_development_environment )

        Show
        Aparup Banerjee added a comment - 1) process_lesson() php_doc mentions MOD/CHOICE 2) process_lesson_pages() -> process_lesson_page() as we're processing just the one page. also in get_path() 3) process_lesson_answers() -> process_lesson_page_answer() as we're processing a single answer thats within a page. also that call to write_xml needs to be checked, i'm not sure about providing the 4th arg hardcoded. have you tested your xml file with the unit test ? (described in http://docs.moodle.org/en/Development:Backup_1.9_conversion_for_developers#Setting-up_the_development_environment )
        Hide
        Rossiani Wijaya added a comment -

        Hi Apu,

        Thanks for reviewing.
        1) fixed
        2) renaming it to process_lesson_page()
        3) renaming it to process_lesson_answer(), and removing the 3rd and 4th arguments (it was there for my testing purpose only)

        I have tested the xml file with the unit test. I will attach the xml file.

        Note: updated diff url

        Show
        Rossiani Wijaya added a comment - Hi Apu, Thanks for reviewing. 1) fixed 2) renaming it to process_lesson_page() 3) renaming it to process_lesson_answer(), and removing the 3rd and 4th arguments (it was there for my testing purpose only) I have tested the xml file with the unit test. I will attach the xml file. Note: updated diff url
        Hide
        Rossiani Wijaya added a comment -

        Assigning David as peer review.

        Show
        Rossiani Wijaya added a comment - Assigning David as peer review.
        Hide
        David Mudrak added a comment -

        Hi Rossi. Thanks for your work. Just details:

        1) please replace the get_moduleid() call with the new one as shown in the template.
        2) please get rid of the trailing whitespace

        Show
        David Mudrak added a comment - Hi Rossi. Thanks for your work. Just details: 1) please replace the get_moduleid() call with the new one as shown in the template. 2) please get rid of the trailing whitespace
        Hide
        Rossiani Wijaya added a comment -

        Updated and submit github pull request for David:
        https://github.com/mudrd8mz/moodle/pull/5

        Show
        Rossiani Wijaya added a comment - Updated and submit github pull request for David: https://github.com/mudrd8mz/moodle/pull/5
        Hide
        David Mudrak added a comment -

        Why isn't the field answertext renamed to answer_text via a recipe in get_paths() ? Then you can use write_xml() wrapper for the whole data easily without using the manual foreach loop.

        Show
        David Mudrak added a comment - Why isn't the field answertext renamed to answer_text via a recipe in get_paths() ? Then you can use write_xml() wrapper for the whole data easily without using the manual foreach loop.
        Hide
        Rossiani Wijaya added a comment -

        Hi David,
        I fixed the patch as you suggested.

        Resubmitted patch for github pull request:
        https://github.com/mudrd8mz/moodle/pull/6

        Show
        Rossiani Wijaya added a comment - Hi David, I fixed the patch as you suggested. Resubmitted patch for github pull request: https://github.com/mudrd8mz/moodle/pull/6
        Hide
        David Mudrak added a comment -

        Thanks Rossie, lookign good now.

        Show
        David Mudrak added a comment - Thanks Rossie, lookign good now.
        Hide
        David Mudrak added a comment -

        Merged into the pre-integration branch. Thanks Rossie

        Show
        David Mudrak added a comment - Merged into the pre-integration branch. Thanks Rossie
        Hide
        David Mudrak added a comment -

        Rosie, can you please review all steps in mod/lesson/db/upgrade.php and make sure they all are reflected in moodle1 converter? For example the upgrade step 2010121400 does not seem to replayed. Thanks.

        Show
        David Mudrak added a comment - Rosie, can you please review all steps in mod/lesson/db/upgrade.php and make sure they all are reflected in moodle1 converter? For example the upgrade step 2010121400 does not seem to replayed. Thanks.
        Hide
        Aparup Banerjee added a comment - - edited

        Thanks David, i'll put that bit in soon, looking at it now. (Rosie, i'm just grabbing this bit while your away)

        Show
        Aparup Banerjee added a comment - - edited Thanks David, i'll put that bit in soon, looking at it now. (Rosie, i'm just grabbing this bit while your away)
        Hide
        Aparup Banerjee added a comment -

        fyi: i've got a branch up at https://github.com/nebgor/moodle/compare/backup-convert...MDL-27446 , still checking the rest of upgrade steps.

        Show
        Aparup Banerjee added a comment - fyi: i've got a branch up at https://github.com/nebgor/moodle/compare/backup-convert...MDL-27446 , still checking the rest of upgrade steps.
        Hide
        Aparup Banerjee added a comment -
        Show
        Aparup Banerjee added a comment - David, i've created https://github.com/mudrd8mz/moodle/pull/12
        Hide
        David Mudrak added a comment -

        Apu's recent changes merged into the pre-integration branch. Thanks!

        Show
        David Mudrak added a comment - Apu's recent changes merged into the pre-integration branch. Thanks!
        Hide
        Aparup Banerjee added a comment - - edited

        David, i can't see that the <jumpto/> issue fix pulled into your backup-convert branch yet. I've merged it into my current working branch for MDL-27819.

        Show
        Aparup Banerjee added a comment - - edited David, i can't see that the <jumpto/> issue fix pulled into your backup-convert branch yet. I've merged it into my current working branch for MDL-27819 .
        Hide
        David Mudrak added a comment -

        Oops sorry my bad. I just cherry-picked the submitted patch, please re-fetch.

        Show
        David Mudrak added a comment - Oops sorry my bad. I just cherry-picked the submitted patch, please re-fetch.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: