Feature to provide mostly used countries in top section of Country select list

[sunilpawar]

Overview

Functionality to provide mostly used (Favourite) countries in top section after default country. So user don't have type to search country name again.

Before

After

Billing form and contact address both will have mostly used countries in top section.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

If it's spelled as "Favourite" here in the title then there's no way for the traunslatioun systeum to output a US spelling.

Or another option is use a word like "Pinned". Or it's a bit long but "Countries Pinned to Top of Selection Lists" is accurate.

Also the add would currently be 5.33.

[sunilpawar]

@demeritcowboy , i changed label to Pinned Countries and corrected version too.

[eileenmcnaughton]

@sunilpawar style issue

[demeritcowboy]

I have a couple other r-code comments too, just figured I'd post all at once.

[demeritcowboy]

I like the idea, but it should have a unit test and I don't think it would be hard to write. Also some r-code notes below.

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
    • While the current docs are really light, and the wiki adds a little more info, the setting seems straightforward.
  • [r-run] PASS
    • I didn't test with multiple domains or fancy setups. But did also test with non-english (single-language).
• Developer standards
  • [r-tech] PASS
  • [r-code] Minor Issues
    • The help_text in settings.php needs ts(). (I know it's not currently used on that screen since some screens don't use the newer settings system fully.)
    • Using static vars as a way of caching within a function is mostly deprecated within civi. I know it's still used elsewhere in this file but for reference the better way is to use Civi::$statics - see e.g. dev docs and blog post. The Civi::settings() system is cached already, so there may not be much additional savings here since the number of pinned countries is likely to be small, but if you are going to do it, use Civi::$statics.
    • I had an urge to greatly simplify this loop down to two lines, except I don't see to how to keep the preferred order that the person has configured without doing the loop as you've done. So in case someone else comes along and also has that urge, it might be useful to add a comment saying something like "we need to loop like this to keep the preferred order of pinned countries".
    • Lines 142 and 143 below that could be combined, but also if you look at what defaultContactCountry() does, it converts id to ISO code. Then this code here is converting it back from ISO code to an id. It would seem simpler to just call Civi::settings()->get('defaultContactCountry'); and then you've got your id. Like so:

      $pinnedContactCountries = CRM_Core_BAO_Country::pinnedContactCountries($availableCountries);
      // if default country is set, percolate it to the top
      if ($defaultContactCountry = Civi::settings()->get('defaultContactCountry')) {
        $default = [$defaultContactCountry => $availableCountries[$defaultContactCountry] ?? NULL];
        $availableCountries = $default + $pinnedContactCountries + $availableCountries;
      }
      elseif
      

    • Some people are against the use of _ to start function names.
  • [r-maint] Issue
    • It shouldn't be too hard to write a test to test the logic. Even a simple one like just setting the setting, then calling the pseudoconstant, then checking the order.
  • [r-test] Pending

[sunilpawar]

@demeritcowboy Added Test case and other changes you suggested

[demeritcowboy]

Thanks! Looks good to me.

[eileenmcnaughton]

@demeritcowboy you are happy for this to be merged now?

[demeritcowboy]

Yes.

[eileenmcnaughton]

Done - thanks @sunilpawar @demeritcowboy