I use: D6, Ubercart 2, VAT-ID, Vat-Tax, Prices are stored without Tax. I activated the modules and found this is not correct:

If a customer wants to go to the checkout he must enter a VAT-ID, which is wrong.

Instead: the customer should have the OPTION to enter the VAT-ID. On case he enters a VAT-ID, resides within the EU, but not within the Shop-Owner's country then the Value added Tax must be removed from the bill / invoice. In case the customer resides outside the EU the "Enter VAT-ID" field can disappear generally (and ofcourse the VAT must also be removed from the bell).

Is this wrong behaviour a configuration issue or an matter of code-implementation??

Thanks for any hints or help.

Regards
Ced

PS: Any documentation on how to install this module would be very much appreciated.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fictionindustries’s picture

I have exactly the same problem. VAT-ID can't be a must and I cant configure this thing correctly.

There is no manual to be found anywhere so I don't know if my configuration is wrong or the module is just not working as it supposed to be.

Any help is appreciated.

patkai’s picture

Exactly the same issue here. The problem is not implementation, but I don't know how to get started with it. Somehow I don't see how this EU-VAT feature fits into the ubercart+tax, as the valid VAT number should "remove" the tax, and how do you remove a tax after it was added?

attiks’s picture

I added a patch to #645260: Is there something wrong, which adds a new permission to make the VAT field optional amongst other fixes

wimh’s picture

