Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Administration
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      15564

      Description

      Similar to $CFG->country - i.e. pre-fill in the add/edit user and sign-up forms with a default city based on a new $CFG->city setting.

      Useful for small school Moodles where the students and teachers all come from the same city.

        Issue Links

          Activity

          Show
          Jonathan Harker added a comment - Git branch here: http://git.catalyst.net.nz/gw?p=moodle-r2.git;a=shortlog;h=refs/heads/MDL-25778
          Hide
          Sam Hemelryk added a comment -

          Hi Jonathan,

          This gets my vote!

          The only thing I don't really like about it is the setting name city, I think it would be clearer to call it defaultcity, or perhaps defaultusercity.
          But then there is already a default country settings called country so I suppose just calling it city is like sticking to the standard

          Cheer
          Sam

          Show
          Sam Hemelryk added a comment - Hi Jonathan, This gets my vote! The only thing I don't really like about it is the setting name city , I think it would be clearer to call it defaultcity, or perhaps defaultusercity. But then there is already a default country settings called country so I suppose just calling it city is like sticking to the standard Cheer Sam
          Hide
          Jonathan Harker added a comment -

          I agree - I was tempted to call it defaultcity - no harm in doing so I guess.

          Show
          Jonathan Harker added a comment - I agree - I was tempted to call it defaultcity - no harm in doing so I guess.
          Hide
          David Mudrak added a comment -

          Would it make sense to come with a robust solution to provide default values for other profile settings, too? If so, which fields?

          Show
          David Mudrak added a comment - Would it make sense to come with a robust solution to provide default values for other profile settings, too? If so, which fields?
          Hide
          Jonathan Harker added a comment -

          Perhaps, but this was mainly to cover the two "outlier" fields that MUST be filled in (country and city) other than the usual name, email, and password.

          Show
          Jonathan Harker added a comment - Perhaps, but this was mainly to cover the two "outlier" fields that MUST be filled in (country and city) other than the usual name, email, and password.
          Hide
          Jonathan Harker added a comment - - edited

          Otherwise I suppose we would have a set of key value pairs, where $key is the form element name. If ! empty($value) we could then $form->setDefault($key, $value) in the form definition. Then we'd need to have some way for the user to control the values, probably a "Default User Settings" admin page, with elements for each field; I doubt we could auto-generate the admin page, so we'd have to set the elements up manually. Either way we can then easily cobble a simple dictionary class/assoc array from the default user settings to pass around for use in the form definitions. This would be useful elsewhere too, such as the authentication and enrolment plug-ins that auto-generate users as needed. Wish I had time!

          Show
          Jonathan Harker added a comment - - edited Otherwise I suppose we would have a set of key value pairs, where $key is the form element name. If ! empty($value) we could then $form->setDefault($key, $value) in the form definition. Then we'd need to have some way for the user to control the values, probably a "Default User Settings" admin page, with elements for each field; I doubt we could auto-generate the admin page, so we'd have to set the elements up manually. Either way we can then easily cobble a simple dictionary class/assoc array from the default user settings to pass around for use in the form definitions. This would be useful elsewhere too, such as the authentication and enrolment plug-ins that auto-generate users as needed. Wish I had time!
          Hide
          Jonathan Harker added a comment -

          Meanwhile, how do we get this into master?

          Show
          Jonathan Harker added a comment - Meanwhile, how do we get this into master?
          Show
          Jonathan Harker added a comment - Updated branch: http://git.catalyst.net.nz/gw?p=moodle-r2.git;a=shortlog;h=refs/heads/MDL-25778_20110209
          Hide
          Petr Škoda added a comment -

          I have submitted a bit improved PULL request, thanks a lot!

          Show
          Petr Škoda added a comment - I have submitted a bit improved PULL request, thanks a lot!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          From PULL-313:

          Side comment: Just guessing why there aren't any "$CFG->country" occurrences in:

          • admin/uploaduser.php
          • auth/db/auth.php
          • auth/ldap/auth.php

          Does that mean that those methods are not observing the default country? Should we add that?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - From PULL-313: Side comment: Just guessing why there aren't any "$CFG->country" occurrences in: admin/uploaduser.php auth/db/auth.php auth/ldap/auth.php Does that mean that those methods are not observing the default country? Should we add that? Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've created MDL-26409 about other "defaults" (country...) potentially not being applied in all the user creation options.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I've created MDL-26409 about other "defaults" (country...) potentially not being applied in all the user creation options. Ciao
          Hide
          Jonathan Harker added a comment -

          I don't mean to cause too much of a stir but
          1. I can't comment on either PULL request,
          2. having $CFG->defaultcity instead of $CFG->city is inconsistent with the existing $CFG->country setting. If we're going to have $CFG->defaultcity then we must change $CFG->country to $CFG->defaultcountry as well, which is why I went with $CFG->city. Such a change would be reasonably disruptive and should be done on a separate issue number, and
          3. Petr's proposed commit on PULL-313 does not retain my authorship.

          Show
          Jonathan Harker added a comment - I don't mean to cause too much of a stir but 1. I can't comment on either PULL request, 2. having $CFG->defaultcity instead of $CFG->city is inconsistent with the existing $CFG->country setting. If we're going to have $CFG->defaultcity then we must change $CFG->country to $CFG->defaultcountry as well, which is why I went with $CFG->city. Such a change would be reasonably disruptive and should be done on a separate issue number, and 3. Petr's proposed commit on PULL-313 does not retain my authorship.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Jonathan,

          about commenting on PULL request... I think the idea was to keep discussion active here (in the MDL) and use the PULL exclusively for comments related to the integration & testing phase. Mainly because if we mix comments in both places, surely some of them will be forgotten.

          about the name (defaultcity), I remember it was discussed when Petr was performing modifications to your original code and we agreed "default" was better for the future, with others like country and friends be renamed. But we didn't want to perform that change for existing ones in the middle of stable. Yes, it causes somehow a inconsistency, but aimed to be cleaned in the future with all them being "defaultxxxx"

          about authorship, I cannot tell much, I don't know which git magic was used nor the differences between your original changes and the final ones, "git blame Petr".

          Thanks for your support! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Jonathan, about commenting on PULL request... I think the idea was to keep discussion active here (in the MDL) and use the PULL exclusively for comments related to the integration & testing phase. Mainly because if we mix comments in both places, surely some of them will be forgotten. about the name (defaultcity), I remember it was discussed when Petr was performing modifications to your original code and we agreed "default" was better for the future, with others like country and friends be renamed. But we didn't want to perform that change for existing ones in the middle of stable. Yes, it causes somehow a inconsistency, but aimed to be cleaned in the future with all them being "defaultxxxx" about authorship, I cannot tell much, I don't know which git magic was used nor the differences between your original changes and the final ones, "git blame Petr". Thanks for your support! Ciao
          Hide
          Petr Škoda added a comment -

          Opps, sorry I did not give you the 'git author' credit for the string 'A city entered here will be the default city when creating new user accounts.', I thought the info in commit message would be enough. I will use my own strings next time, sorry. I have written the rest of the code myself - I just copy-pasted country code and added fixes for upload user and originally for auth plugins too, that is why I used myself as author and only gave you credit for the idea in the commit message.

          Normally I would not file a PULL request myself, but it was the only chance to get this issue resolved in 2.0.2. I have discussed the defaultcity name with David and Eloy, we all agree that country was a bad string and setting name.

          In any case I have added you to the list of ppl that are allowed to create PULL requests, please tell other developers to ping me if necessary. Next time this happens your PULL requests will be simply rejected and you will have time to rework or fix them so that you get all the due credit.

          I am going to bring up the topic of authorship and handling of git contributions, it would be great if there were some solid guidelines for people doing the code reviews and integrations, it used to be a lot easier in the old CVS days, we are still learning the new and better git way....

          Thanks a lot for your contribution and notifying us about this problem, I am looking forward to more PULL requests from you soon!

          Petr

          Show
          Petr Škoda added a comment - Opps, sorry I did not give you the 'git author' credit for the string 'A city entered here will be the default city when creating new user accounts.', I thought the info in commit message would be enough. I will use my own strings next time, sorry. I have written the rest of the code myself - I just copy-pasted country code and added fixes for upload user and originally for auth plugins too, that is why I used myself as author and only gave you credit for the idea in the commit message. Normally I would not file a PULL request myself, but it was the only chance to get this issue resolved in 2.0.2. I have discussed the defaultcity name with David and Eloy, we all agree that country was a bad string and setting name. In any case I have added you to the list of ppl that are allowed to create PULL requests, please tell other developers to ping me if necessary. Next time this happens your PULL requests will be simply rejected and you will have time to rework or fix them so that you get all the due credit. I am going to bring up the topic of authorship and handling of git contributions, it would be great if there were some solid guidelines for people doing the code reviews and integrations, it used to be a lot easier in the old CVS days, we are still learning the new and better git way.... Thanks a lot for your contribution and notifying us about this problem, I am looking forward to more PULL requests from you soon! Petr
          Hide
          Jonathan Harker added a comment -

          Sorry Petr I did not see the extra line in the commit message

          Show
          Jonathan Harker added a comment - Sorry Petr I did not see the extra line in the commit message

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: