Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41744

Fix custom profile menu field support on csv user upload form

    Details

    • Testing Instructions:
      Hide

      Create Custom Profile Menu without a default value from the list of options.
      Upload a User CSV and pick any option from the Custom Profile Menu.
      Upload the user
      The user will not have any value for the menu.
      Apply the patch and repeat will work correctly.

      Show
      Create Custom Profile Menu without a default value from the list of options. Upload a User CSV and pick any option from the Custom Profile Menu. Upload the user The user will not have any value for the menu. Apply the patch and repeat will work correctly.
    • Workaround:
      Hide

      Include default values into the CSV as additional columns.

      Show
      Include default values into the CSV as additional columns.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      wip-mdl-41744

      Description

      MDL-31654: Broke the ability to upload a csv without custom profile menu fields and select the default values within the user upload form.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for spotting that and providing the patch Tim.

            I have put this on backlog and will try review this today.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for spotting that and providing the patch Tim. I have put this on backlog and will try review this today.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for fixing this Tim,

            Should this be backported to 24 as well?

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Tim, Should this be backported to 24 as well?
            Hide
            tlock Tim Lock added a comment -

            Added moodle24 github link.

            Show
            tlock Tim Lock added a comment - Added moodle24 github link.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for updating the branch Tim.

            Looking at the patch again, it seems to fail for menu with numeric value (1,2,3,4,5...)
            IMO solution should be to convert key to value if it comes from form and then let the later code do it's job.
            Here is a quick patch to resolve this.
            https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-41744

            Let me know your thoughts.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for updating the branch Tim. Looking at the patch again, it seems to fail for menu with numeric value (1,2,3,4,5...) IMO solution should be to convert key to value if it comes from form and then let the later code do it's job. Here is a quick patch to resolve this. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-41744 Let me know your thoughts.
            Hide
            tlock Tim Lock added a comment -

            Hi Rajesh,

            I'm happy with just your patch, so let me know if you have any more problems resolving this.

            Show
            tlock Tim Lock added a comment - Hi Rajesh, I'm happy with just your patch, so let me know if you have any more problems resolving this.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Tim,

            As you have peer-reviewed this, pushing this for integration review.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Tim, As you have peer-reviewed this, pushing this for integration review.
            Hide
            marina Marina Glancy added a comment -

            Hi Tim, Raj,
            thanks for working on it.
            This bit of code looks very strange:

            if (method_exists($formfield, 'convert_external_data')) {
                $user->$field = $formfield->edit_save_data_preprocess($user->$field, null);
            }

            Are you sure it is doing what it was supposed to?

            Also, for improving readability, can you consider using 'profile_field_xxx' as index in array $proffields (instead of just 'xxx')?
            And also rename the variable $newfield into $datatypeclass. A bit confusing to try to understand this code.
            Thanks again.

            Show
            marina Marina Glancy added a comment - Hi Tim, Raj, thanks for working on it. This bit of code looks very strange: if (method_exists($formfield, 'convert_external_data')) { $user->$field = $formfield->edit_save_data_preprocess($user->$field, null); } Are you sure it is doing what it was supposed to? Also, for improving readability, can you consider using 'profile_field_xxx' as index in array $proffields (instead of just 'xxx')? And also rename the variable $newfield into $datatypeclass. A bit confusing to try to understand this code. Thanks again.
            Hide
            marina Marina Glancy added a comment -

            Sorry have to reopen it because received no reply. I know that Raj is away atm, we'll just wait for him to come back

            Show
            marina Marina Glancy added a comment - Sorry have to reopen it because received no reply. I know that Raj is away atm, we'll just wait for him to come back
            Hide
            cibot CiBoT added a comment -

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

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

            Thanks for looking at this Marina,

            I agree the code is messy and it will be nice to re-write it nicely.
            Code snippet you mentioned do work as expected. This is currently used only for menu fields.

            Have updated key and variable.. pushing it back for review.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for looking at this Marina, I agree the code is messy and it will be nice to re-write it nicely. Code snippet you mentioned do work as expected. This is currently used only for menu fields. Have updated key and variable.. pushing it back for review.
            Hide
            poltawski 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
            poltawski 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
            samhemelryk Sam Hemelryk added a comment -

            Just noting that Raj and myself are both going to look deeply into this issue and the purposed solution.

            Show
            samhemelryk Sam Hemelryk added a comment - Just noting that Raj and myself are both going to look deeply into this issue and the purposed solution.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Raj, after studying the upload tool I think given its construction there is no better solution available without really hacking on it.

            As such I've integrated this now thanks!

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Raj, after studying the upload tool I think given its construction there is no better solution available without really hacking on it. As such I've integrated this now thanks!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Testing and passed during integration review.

            Show
            samhemelryk Sam Hemelryk added a comment - Testing and passed during integration review.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Sam

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Sam
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels.

            Or, if you prefer, yes, you fixed that boring issue.

            Thanks anyway! Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels. Or, if you prefer, yes, you fixed that boring issue. Thanks anyway! Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13