There is a "Have to fill in VAT" permission, for users with this role a VAT number is mandatory. But if you don't give this 'permission' to anyone, the VAT number is not required (at least, that's the current implementation, in release 6.x-1.0-beta1). Note that the Administrator user automatically gets all permissions, including the "Have to fill in VAT" one, so Administrator always needs to fill it in (but you shouldn't buy anything as Admin anyway!).

@bela.patkai -- about the removal of VAT: the current module defines a condition "Order should have VAT applied", which takes into account the stores country, the billing country, and the fact whether the customer provided a valid VAT number. If you make your VAT Tax rule conditional on this condition, you should have the desired behavior.

Re documentation: you're totally right, this module needs some. However, I'm glad already when I can find some time to make the features work. If someone would be willing to write up an installation guide (which is best done while actually installing this module the first time), I would be very happy to put it up on the site!

wimh’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

antiorario’s picture

Status: Closed (fixed) » Needs work

I know this was marked as closed but this issue is not at all fixed, as the "feature" in question goes against one basic principle: there should not be a permission called "have to fill VAT number." As the wording suggests, it does not predicate a permission, but a requirement—hence it's completely out of place in the permissions list. Consequently, being configured the way it is, as wimh said at #4, it actually restricts user 1's actions.

(About not buying anything as Admin [or user 1, as I call it], how about testing? That's why you should never, ever restrict user 1's actions.)

I think this sort of control (of which I honestly don't see the point) could be handled better in the module's settings. However, if—for simplicity's sake—it really needs to be kept in the permissions list, it should be worded the opposite way, something like "skip filling VAT number." This would have the following results:

  • it would make the VAT field mandatory for everyone by default
  • it would make "skip filling VAT number" an actual permission, not a requirement (hence in tune with Drupal's philosophy, and common sense)
  • it would not restrict user 1's actions.
wimh’s picture

Hi antiorario,
Actually I agree with you, I just didn't have the time - or felt enough need - to make the change myself. But if someone could provide a patch that does this I will certainly apply it.

antiorario’s picture

I haven't had time to think about this, but I can definitely provide a patch as soon as I get back on track. The only thing is that this kind of change will break the way sites using the module currently work, so site admins will have to be warned that they need to change the relevant permissions. (But I'm getting ahead of myself here.)

wimh’s picture

We could make your "skip filling VAT number" permission active by default for all roles (if that's possible, and good practice, I'm not sure about this). An alternative is that there's a module-wide "VAT is required" setting, and on top of that a "skip VAT even when set to required" permission that can be off by default.

But in any case, as you say, changing the permission to "skip VAT" and keeping it off by default would upset a lot of current users so let's not do it that way.

Bartezz’s picture

subscribe

To me it would be logical to create a permission 'EU users have to fill VAT number', which will check if the country is within EU and then the validity of the VAT number....

Have to fill VAT number is to wide of a setting and per role... a role is not a geographic setting while having a VAT number is geographically determined....

I would think one would require certain roles to enter VAT but only if the user with that role is from within the EU.

Does it make sense?

Cheers

dag729’s picture

I agree with you about the permission of "EU users have to fill VAT number" and I disagree with it to be mandatory for everyoone because, for instance, I have an e-commerce site where both wholesale and retail clients could buy: while the former MUST enter a valid VAT number, the latter COULD either have it or not but I cannot cancel nor accept their purchase on that basis.

Also, the same issue should be applied to the company name, because IF the buyer is a wholesaler he must enter it, while the retailer could enter it or not (none of our business).

(I think that in order to do all the needed customizations would require a solid legal/economics background, aside from the understanding of Drupal).

Bartezz’s picture

Thought I'd share my solution.

I'm building a shop for retailers only so all EU customers must enter a VAT number. Because prices are only shown to logged in users one has to create an account before they can add products to their cart. I've extended the account creation form to require billing information and vat number via profile module. Then I created a custom module which validates the submission of user_register and user_profile_form. (This custom module is dependent of uc_vat module)

The validation code is as followed;

		if(!$form_state['values']['profile_vat_vatnumber'] && in_array($form_state['values']['profile_billing_country'], uc_vat_eu_countries())) {
		// if no VAT was entered but billing country is inside EU
		form_set_error('profile_vat_vatnumber', t('VAT number field is required for customers from within the E.U.'));
	} elseif($form_state['values']['profile_vat_vatnumber'] && !in_array($form_state['values']['profile_billing_country'], uc_vat_eu_countries())) {
		// if VAT was entered but billing country is outside EU
		form_set_error('profile_vat_vatnumber', t('VAT number field is only required for customers from within the E.U.'));
		$form_state['values']['profile_vat_vatnumber'] == NULL;
	} elseif($form_state['values']['profile_vat_vatnumber']) {
		// if VAT was entered
		$vat_check = uc_vat_number_validate_vat($form_state['values']['profile_vat_vatnumber']);
		if($vat_check !== FALSE) {
			form_set_error('profile_vat_vatnumber', $vat_check);
		}
	}

Hope this helpes others!

Cheers

antiorario’s picture

Status: Needs work » Needs review
FileSize
3.07 KB

Bartezz and dag729, "EU users have to fill VAT number" is, again, not a permission and could never be implemented as one, plus it would still go against good practice as I described in #7.

I think I understand Bartezz's solution via the profile module, but I think it only adds complexity to a module that should (and already does) work quite well, except for the glitch in the permission system.

I think the discussion is veering toward issues that have little to do with the way Drupal works, and I'd like to steer it back onto its original track.

Since #645260: Is there something wrong didn't fix this (despite various promises), I've decided to give it a try. The attached patch works against HEAD, or the latest dev version.

I added the permission "skip filling VAT number" (note that the first word is not capitalized, as per Drupal standard), and I reversed the behavior of the previous permission in the module. It's very important to run update.php after applying this patch, as I've made sure that the roles that previously had the old, wrong permission now don't, and the remaining roles now have the new permission (including, by default, user 1, which is now not forced to enter a VAT number).

Also, wimh, Drupal updates should be numbered starting with the Drupal's and the module's version numbers (see http://api.drupal.org/api/drupal/developer--hooks--install.php/function/...) so the update function I added is uc_vat_number_update_6103(). It ends with 3 only for consistency with the fact that the previous update was numbered 2.

Please do not test this patch on a production site. Let me know if something is not working as it should. Provide any error messages you receive.

wimh’s picture

Thanks antiorario, your patch looks good at first glance, I'll try to test it later.

It does change the default for new user roles though, from "VAT not required" to "VAT required". I would prefer there to be a configuration (off by default) that makes the VAT number required (and which can then, for selected roles, be disabled again).

antiorario’s picture

I think you have it backwards. Newly created roles do not have permission to skip filling the VAT number, which makes VAT a requirement (except for user 1).

Bartezz’s picture

@Antiorario I've read #4 and #7 with more attention and agree it shouldn't be a permission but a module setting.

My solution for EU customers could be a nice feature for this module via a setting too, possibly a setting in which one can select which roles this setting should apply for. If depending on uc_vat module one can use a function called uc_vat_eu_countries() which returns all EU country IDs. It wouldn't be hard to add validation to forms with a uc_vat_number field to check if a user enters a country from the EU and if so check if the VAT number was entered.

Cheers

wimh’s picture

Newly created roles do not have permission to skip filling the VAT number, which makes VAT a requirement (except for user 1).

Right. But currently, by default for new roles, VAT is *not* required. Your patch would change that.

antiorario’s picture

Bartezz, what you're suggesting can already be achieved using UC conditional actions.

antiorario’s picture

wimh, I understood your previous comment now (it was I who had it backwards). My idea is that you don't create new roles every day. When you do, the first thing you do is assign permissions (otherwise it makes no sense to create a new role to begin with). That's the way Drupal works, for security reasons. If you want the module to work the other way around, we should make it a module setting, and use the permission system to introduce something like "skip filling VAT number even when required", which would be useful for user 1.

wimh’s picture

My concern was just to keep the functionality the same for people who just upgrade and don't expect any changes. For existing roles, this is fine since your patch adds the permission. For new roles, the proper thing to do is indeed walk through the permissions. But the fact that there is now a "skip VAT" permission (which all my existing roles mysteriously acquired) does not make it clear IMO that VAT is now required (especially since it didn't used to be by default). Having a configuration setting, off by default, would make that you would actively need to change this setting to change the behavior of the module and make VAT required.

(BTW user 1 automatically has all permissions, so the mere existence of the "skip VAT" permission makes that user 1 can skip VAT, there would be no need to set it explicitly).

wimh’s picture

On the other hand, on a site that did have the current "require VAT" permission set, doing it with the setting that's default off would now change behavior in that VAT would be optional for all roles, until you enable the new config option. Maybe the upgrade procedure should, for those sites where at least on role has the "require VAT" permission set, make the default *on* ?

antiorario’s picture

I need to rephrase what I said in #19: I guess what Bartezz suggests can indeed be achieved using UC conditional actions, skipping roles altogether, only if uc_vat_number hooks into conditional actions itself—which it currently doesn't, if I'm not mistaken.

About the more general points wimh raises in #21 and #22, I have ideas and will send an update later.

wimh’s picture

Making this into a conditional action would indeed be good. Note that uc_vat_number already exports a number of usable conditions, such as uc_vat_number_condition_billing_country_europe.

antiorario’s picture

Making this into a conditional action would indeed be good. Note that uc_vat_number already exports a number of usable conditions, such as uc_vat_number_condition_billing_country_europe.

Ah, I thought it did but wasn't sure—sorry I didn't double check before speaking. However, I don't think this should be addressed until we fix the permission problem.

Bartezz’s picture

Conditional action is fine by me. I have solved it for now with my custom module which is too specific to share here. But for other users I think the feature added either via setting or CA might be very usefull. As you both know being fellow EU folks is it ok to sell outside of EU without VAT, so no VAT number needed, doesn't validate either via the SOAP, but inside EU (and outside your own country) it's only allowed if you have the buyer's VAT number.

Cheers

antiorario’s picture

My concern was just to keep the functionality the same for people who just upgrade and don't expect any changes. For existing roles, this is fine since your patch adds the permission. For new roles, the proper thing to do is indeed walk through the permissions. But the fact that there is now a "skip VAT" permission (which all my existing roles mysteriously acquired) does not make it clear IMO that VAT is now required (especially since it didn't used to be by default).

It's not a mystery, it's the way the update works, to keep existing functionality intact. But see below.

(BTW user 1 automatically has all permissions, so the mere existence of the "skip VAT" permission makes that user 1 can skip VAT, there would be no need to set it explicitly).

Of course, this has been my problem with the current permission the module implements, which effectively limits user 1, instead of giving it more freedom.

wimh also notes the following:

Having a configuration setting, off by default, would make that you would actively need to change this setting to change the behavior of the module and make VAT required.

On the other hand, on a site that did have the current "require VAT" permission set, doing it with the setting that's default off would now change behavior in that VAT would be optional for all roles, until you enable the new config option. Maybe the upgrade procedure should, for those sites where at least on role has the "require VAT" permission set, make the default *on* ?

To sum it up: what you're saying, correctly, is that if we change the module's behavior so that VAT requirement is the default (like my patch does), it will confuse admins who do not currently have any requirements in place. On the other hand, if we make it so that the VAT number is *not* required by default, it will confuse those who have assigned the infamous requirement "permission".

There is no way to make this module behave better without confusing users. One sure thing is that we can't leave the permission system of this module the way it is.

My suggestion is to implement a module setting for VAT to be required, which can be:

  1. system-wide: all users must enter a VAT number
  2. role-based: only the specified roles will be required to enter a VAT number. This is similar to the current functionality, but is not achieved through the permission system, but via the module's settings page
  3. based on conditional actions: flexible and extensible.

This would allow us to skip the permission system altogether, and although it would require a bit more coding I think it's the way to go. User 1 can be given the ability to skip the requirement by default, or—if we want to have more than one admin on the site with this ability—there is no harm in maintaining the "skip filling VAT number even when required" permission.

I'm not a fan of the role-based solution, but it must be implemented to guarantee legacy support. The update function would take care of migrating the current system to the new one in a way that doesn't break any sites or confuse anyone. (I may be a control freak, but I'm thinking we could have the role-based setting appear only if the site is currently using the VAT-requirement "permission".)

To minimize confusion, I think the module version number should be bumped to 2 and we should make sure that admins receive a warning on the update page to read the release notes for important changes. Same warning on the project page.

wimh’s picture

role-based: only the specified roles will be required to enter a VAT number. This is similar to the current functionality, but is not achieved through the permission system, but via the module's settings page

I like this as an first solution (CA would, in the longer term, of course be preferred). If the upgrade function sets the list of roles based on the current permissions (all roles that now have "have to fill in VAT" go into this list), wouldn't the upgrade be totally transparent then?
(Except that it would fix the user-1 problem, as he wouldn't be in the roles list)

antiorario’s picture

I like this as an first solution (CA would, in the longer term, of course be preferred). If the upgrade function sets the list of roles based on the current permissions (all roles that now have "have to fill in VAT" go into this list), wouldn't the upgrade be totally transparent then?

If by transparent you mean that it wouldn't break existing sites, that's my goal. Of course, like I said, we would still have to warn admins of this change (and I'd still bump the module version to 2.x). If you want, I can work on a patch.

wimh’s picture

I've seen larger changes that didn't bump to 2.x, and this module is still in beta anyway. But if you think it's needed we can do it. A patch would be greatly appreciated!

antiorario’s picture

Assigned: Unassigned » antiorario
FileSize
7.01 KB

I have a new patch, in which I have

  • added a settings page at admin/store/settings/vat-number (I thought this would be appropriate, but can be changed), which is used to select the roles that will be required to enter a VAT number (selection stored in system variable)
  • added the 'administer VAT number settings' permission
  • moved the hook_menu() implementation toward the top of the .module file (better practice)
  • added the helper function _uc_vat_number_required() that checks if the current user is required to enter the VAT number
  • changed the necessary settings for the requirement to be effective
  • added the _6200 update to the .install file, which removes the old permission from any roles that might have it and populates the appropriate system value accordingly. If this task is performed, the user also gets a warning at the end of the update procedure that advises to go have a look at the project page for further info (to do: write a few lines of explanation on the project page).

The update number reveals that the module is at version 2. I do think this is appropriate, because this patch changes the way the module works quite significantly—quite a lot for such a small module. Also, the new version number will tell users to pay special attention.

Please, everyone, triple check that the requirement settings actually work for the following cases:

  • user 1 (should never be required to enter VAT)
  • anonymous (may or may not be required to enter VAT)
  • other roles. Also check that if 'authenticated user' is selected in the settings page the specific settings for other roles should make no difference, since checking 'authenticated user' is like forcing a system-wide requirement.
wimh’s picture

Thanks antiorario, this looks like very good work. I'll try to test out your patch as soon as possible.

Bartezz’s picture

Wow, well done! Will test next week.

Cheers

antiorario’s picture

Any news?

Bartezz’s picture

Deadline is killing me... Haven't had time... yet sorry!

wimh’s picture

Version: 6.x-1.x-dev » 6.x-2.x-dev
Component: Documentation » Code

I started version 6.x-2.x and committed your patch there. I had to change one thing in the upgrade function (return an empty list in the count($on_roles) == 0 case, else upgrade.php complains and doesn't show the warning message. I also updated the URL in there to point to the new version's release page.

antiorario’s picture

Great, I'll test it later today.

antiorario’s picture

wimh, you commited it to HEAD but you also branched a 2.x version (something that should be done only when you start working on 3.x), which means that HEAD and DRUPAL-6--2 must be committed separately for the changes to show up in 2.x. As you can see from http://drupalcode.org/viewvc/drupal/contributions/modules/uc_vat_number/... the 2.x version branches off a very old version, consequently the 6.x-2.x-dev contains old code.

(By the way, I really hope the Git transition will make all this branching and versioning a lot more intuitive than it is now.)

wimh’s picture

Yeah I'm afraid I never got that CVS branching stuff, AFAIC it can't be dead soon enough.

Can you suggest a good way to fix this before I go in and mess it up even more?

Thanks,
Wim

antiorario’s picture

See if you can connect the 2.x-dev to HEAD instead of DRUPAL-6--2 by editing the release node. The best thing is to keep committing changes to both HEAD and DRUPAL-6--2. I know it's extra work, but hopefully it'll all be easier with Git.

Since we're less than two weeks until Git kicks in, I guess we can keep using the dev version and maybe ask the webmasters for help after that (it's pointless to ask to fix CVS stuff at this point—I did for one of my projects and got no answer). In the meantime, I was planning to get some studying done for the Git migration, so I will share if I find anything helpful.

mandreato’s picture

Subscribe

wimh’s picture

I just applied all 6.x-1.x changes to the existing DRUPAL-6--2 branch, and added your patch on top. We should be good now.

antiorario’s picture

Great. I'll check it out tomorrow morning (I'm trying to limit the time I spend on Drupal coding, otherwise that's all I will do).

MustangGB’s picture

There is a typo in "uc_vat_number.module" at line 235 that brings the checkout page to a crashing halt

With typo (broken)

'#required' => __uc_vat_number_required() ? TRUE : FALSE,

Without typo (fixed)

'#required' => _uc_vat_number_required() ? TRUE : FALSE,
wimh’s picture

Thanks, fix committed

mandreato’s picture

There is also the following error when logged in with a user which no role is assigned to:

user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 1 query: SELECT COUNT(rid) FROM users_roles WHERE uid = 3 AND rid IN (); in ...\sites\all\modules\uc_vat_number\uc_vat_number.module on line 628.

wimh’s picture

I committed another patch.

@antiorario What exactly is the format of the uc_vat_number_roles setting? I can't see where in your patch it's being set.

antiorario’s picture

The uc_vat_number_roles variable is an array, and is set by uc_vat_number_settings(), lines 51–68 of uc_vat_number.module.

wimh’s picture

Right but it seems to be in some specific order as you're handling [0], [1] and [2] specially in _uc_vat_number_required().

antiorario’s picture

FileSize
720 bytes

Lines 51–68:

<?php
function uc_vat_number_settings() {
  $form['vat_roles'] = array(
    '#type' => 'fieldset',
    '#title' => t('Require VAT number by role'),
    '#collapsed' => FALSE,
    '#collapsible' => FALSE,
  );

   $form['vat_roles']['uc_vat_number_roles'] = array(
    '#type' => 'checkboxes',
    '#title' => t('Roles'),
    '#default_value' => variable_get('uc_vat_number_roles', array()),
    '#description' => t('Select roles required to enter a VAT number.'),
    '#options' => user_roles(),
  );

  return system_settings_form($form);
}
?>

As you see, uc_vat_number_roles retrieves the options from user_roles(), a Drupal function returning an associative array of role IDs and role names.

In _uc_vat_number_required(), on line 614 I perform an array_flip() on the uc_vat_number_roles variable, in order to remove from the array the roles without requirement, i.e. those that were not checked in the settings page. In case this is not clear: system settings checkboxes are stored as array that have a value of 0 when not set and a value that equals the checkbox's key when they're set. To remove the items that are not set (i.e. not required), I flip the array (thus inverting keys with their values) and unset the item with a key of 0. See lines 613–615:

<?php
  // Retrieve the variable and remove the roles with no requirement
  $roles = array_flip(variable_get('uc_vat_number_roles', array()));
  unset($roles[0]);
?>

So, $roles[0] is only instrumental to removing certain items. On the other hand, $roles[1] and $roles[2] mean, respectively, that anonymous users and authenticated users have VAT requirements (anonymous and authenticated are the only roles whose rid we can always be certain of: 1 and 2).

This part of the function checks exactly that. Lines 622–634, with improved coding standards and extra comments for clarity, and also a "small" correction that makes the check actually work (sorry about that, I don't know what I was thinking):

<?php
  if (user_is_logged_in()) {
    // This checks if all authenticated users are required to enter a VAT number
    if ($roles[2]) {
      return TRUE;
    }
    elseif (!$roles) {
      return FALSE;
    }
    else {
      // If not all authenticated users are required, this checks if the user has one of the required roles
      // I'm aware this is not an approved way to design a query in Drupal, but since there is no user input it's perfectly safe
      $roles = implode(", ", $roles);
      $q = db_query("SELECT COUNT(rid) FROM {users_roles} WHERE uid = %d AND rid IN (%s);", $user->uid, $roles);
      if (db_result($q) <> 0) {
        return TRUE;
      }
    }
  }
?>

The change is specifically on line 630 (patch attached):

<?php
// Wrong:
if (db_result($q) == 0) {

// Right:
if (db_result($q) <> 0) {
?>

I hope this helps.

wimh’s picture

Great, thanks.

antiorario’s picture

wimh, make sure you check out #1055960: Coding standards.