Moodle
  1. Moodle
  2. MDL-38035

Display custom profile field names in conditional activities with multilang support

    Details

    • Testing Instructions:
      Hide

      0.

      • In system settings/Plugins/Filters/Manage filters, turn on the multilang filter. Set it to apply to both headings and content.
      • In system settings/Advanced features, make sure conditional availability is enabled.
      • This test assumes your language is English, otherwise change the text below.

      1. In system settings/Users/Accounts/User profile fields, create a new Text profile field with the following settings:

      • Short name: multilangfield
      • Name: <span lang="en" class="multilang">English</span><span lang="fr" class="multilang">Francais</span>
        (Other values can be left default.)

      Note that the field name is correctly displayed as 'English' in the user profile field list.

      2. Go to any course website.
      3. Turn editing on.
      4. Add an activity or resource / Label.
      5. In 'Restrict access' section, open dropdown 'User field'.

      EXPECTED: User field list should include the field 'English'.
      BEFORE FIX: User field list includes 'EnglishFrancais'.

      6. Edit settings for any section in the website.
      7. In 'Restrict access' section, open dropdown 'User field'.

      EXPECTED: User field list should include the field 'English'.
      BEFORE FIX: User field list includes 'EnglishFrancais'.

      8. From the course page, add a new Forum activity with default settings except that, in 'Restrict access' section, open dropdown 'User field' and select that the 'English' field must have value 'Frog'. Save changes.

      9. View course page. Check the informational text below the activity that was just added.

      EXPECTED: Text should read: 'Restricted: Not available unless your English contains frog.'
      BEFORE FIX: Text reads: 'Restricted: Not available unless your EnglishFrancais contains frog.'

      10. Log in using a test student account. Check the same message.

      EXPECTED: Text should read: 'Not available unless your English contains frog.'
      BEFORE FIX: Text reads: 'Not available unless your EnglishFrancais contains frog.'

      Show
      0. In system settings/Plugins/Filters/Manage filters, turn on the multilang filter. Set it to apply to both headings and content. In system settings/Advanced features, make sure conditional availability is enabled. This test assumes your language is English, otherwise change the text below. 1. In system settings/Users/Accounts/User profile fields, create a new Text profile field with the following settings: Short name: multilangfield Name: <span lang="en" class="multilang">English</span><span lang="fr" class="multilang">Francais</span> (Other values can be left default.) Note that the field name is correctly displayed as 'English' in the user profile field list. 2. Go to any course website. 3. Turn editing on. 4. Add an activity or resource / Label. 5. In 'Restrict access' section, open dropdown 'User field'. EXPECTED: User field list should include the field 'English'. BEFORE FIX: User field list includes 'EnglishFrancais'. 6. Edit settings for any section in the website. 7. In 'Restrict access' section, open dropdown 'User field'. EXPECTED: User field list should include the field 'English'. BEFORE FIX: User field list includes 'EnglishFrancais'. 8. From the course page, add a new Forum activity with default settings except that, in 'Restrict access' section, open dropdown 'User field' and select that the 'English' field must have value 'Frog'. Save changes. 9. View course page. Check the informational text below the activity that was just added. EXPECTED: Text should read: 'Restricted: Not available unless your English contains frog.' BEFORE FIX: Text reads: 'Restricted: Not available unless your EnglishFrancais contains frog.' 10. Log in using a test student account. Check the same message. EXPECTED: Text should read: 'Not available unless your English contains frog.' BEFORE FIX: Text reads: 'Not available unless your EnglishFrancais contains frog.'
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-38035-m24
    • Pull Master Branch:
      MDL-38035-master
    • Rank:
      47828

      Description

      We have some custom user profile fields with multilang names in Moodle. These multilang strings have worked well until now. Now, within the new feature "Conditional user fields" (http://docs.moodle.org/24/en/Conditional_user_fields), these profile fields are visible to all Moodle teachers.

      Unfortunately, the custom user profile field names aren't processed with the format_string() function, therefore all languages are visible in the dropdown box (see screenshot for details).

      It would be great if you could display custom profile field names in conditional activities with multilang support.

      Thanks in advance,
      Alex

        Activity

        Hide
        Sam Marshall added a comment -

        I agree this ought to work (it isn't my code, but I would probably have forgotten too). Will look at it.

        Show
        Sam Marshall added a comment - I agree this ought to work (it isn't my code, but I would probably have forgotten too). Will look at it.
        Hide
        Sam Marshall added a comment -

        Added a test script.

        Show
        Sam Marshall added a comment - Added a test script.
        Hide
        Sam Marshall added a comment -

        Submitting for peer review. This change is quite small so I hope it will be okay!

        Show
        Sam Marshall added a comment - Submitting for peer review. This change is quite small so I hope it will be okay!
        Hide
        Sam Hemelryk added a comment -

        Hi Sam,

        The code functions perfectly, but I just don't know about the solution.
        What I don't like is that you end up passing lang strings through format_string with this change and while thats unlikely to introduce issues I suppose the possibility is there.

        I also noted that course/editsection_form.php would need to be addressed as well.

        As mentioned I'm not really sure about calling format_string on lang strings. If you think it is the best way to proceed feel free to make the same fix in editsection_form.php and get this up for integration.
        However in thinking about alternatives I wondered about providing an argument to condition_info::get_condition_user_fields asking it to apply formatting where required. As the context is needed the arg would not be obvious although, perhaps something like array $formattingoptions = null if null no formattting is applied, otherwise it will be arg 3 to format_string.
        Again I was just looking for alternatives, I'll leave it up to you to decide what you would prefer.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Hi Sam, The code functions perfectly, but I just don't know about the solution. What I don't like is that you end up passing lang strings through format_string with this change and while thats unlikely to introduce issues I suppose the possibility is there. I also noted that course/editsection_form.php would need to be addressed as well. As mentioned I'm not really sure about calling format_string on lang strings. If you think it is the best way to proceed feel free to make the same fix in editsection_form.php and get this up for integration. However in thinking about alternatives I wondered about providing an argument to condition_info::get_condition_user_fields asking it to apply formatting where required. As the context is needed the arg would not be obvious although, perhaps something like array $formattingoptions = null if null no formattting is applied, otherwise it will be arg 3 to format_string. Again I was just looking for alternatives, I'll leave it up to you to decide what you would prefer. Many thanks Sam
        Hide
        Sam Marshall added a comment -

        Thanks for review Sam. I've changed it as you suggest - I agree it isn't really necessary to call print_string on the lang strings, and also would be nicer if the function 'does it for you'. I've left it as optional param in case anybody wants the raw names. The comment explains this.

        Also fixed the section edit form and added to the test instructions to cover that. (I did the search for the function calls.) I redid the test on my dev server, it still seems to work.

        Since I made all the suggested review changes, I think this is OK to submit for integration now.

        Show
        Sam Marshall added a comment - Thanks for review Sam. I've changed it as you suggest - I agree it isn't really necessary to call print_string on the lang strings, and also would be nicer if the function 'does it for you'. I've left it as optional param in case anybody wants the raw names. The comment explains this. Also fixed the section edit form and added to the test instructions to cover that. (I did the search for the function calls.) I redid the test on my dev server, it still seems to work. Since I made all the suggested review changes, I think this is OK to submit for integration now.
        Hide
        Eloy Lafuente (stronk7) 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
        Eloy Lafuente (stronk7) 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
        Eloy Lafuente (stronk7) added a comment -

        Integrated (24 and master), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (24 and master), thanks!
        Show
        Eloy Lafuente (stronk7) added a comment - Note: I've added: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=87b6981e25689981bc22c648fdb8c4bd4973efac and http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=7765766512a024a8cdc185cb878b857aacfda232 to document the change, just in case. Ciao
        Hide
        David Monllaó added a comment -

        Hi

        All is working according to the testing instructions, but I see that when I add a resource/activity or I edit a section and I select 'English' as 'User field' restriction in the 'Restrict access' section, the course page displays Restricted: Not available unless your EnglishFrancais contains whatever

        Waiting for feedback before passing/failing

        Show
        David Monllaó added a comment - Hi All is working according to the testing instructions, but I see that when I add a resource/activity or I edit a section and I select 'English' as 'User field' restriction in the 'Restrict access' section, the course page displays Restricted: Not available unless your EnglishFrancais contains whatever Waiting for feedback before passing/failing
        Hide
        David Monllaó added a comment -

        Ups, I pressed 'Test passed'... waiting for feedback anyway

        Show
        David Monllaó added a comment - Ups, I pressed 'Test passed'... waiting for feedback anyway
        Hide
        Sam Marshall added a comment -

        Drat. The error was only reported about the dropdown box, I didn't realise it affected the student display also... Looking into it.

        Show
        Sam Marshall added a comment - Drat. The error was only reported about the dropdown box, I didn't realise it affected the student display also... Looking into it.
        Hide
        Alexander Bias added a comment -

        Sorry, Sam, I also didn't realize that the students view is affected when I reported the issue. Thanks for looking into this issue.

        Alex

        Show
        Alexander Bias added a comment - Sorry, Sam, I also didn't realize that the students view is affected when I reported the issue. Thanks for looking into this issue. Alex
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi, I've marked this as failed... just to know how the 2nd part goes and avoid forgetting it (us, integrators).

        Show
        Eloy Lafuente (stronk7) added a comment - Hi, I've marked this as failed... just to know how the 2nd part goes and avoid forgetting it (us, integrators).
        Hide
        Sam Marshall added a comment -

        I have added items 8-10 in the test script (there are 2 code paths when displaying these, hence the two tests) and created new commits on each branch to deal with the display. If applied, these should hopefully fix the problem identified during testing.

        master - 3f82376d6ab25fecbb8b37c9e1fd2e71d3fe4901
        2.4 - 4b82308f473ff39585cf051255d74a14443175bd (cherry-pick)

        Show
        Sam Marshall added a comment - I have added items 8-10 in the test script (there are 2 code paths when displaying these, hence the two tests) and created new commits on each branch to deal with the display. If applied, these should hopefully fix the problem identified during testing. master - 3f82376d6ab25fecbb8b37c9e1fd2e71d3fe4901 2.4 - 4b82308f473ff39585cf051255d74a14443175bd (cherry-pick)
        Hide
        Sam Marshall added a comment -

        argh - fixed commits (including new way of getting context for the condition_info):

        master - acfee0d4baba4c214dabcdcd94ed8eb7a2d62c79
        2.4 - 50701d1e891ac01001ac0be0224851e3a5995390

        Show
        Sam Marshall added a comment - argh - fixed commits (including new way of getting context for the condition_info): master - acfee0d4baba4c214dabcdcd94ed8eb7a2d62c79 2.4 - 50701d1e891ac01001ac0be0224851e3a5995390
        Hide
        Eloy Lafuente (stronk7) added a comment -

        New commits integrated, thanks!

        Sending back to another round of testing...

        Show
        Eloy Lafuente (stronk7) added a comment - New commits integrated, thanks! Sending back to another round of testing...
        Hide
        David Monllaó added a comment -

        It passes, working as expected, thanks

        Show
        David Monllaó added a comment - It passes, working as expected, thanks
        Hide
        Damyon Wiese added a comment -

        This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

        Thanks for your contributions!

        Show
        Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: