Download & Extend

Better inline code documentation for function money_get_widget_currencies()

Project:Money CCK field
Version:6.x-1.x-dev
Component:Code
Category:task
Priority:normal
Assigned:Unassigned
Status:closed (fixed)

Issue Summary

Hi,

i installed money like indicated in readme and after installing the required modules.

but i have no currency displaed in the select options, and i get a message currency needed when an amount in typed in.

Help please.

Thanks

Comments

#1

Title:no currency displayed in select» No currency options if none enabled in field/widget settings
Version:6.x-1.x-dev» 6.x-1.0
Category:support request» bug report
Status:active» needs review

One way to fix this is to check all the checkboxes in the CCK field's settings page under the available currencies checkbox. (Money module supports Checkall module to facilitate this.)

However the fact that no currencies are available in the <select>'s <option> list is a bug. Since the #description of the aforementioned checkbox group states Choose the currencies that you want to enable for this field. Do not select any currency to enable them all..

The attached patch fixes this bug, makes the code that resulted in the human error causing this bug easier to read.

AttachmentSize
currencies.patch 2.36 KB

#2

Version:6.x-1.0» 6.x-1.x-dev

Doh! I should have updated to dev before writing patches. This patch is rerolled for DRUPAL-6--1. Even though the bug is fixed there, the code flow and readability issues are not, so this patch is still useful.

AttachmentSize
372128.patch 2.33 KB

#3

Hi,

Thank you for your help.

i missed the available currencies in doc (or there was not).

Patch is a good and elegant way to fix this issue.

Thank you

#4

I'm sorry for not getting back earlier. Isn't this issue similar to this? #356944: Currency not allowed when setting the default value of the field

That patch seemed to fix the other issue. I'm going to put it in CVS right now, so it's available in the nightly snapshot. Does it fix this as well?

#5

Status:needs review» closed (fixed)

Well, I tested this at the office and seems to be fixed by the other patch. Please, feel free to re-open if it doesn't work for you.

#6

Status:closed (fixed)» needs review

#5; Yes. See my comment #2

... Even though the bug is fixed there, the code flow and readability issues are not, so this patch is still useful.

#7

Title:No currency options if none enabled in field/widget settings» Better inline code documentation for function money_get_widget_currencies()
Category:bug report» task
Status:needs review» needs work

hmm... agree the inline code documentation could be a bit more explicit, but the code itself is fine for me. It uses less operations.

I'm changing the issue title to reflect the correct issue. I'll commit better inline comments later.

#8

Status:needs work» closed (fixed)

I have made some changes to the module, including a detailed explanation of the steps taken in the money_get_widget_currencies().

#9

> hmm... agree the inline code documentation could be a bit more explicit, but the code itself is fine for me. It uses less operations.

Understandable. I prefer readability over efficiency, but understand that is subjective.

#10

Feel free to provide a patch to enhance the documentation. It will be great.

But if you wish to change code that works, then you need something more than just readability. We're talking about a single function that just have a few IFs that are visible on one single screen. Come on.

#11

Note I had to resubmit the field settings form to get this to work after updating to DRUPAL-6--1. I then noticed that there were database updates, which may also have fixed this without resubmitting the field's settings form.