Author Topic: Tackling the backlog of 'ready to review' extensions (Read 3408 times)

capo · October 09, 2013, 03:00:00 am

I've been looking with John his UK Phone Number Validator extension.

Pre-installation review

The description is adequate and clear. It says what the extension does.
They are already some screenshots at the extension directory. Cool!
The extension is in GitHub. Happy face smiley!
The xml of the extension includes a TODO list -in the comments- with the future improvements. Well played!

Functional review

Installation worked nice over a 4.3.7 instance.
As the description says, you find a new "UK Phone Number Validator" option under the Contacts menu.
When you open it, you see the first 50 phone numbers to be validated for three categories: broken phone numbers, landlines as mobiles and mobiles as landlines.
You can correct them inline.

Code review

Code is clear and direct, and it appears to respect CiviCRM standards (2 spaces for indentation, etc).
No obvious vulnerabilities were found.
Strings are not being ts'ed.

The extension seems a good starting point for a generic and international validator (maybe phones, account numbers, credit cards, addresses, etc).

No screenshots are attached, as long as the description already have a few.

capo · October 09, 2013, 03:25:15 am

mathieu, I'll take a look at your region lookup extension. And I'll try to see if it works with Ixiam's Options Value Importer.

capo · October 09, 2013, 05:49:28 am

After bothering Mathieu with some issues that were related with bad settings in my installation (sorry Mathieu!), I've been testing a little bit the Region Lookup extension.

Pre-installation review

The description looks clear and explains exactly what the extension does.
The extension source code is in GitHub. Cool!
A screenshot is already available at the extensions directory.

Functional review

After installation, you'll have a new option Regional Lookup Settings under Administer > System Settings. It opens the settings page for the extension (as in the attached file: regionlookup-settings.png).
It requires some technical skills to make it work, but it works nice.
You'll have to do two things to make it work: specify the ID's of the fields you want to link (the one that triggers the changes and the one that you'll change) and populate the table civicrm_regionallookup as needed.
A text under the settings page, says that you can report bugs in GitHub, with a link to the project. Nice!

Code review

Code is clear and direct, and it appears to respect CiviCRM standards (2 spaces for indentation, etc).
No obvious vulnerabilities were found.
All the strings are being properly ts'ed.

As all the other extensions I've been testing, it's a really useful extension. If we manage to do it even more generic and user oriented, it will became a basic extension.

NASACT · October 09, 2013, 08:54:13 am

Yes, I meant quick contact autocomplete, sorry was in a rush to get out the door yesterday. Comments below based on Capo's format for this.

Here are my comments about Options Value Importer.

Pre-installation review

The description is adequate and clear. It says what the extension does.
GitHub extension distro, as Capo says, "Nice"
Had to download zip and install by FTP'ing the folder (has a very odd naming convention of the zip/folder that might be useful to change to something else)

Functional review

After installation, it replaces the default quick search behavior. Did find it to significantly faster on search results for 5K worth of individuals in my testing DB.
Tested on CiviCRM 4.2.7
Tested as described
(Possible improvement) It does do the @gmai and .com but I think it should be expanded to search @XXX and .XXX for any email addy instead of just those for usefulness.

Code review
Code is clear and direct, and it appears to respect CiviCRM standards (2 spaces for indentation, etc).
Comments are short and concise but adequate in length.
No obvious vulnerabilities were found.
Could not locate snippet of code that defined why @gmai works but not @nasa doesn't. (Xavier probably knows and may have hardcoded this but doubt it, he's too skilled for that)

Here are some screen shots that can be used for documentation on the extension page if you want.

Install screen on the extensions page in CiviCRM after manual placement of directory in my extensions folder. Screenshot_1.png

Confirmation of Install and extension overview info. Screenshot_2.png

Post Install confirmation and confirmation that enable is the current setting along with disable option. Screenshot_3.png

Typing of just 2 characters. Screenshot_4.png

More to come in second post.

NASACT · October 09, 2013, 08:56:49 am

3 or more characters, results in correct searching. Screenshot_5.png

Where @nasact did not produce search result of me which I felt it should have done but may not be in the scope of things. Screenshot_6.png

xavier · October 09, 2013, 10:12:19 am

HI,

thx for the review. Could you post your suggested improvement on github so we can keep track of it?

Definitely not hardcoded it works on gmail but not on nasa, but will have to look at the code, don't remember if I test if there is a @ to do a mail first search.

X+

Dave Greenberg · October 09, 2013, 10:54:01 am

I've notified the author of 'Duration' extension that there is a review here which they should check and take action on.

mathieu · October 09, 2013, 02:33:59 pm

Thanks @capo for the review! I have marked the "regionlookup" extension as "ready for automatic distribution".

PS: new extension up: payment contribution popup confirmation (a compromise between no confirmation page at all, and the big confirmation page): http://forum.civicrm.org/index.php/topic,30232.0.html

NASACT · October 10, 2013, 05:39:37 am

Quote from: xavier on October 09, 2013, 10:12:19 am

HI,

thx for the review. Could you post your suggested improvement on github so we can keep track of it?

Definitely not hardcoded it works on gmail but not on nasa, but will have to look at the code, don't remember if I test if there is a @ to do a mail first search.

X+

I think I did it where you wanted me to but couldn't assign it to "Enhancement" category.

NASACT · October 10, 2013, 05:46:45 am

Just FYI Google Apps sync requires a paid version of Google Apps for Business or Education per the Github info located at https://github.com/cividesk/sync.googleapps/

Might want to note that in the Extension page description.

Not sure if anyone has a paid version of Google Apps to test this extension.

Dave, could you move the needing review ones that are complete down below that so we know which ones are still needing review. Thank you sir!

capo · October 11, 2013, 04:12:28 am

Are the descriptions of extensions translatable? I've just realised that we've been doing an effort to promote the internationalization of extensions, by reviewing the code looking for ts'ed strings, etc. But the titles and descriptions of the extensions are allways in English.

I understand that CiviCRM.org is in English but the list of extensions is something we can see into our CiviCRM web interface (Administer > System Settings > Manage Extensions). Wouldn't it be nice to have them ts'ed, too? Maybe it is already possible? And autosend them to the Transifex CiviCRM Extensions project?

Sorry if it's something you've already talked about.

xavier · October 11, 2013, 05:51:01 am

I think that better l10n of the "user" interface is great. However, translating the description of the extensions for the admins and technical staff that deal with these part can stay in english. After all, github, jira and sourceforge are in english too.

X+

mathieu · October 11, 2013, 06:09:15 am

@Xavier: I disagree, as you might expect

Although I don't think it is the biggest priority, but it would be a "nice to have", as much as it would be nice to have:
- a localised installation working,
- localised installation/upgrade documentation
- localised 1-page civicrm.org overview of what is CiviCRM (or localized separate sites, if a community has the energy to do it).

Technically, I'm not sure what's the best way to do it:

- The descriptions should be on Transifex (in the civicrm_extensions project)
- but we need to display the description before installation, which is mostly reading the XML.. so an option could be to generate localised versions of the extensions listing.

Ex: /extdir/ver={ver}|cms={uf}|locale={locale}

EDIT: this doesn't fix locally installed extensions, however. That would probably have to use the local ".mo" file.

CiviCRM Community Forums (archive)

News:

Author Topic: Tackling the backlog of 'ready to review' extensions (Read 3408 times)

capo

Re: Tackling the backlog of 'ready to review' extensions

capo

Re: Tackling the backlog of 'ready to review' extensions

capo

Re: Tackling the backlog of 'ready to review' extensions

NASACT

Re: Tackling the backlog of 'ready to review' extensions

NASACT

Re: Tackling the backlog of 'ready to review' extensions

xavier

Re: Tackling the backlog of 'ready to review' extensions

Dave Greenberg

Re: Tackling the backlog of 'ready to review' extensions

mathieu

Re: Tackling the backlog of 'ready to review' extensions

NASACT

Re: Tackling the backlog of 'ready to review' extensions

NASACT

Re: Tackling the backlog of 'ready to review' extensions

capo

Re: Tackling the backlog of 'ready to review' extensions

xavier

Re: Tackling the backlog of 'ready to review' extensions

mathieu

Re: Tackling the backlog of 'ready to review' extensions