Hey blackice,
I finished a copy for you to check out/anyone else who may find it useful. Amount of code is about the same, although MUCH more powerful in its current state (especially after token/views is re-added).
I see no reason to fork the project at this point... Assuming your alright with me at a co-authoring/maintaining status.
I got it to a stable point, but I am sure there is still some bugs I have not found.
I don't imagine it will be hard to hook it back into views/tokens, but if you can have a look, that would be very awesome.
That reminds me, there is one thing, the name "extra fields pane" seems like a bad description of where this module currently stands with my modifications. Let me know what you think.
Thanks, hope to hear from you soon!
| Comment | File | Size | Author |
|---|---|---|---|
| #83 | 831134-uc_extra_fields_pane-MC_004.patch | 110.39 KB | megachriz |
| #60 | uc_extra_fields_pane-panthar_004.patch | 96.86 KB | panthar |
| #58 | 831134-uc_extra_fields_pane-MC_003.patch | 94.62 KB | megachriz |
| #38 | uc_extra_fields_pane.patch | 6.7 KB | panthar |
| #31 | 831134-uc_extra_fields_pane-mrfelton_003.patch | 64.71 KB | mrfelton |
Comments
Comment #1
maurizio.ganovelliHi panthar,
I'll test your mod in next days and post here as soon as possible!
I think that, if all ok, we can create a stable 1.0 release (i'm testing last dev on two live sites) with current code and then work togheter to a new 2.x branch.
Bye!
Comment #2
panthar commentedWay cool.
Let me know if there is anything you need.
Comment #3
megachriz@panthar
I tried out your version of the module. When I add a new custom field with accepting the defaults I get this error:
Add new field: you need to provide a way of processing the value for this field as either textbox, select, constant, or php.The field 'Field name' is red outlined, as if the error was with that field. When I change the value of 'Define where the value comes from:' I can get the form posted without errors.
I saw in the form that I can choose in which pane the field will be hooked into. I hoped this meant that when I selected 'Delivery pane', the field would become in the already existing delivery pane, but unfortunalety I noticed the field came in an extra pane. Not quite what I would have expected.
Maybe this module should be extended with more functionality. What I have in mind:
- Administer multiple panes with the ability to title them.
- Ordering fields by grab and move them (as on the blocks editing page).
- Choose to have the extra fields inside both the already existing delivery and billing pane (instead to have separate panes). I think those fields should be appearing on the 'Address fields'-page.
- Ability to order the address fields on the 'Address fields'-page (if possible).
- Integration with contrib module UC Addresses.
Because I will need some of the functionallity described above, I'd like to participate. I only do not have experience with cvs yet.
Comment #4
panthar commentedMegaChriz,
Firstly, looks like there is an error, I should assume a default "value", and not leave the radio buttons blank. Thanks for that.
Your other comments:
1.)Not sure how easy it is to add ability to title the panes with the module itself. Certainly you can do this manually by going through the code, it should only be a 1-line hack inside hook_pane. Administration: Sadly, this was my original intention ALSO, BUT, I could not figure out how to make an extra pane show up without also making a callback function to that pane inside the php-code. So instead of knocking my head at it, I tried to facilitate the making of new panes to be as easy as possible. 3 steps, a.)Create a pane inside hook_pane b.)add a new "pane name" to select(s) in create/edit c.)Add a callback function to the pane you created in a.).
2.)I personally don't know ajax, but that was the intention, to eventually be draggable, this is a start though.
3.)Not currently setup this way, but another potential feature. The problem is, I did change the database to support multiple panes, so re-adding the ability to show up on multiple panes AT-ONCE might be hard.
4.)I can't really change the ordering of the fields of OTHER panes. At least I don't know how.
5.)Never heard of this module, might be worth looking into.
Thanks for the feedback, I am hoping in the next couple weeks, blackice and I can get a stable version released. If you have corrections/new features feel free to upload the appropriate files onto this thread for now.
Comment #5
EpyZooD commentedHi panthar!
I'm looking for these functions for a long time and I'm testing for a few days. It's working perfectly for me. Only one thing, what I miss. The tokens for the invoice template. It was a function in the original modul. I'm not an expert in php, but I need this function. Is it possible to solve this?
Comment #6
maurizio.ganovelliHi pantar,
i spent a long time looking for ucxf future features from current status, considering d.o user's requests and my customer's experience.
This is a short summary:
These are my considerations:
alteration of billing/shipping existent forms and panes, integration with uc_addresses etc..etc.. - just ideas!), one sub-module for
handling more generic extra order data (unlimited and configurable extra panes with extra fields).
Changes aren't simple but, in this way, i think that this module can be really complete and useful.
I also tried your module and i think that some portion of code can be re-used.
Please, let me know what you think about these points.
Comment #7
megachriz@panthar
1.) I didn't study the code very much, but I see what you mean. It's not much a problem to create multiple panes and give each it's own title, but the problem is with the callback-function. It's currently impossible to use the same callback-function for all the extra panes, because Ubercart doesn't give any information about the pane in the callback function. I think there are three ways to achieve this:
- the first way is to make use of drupal_eval(), creating dynamically callback functions this way for example:
- The second way is to implement an own way of creating panes and place them with hook_form_alter()-functions so hook_order_pane() won't be used. I won't recommend this method, because it might be hard to get this right for every possible situation. If there is for example a module that builds the checkout-page in a different way and calls module_invoke_all('order_pane', NULL); to get the panes we probably introduce some incompatibility.
- The third way is to create a patch that makes changes to the Ubercart core modules and then we must hope the changes will made it in Ubercart 2.x core.
2.) There are drupal-functions to get a draggable table: drupal_add_tabledrag(). They can be used in theme hooks.
3.) It might be hard to get the fields in the already existing panes, but it could be done with hook_form_alter(). Compatibility issues are possible as I said in 1.). It would be nice if there was a hook_order_pane_alter()-hook. When I looked for a solution I also came accross this page:
http://www.stellapower.net/blog/ubercart-modifying-checkout-panes
I didn't study this yet, but it might be a good solution.
4.) Changing the order of other panes can also be done with hook_form_alter(). With the Form API you can change the 'weight' of each field.
@blackice78
I think breaking up the module in two submodules is a good idea! Two different functionallity's are asked: modify address panes and add custom panes.
Comment #8
panthar commentedI am getting to the fixes. I have been really busy the last couple weeks with other code projects.
First, I can't repeat the problem that the first poster had, and it all works fine for me..
Second,
I like the idea of two separate modules, EXCEPT that my version encompasses everything that blackice's does. I understand that there is one fallback, that is, you have to declare two fields now if you want a field on both delivery and billing address panes. However, from my perspective, this also gives you more power of choice. Sometimes you may want extra stuff inside the billing and not the delivery, I don't know.. What do you think blackice?
Also, I like the idea of using drupal_eval to create custom panes. If its easy to do, I will definitely implement it in the next couple weeks.
Comment #9
megachriz@panthar
You have a point that it's possible that a user may want some extra stuff inside billing and not inside delivery. The question is then: would the specific billing fields be considered part of the address? And is it a problem if the specific billing fields would be appear inside an extra pane?
My suggestion is that when an user adds an address field, they should appear in both address panes by default. I'm not sure if there is need to have extra billing fields inside the billing pane. But if there is, maybe we can add two checkboxes (one for delivery and one for billing) to let the user decide if (s)he want the fields in both panes. The checkboxes will be checked by default.
Comment #10
maurizio.ganovelliHi all and excuse me for great delay ... but i'm very busy at work.
I created a first stable release for this module from my code, since i tested it for a long time in variuos production environments.
Then i created a 2.x-dev branch with the code posted here: we can work on this for future implementations.
@panthar: i try to grant you an access to project cvs directory but i get the error "panthar does not have a CVS account.". Please apply for a cvs account (see documentation) and let me know as soon as possible (directly with my user's contact form).
@MegaChriz: your last post seems the best way to go. We can try to keep a single a module and add all features (if possible...) listed at post #6.
From next week, until end of next month, i'll be away (for both work & vacations :-) ), so panthar please let me know about your cvs account: with this you can work on this module when i'm away also.
Thank you all,
Bye!!
Comment #11
maurizio.ganovelliChanging issue title, project version and category.
Comment #12
panthar commentedHey Blackice,
So I posted a request at http://drupal.org/node/858662 , they responding by saying that I either need to create a issue on here asking to be a co-maintainer specifically, or, you can post directly to my CVS account request saying that you grant me permission.
If you would like to get this going, I would appreciate if you post on that and let them know you approve of me being a co-maintainer.
Look forward to getting this moving!
Thanks.
Comment #13
megachrizHi panthar,
From next week, for two weeks long, I have some extra time to help with this module. Can you tell me in which way I could help? Should I (try to) implement some features that were listed? Which features are you currently busy with?
I can post my contributed code here as patches.
Hopefully we can get to a first alpha version very soon (within three weeks from now on). That's the goal what I have in mind (for now).
Comment #14
panthar commentedHey Megachris,
I would like to add the multiple-pane ability with that eval() method you outlined. Also, I would like to integrate the token support with this.
I believe I have CVS access now, so I will be able to update the project. If there is anything you would like to add, feel free to post the changes you make here.
Thanks.
Comment #15
megachrizI just noticed a new version of Ubercart has come out. One of the changes were The definitions of cart panes and checkout panes can now be altered by other module. It looks like some new hooks were introduced. Might be worth to look into first.
http://www.ubercart.org/news/jul-16-2010/ubercart_30_alpha_3_released_no...
The new Ubercart version also improved the invoice system by using the Drupal theme layer now.
I haven't looked into the new Ubercart version yet (I will study it next week), but I would say that we should build the 2.x-version of this module upon Ubercart 2.3, because it might be easier and neater to achieve the goals with the new features of Ubercart. That is, if I'm right about that there are new hooks we can use.
I think I'll struggle first with getting the extra address fields in the existing panes, plus add the possibility to order the address fields.
Comment #16
megachrizMy progress so far:
I managed to add the feature to order the address fields. I'm still working on the feature to get the extra address fields in the existing panes. For this I removed the panes 'extra_delivery' and 'extra_billing' from the code, because the extra address fields will get in the original panes.
In order to get a good overview of all the code (many code will be added) I'm currently also reorder functions and putting the 'admin'-functions in a separate file called 'uc_extra_fields_pane.admin.inc'.
I will post a patch later this week, once I got the feature 'extra address fields in the existing panes' ready. If I will not succeed with getting this feature ready before the end of this week I will post what I got.
What's your progress, panthar?
Comment #17
megachrizI've implemented the feature 'extra address fields in the existing panes'. This means the extra address fields will get in the original checkout panes. I still need to figure out how to get them in the original order panes as well.
There is at least one bug in it: when module 'uc_addresses' is enabled and at least one extra address field is added, '/cart/checkout' is resulting in a white screen of death. I haven't figured out yet what is causing this.
Summary of made changes:
- feature 'order address fields' added.
- feature 'extra address fields in original checkout panes' added (not compatible with uc_addresses yet).
- module now relies on Ubercart 2.3
- reordered functions
- little bugfixes
@panthar
If you didn't start yet with your changes, please use this version as a starting point (after you reviewed it of course :))
Patch info
- Patch is made upon the 6.x-2.x-dev version of 19 July 2010 (revision 1.1.4.1)
- File 'uc_extra_fields_pane.admin.inc' added (could not be included with patch).
Comment #18
megachrizProviding a new patch. I found a die()-statement which was causing the white screen of death.
Edit:
@panthar
In the code I added some comments prefixed with 'MegaChriz:'. These comments are meant for you, so you can see what exactly I have changed. You can remove those comments once you reviewed the code.
Comment #19
mrfelton commentedInteresting. I'll be checking this out shortly. I simply need to be able to add 2 checkboxes to the checkout form to add 'data collection preferences' (Click here if you do not wish to receive updates by post
& Click here if you do not wish to receive updates by email). Correct me if I'm wrong, but it sounds like this new version of the module should let me do that.
Comment #20
megachriz@mrfelton
Please note the provided patch is not bug-free (I just found a bug) and currently the extra address fields only get in the original checkout panes and not yet in the address presentations. The patch is just a first step to a new major release.
About the bug: I'm currently examine it, I had an error on the review page when I added one delivery field, but not a billing field.
Comment #21
mrfelton commented@MegaChriz: I can help out with code review and testing. This feature is required on a project I'm working on. Actually, I don't want these additional checkboxes to appear on the Address pane. I don't want to touch the address pane at all, but want two add an additional pane with just these 2 checkboxes. Is this module generic enough to do that? What is the reasoning to tying these additional fields to the address panes? I can see why some people may want to add additional fields to these panes, but if it were more generic, allowing the definition of both fields and panes, it would be a lot more useful, IMO.
Note though, I have not even looked at the new code yet. Will be doing so shortly.
Comment #22
mrfelton commentedForgot to ask - is any of this code in CVS yet? I assume the patch and additional file in #18 are to be applied to the v2 branch, right?
Comment #23
megachriz@mrfelton
Well, exactly the 6.x.2.x-dev version of the module currently provides (or is going to provide) two functionallity's:
- extra address fields appearing in the original panes
- extra other fields in extra panes (what you needed)
It's possible that those two functionallity's will be spread over two submodules, but developing didn't came that far yet.
The reason why the extra address fields need to get in the original panes is that I need a suffix, a title and a gender field for my client. In my opinion, it looks a bit clumsy if those are presented separately.
Checkboxes in the extra (information) pane is currently not implemented, but there are plans for that.
The code of the v2-branch is currently in CVS, yes. The patch in #18 is made upon that version.
If you need those checkboxes fast, you could consider create a custom module yourself and implement hook_checkout_pane() and hook_order_pane() in it. Or else you can help review the changes for this module, provide a patch that adds the checkbox-functionallity, or wait until it will be implemented.
Comment #24
megachrizHi panthar,
I give you a little update so you know where I am currently working on for this module.
I'm currently busy with some refactoring. I noticed the code of uc_extra_fields_pane_form() is similar to that of uc_extra_fields_pane_field_edit(), so I am combining those two forms now in a common API function which I call _uc_extra_fields_pane_addFieldForm(). I have also plans to change the interface into something similar to the attributes-page (/admin/store/attributes/overview). The feature to add extra address fields will get on the 'Address fields'-page. The feature to add other fields will remain on the 'Custom order fields'. I'll post a new patch as soon as its ready. I think that will be next week. I'm optimistic, I think it will be ready at the beginning of the next week (Monday or Tuesday).
Then something else. I've tried to alter how the address is being rendered, but I didn't yet manage to do that without hacking Ubercart core, so I requested a change on Ubercart.org in order to get the feature in. About altering the address-rendering: if you add an extra address field, you probably want to see it in the address text as well instead to have it displayed separately.
http://www.ubercart.org/forum/development/17550/hook_order_pane_alter_an...
I hope they will add it, or else I could add this functionallity as an optional submodule with the warning that it only would work with a patch applied.
By the way, will you review the patch posted in #18?
Comment #25
mrfelton commentedOverview made changes:
* - Added support for checkboxes.
* - Rework widget settings to use defined constants.
* - Code standards review (coder).
Comment #26
mrfelton commented@blackice78: What are the chances of getting this into CVS, so that we don't keep having to attach zip files along with the patch. Also, the scope of this one patch is getting so large that it is hard to tell what is what.
Comment #27
mrfelton commentedWhy is field_name (which is the Label field) used as the unique key in the database? Surely it should be field_db_name (which is the machines readable name)?
Comment #28
megachriz@mrfelton
Yes, you are right. I will change this in the current patch I'm working on (probably going to be posted next Tueday). I will also include your work in my next patch.
I haven't fully looked at your changes yet, but I have already one comment on your changes: you defined the field types in constants, I was thinking of just using strings for the field types. What do you think about that?
Comment #29
mrfelton commentedUpdated patch.
This uses hook_update to adjust the unique key on the database. It also fixes a bug where if you had two different checkbox or select items, the second item would contain all the options from both. This is because $options was not being reset in the loop. Fix is at line 684:
Personally, I prefer the use of constants for this, as it gives one central place where all the options can bee seen/altered. Just makes for cleaner code IMO.
Comment #30
mrfelton commentedWhat I would do though, is consider changing the UCXF_WIDGET_TYPE_TEXTFIELD from a 0 to something else. There are hacks in the code to cater for that fact that sometimes a zero is used (ie. for textfields). I have probably introduced a bug by removing these hacks without also changing the value of UCXF_WIDGET_TYPE_TEXTFIELD from 0 to something else. If we do change the values assigned to the constants, we will also need an update hook to alter any existing values for field_value_type.
Also, looking at the schema - why does field_value_type need to be length 10? That seems a bit overkill since we are only storing single digit values here.
Comment #31
mrfelton commentedThis version changes the 0 to a 5, and updates the database accordingly.
Comment #32
panthar commentedWoah, way cool, I will be looking at this code, I am new to CVS (not completely sure how to do it yet), I will try to get this out and updated ASAP, today or tomorrow and assess what I can do to improve upon this.
Thanks for the patches!!!
Comment #33
megachriz@panthar
Please note that I'm currently heavy improving the add/edit/delete field forms, so please don't make changes on that. Instead make a comment here. I will post new changes on Monday or Tuesday.
And you will only need the last patch, that patch includes also the changes from the previous patches.
Comment #34
panthar commented@megachriz
Appreciate the heads up. I apologize, I have been swamped with work, that coupled with getting all this CVS stuff figured out. I got some time now though. Almost got this CVS goodness figured out. Should be up later today as the 2.0 branch.
Comment #35
zkrebs commentedWill it be possible to upgrade from the previous version, 6.x.1-0 running on Ubercart 2.2 to this?
Also, I have a very specific need I would like addressed if at all possible, in the extra fields section. I would like to have a "Shipping Date" field. The Shipping Dates are not a calendar view, because they are just the first Monday's of every week, minus Holidays and other specific dates. It seemed easiest to just make a drop-down list with the available dates as text values. Would it be possible to use this module to create a drop-down with a list, as an extra field?
I would like to take the value and put it on the invoice and as a token emailed to the customer. The first is necessary for administrative purposes, the second is a nice to have.
Unless the above scenario is totally crazy, noobish, and there's obviously a better solution.
I will in exchange happily test the modules and report problems.
Comment #36
panthar commentedFYI, I commited/tested the patch provided, it will probably show up sometime later today under the 2.0 branch (assuming I checked it in all-right).
@Slavojziek, with regard to an upgrade from previous version, the database is different, and there is not currently a upgrade script in the .install file that updates the tables, feel free to create one if you feel the need, migrations should not be too difficult, many of the same columns are already there.
Yes, I want to get token support back into this module, I will take a stab at it once megacriz gets his changes made. Its kind of a unknown to me how tokens will be combined with multiple panels, but I am sure there is a way.
With regard to the "shipping date", I REALLY like this idea for more features. We could add a select box where the elements are determined by PHP-filter, so you can customarily create the select box inputs with php-code that returns an array. Additionally, this could be applied to other form element types with relative ease I Would imagine.
What do you think? And also, appreciate the testing.
Thanks.
Comment #37
zkrebs commentedThanks for your quick response.
All of the shipping date modules that exist seem to be lacking in flexibility and also having a ton of modules is a pain, it would be more strategic to push off a bunch of little customizations into this module I think.
Existing Shipping Date Modules:
http://drupal.org/project/uc_deliverytimeslot - This is great if you actually have "timeslots" with times, however its a bit too specific for my purposes, and the GUI is excessive.
http://drupal.org/project/uc_ddate - This does most of what I want, but it doesn't allow for "allowable ranges" of shipping dates. The context is just, "pick a date, whatever date you want". If the calendar popout in this only showed allowed dates in a different color, that would be another story. But again, why have a separate module for every little thing you want done in UC?
I like the idea of the select box with a php filter. Just plug in the dates, or whatever your data may be. Customer chooses value on checkout. Ideally it would then be stored in the db, reflected on invoice (for administrative purposes), and then available for emailing and also super-ideally available for views so you could see a list of all the orders by this shipping date. However, that last concern would mandate it would be a real date field, not just a text entry. Could the PHP filter allow me to input values stored as dates? CCK has many ideas about how to do these things, I bet. If this was like a mini-CCK module for UC fields, that would be epic.
Another real world use case I am having is my customer wants to offer "free products" upon checkout. They want to have boxes where a customer can choose them. Right now, I'm using the old version to expose 3 "free product" fields, using String Overrides to have the description of the pane link to a "free product list". The customer hand types the products into these fields. However, what if this field could be a node reference? Then, it would more resemble the setup of the rest of the Drupal site.
I have no creative solution, really looking for ideas on how I could use this great module to store my extra fields, like shipping date, but then store it in the DB correctly, make it easy for the user to select, and allow administrators of the site to find and use the data.
But for now, your simple solution outlined above would greatly assist me and I would accept / test it on my site.
Comment #38
panthar commented@slavojzizek,
I added the ability to create checkboxes/selects with a php input as I specified on the last post.
I attached the patch to this file (It is a patch that must go AFTER the previous patch posted on here), and additionally checked it into cvs into the 2.0 branch, so it should be up there shortly.
@megacriz I attached this patch so you could add this to your version your working on, if you happen to see this in the near future.
@mrfelton
By the way, the checkboxes do not display the selected checkboxes for me inside the order-review and order-view (and possibly order-edit). Do they work for you?
Comment #39
mrfelton commented@panthar: I haven't got that far in my testing yet... I have just been working on the checkout form itself so far. I'll look into it.
Comment #40
megachrizHi panthar,
You can just work on multiple pane support and token support, but if you have changes for the field forms (add/delete/edit), then you could better inform me about those changes, because that part is what I'm currently refactoring.
About a possible mini-CCK: A few months ago, when I first created a module upon Ubercart, I tried to combine that module with the logics of CCK, but that looked kinda impossible, because CCK is designed to work with nodes. CCK is a great module, I think it will be hard to get this module as good and as flexible as CCK is.
I was thinking about introducing some hooks, so new field types can be easier added by other modules (so the other modules don't have to do a bunch of form alter hooks).
About token support:
panthar, since the field names are unique, it might be enough to just title the tokens something like this:
order-ucxf-genderI'm not sure (I haven't thought it out), but it's possible that in the token hooks you need to check which panes are enabled.
@slavojzizek
The latest version (in cvs) depends on Ubercart 2.3. But you can still use it with Ubercart 2.2 if you only want to use custom order fields and not extra address fields. The Ubercart 2.3 depending is because of an implementation of hook_checkout_pane_alter() which was introduced in Ubercart 2.3.
Comment #41
zkrebs commentedThanks MegaChriz and panthar,
I would love to test the current module in UC 2.2 and 2.3 when I get there. As I am not adept at applying patches, would you recommend using the dev module as it stands right now to get the improvements made in #38?
Comment #42
panthar commented@slavojzizek
Take the dev module 2.0 and apply patches 31 and 38. Not sure what OS you are using, but the best freeware program I use for windows is called "win merge".
I thought I updated the cvs for the most current version, but apparently I did something wrong. It shows I committed it, but its not showing up on the page under the 2.0 version. Anyone know whats going on?
@megacriz ,
I noticed there were changes made to the module with regard to the address fields, I am not sure having exclusive support for 2.3 is the way to go. Additionally, this defeats the purpose of having a "1 function" to hook them all, ie- multiple panes like I had originally built.
Comment #43
zkrebs commentedPanthar, took your suggestions.
patched and installed ok. see new admin section. added a field with a checkbox list of free products.
issue, multiple choices on checkout with radio buttons causes error:
on checkout, review order page, i get error:
"warning: mysqli_real_escape_string() expects parameter 2 to be string, array given in /var/www/rvr/includes/database.mysqli.inc on line 323."
Switched to just using a drop down box for selections, and the above error is gone. However, when I submit, I still receive the Internal Server Error".
Notice however that the order is completed and available in admin section, with the field stored on the invoice as requested.
My server setup could be bad right now, thinking sendmail isn't working right. Your module appears to do as stated.
Comment #44
panthar commentedYeah, there is a checkbox problem, and I am not sure I know how to fix this at this exact moment or whether mrfelton who started this is going to finish it? (if so how long) This is why I originally did not build checkboxes in due to complexity, but then I saw this thread and I was under the impression checkboxes worked, but they don't yet.
Comment #45
zkrebs commentedAnother issue with #42's suggestion,
On anonymous checkout pane, only the first field shows in the Custom Order Fields pane, even though there are 4 present.
When I sign in, returning to cart, all 4 are present.
Did not notice a field permission for this under user permissions.
Comment #46
arski commentedHey guys,
I was just pointed to this module by someone and it seems very much that in this particular issue you're trying to reproduce something that is already done by http://drupal.org/project/uc_webform_pane with the integration of webform module. I have to admit that I'm the maintainer of that module, but still, do you think it's a good idea to re-create webform generation in here? I like your module and there is definitely a need for more configurable checkout panes, I just feel that the form part is already pretty much covered.
Take care,
Martin
Comment #47
megachrizpanthar wrote:
Hi panthar,
I agree with you that it would have been nicer if the module could also work with older versions of Ubercart 2. But with the new introduced hook in Ubercart 2.3 I saw a much easier and neater way to implement the new feature (getting address fields in original panes) that I found the Ubercart 2.3-dependency worth it. I thought: why struggle for hours and hours if I can just use the new hook?
About the 1 function to hook them all, I assume you mean the uc_extra_fields_pane_load_pane_type()-function. I added uc_extra_fields_pane_uc_checkout_pane_address()-function because I didn't want to mess much with load_pane_type-function in the first place. I thought: first getting the new feature to work, then refactoring. So the module might need to be improved on that part. But, on the other hand, the delivery- and billingpanes work a little bit different than the information pane, so maybe it's better for the overview to use a separate function for that (which is, by the way, also pointing to the load_pane_type-function in order to get the extra addres fields).
About the differences: the custom order panes will never need to get the pane-contents of the original delivery- and billingpane (which you can get by calling uc_checkout_pane_delivery() and uc_checkout_pane_billing()), but to get the extra address fields inside the original panes this is mandatory. I think the load_pane_type()-function will be harder to read and maintain if it contains specific delivery- and billingpanescode. What do you think?
If supporting older versions of Ubercart 2 is important, we could also split the modules in two submodules, like blackice had proposed earlier. One module would provide features for custom checkout panes, while the other would do the additional address fields (with the notice it depends on Ubercart 2.3). This way, the first module works also with older versions of Ubercart 2.
@arski
I didn't looked at your module, the uc_webform_pane module, yet, but I think you are right that would have a much more powerfull way of adding fields with different types. But it depends on the webforms module and I'm not sure how I can combine webforms with extra address fields functionality. Don't forget it is also my intention to integrate extra address fields functionality with the uc_addresses module, so that would require to combine webforms with uc_addresses also. Maybe this is a way to consider, but if it's not, support of more field types would be needed anyway.
Is it hard to integrate modules with webforms? Is it possible to define fields on other places than the webforms interface? Can I add webform fields to an existing form by using hook_form_alter()? I have to admit I didn't take a look at the webforms code yet.
Comment #48
arski commentedHey there,
Well I think a webform-dependency isn't a big issue really, if any at all. You're right that you have a special agenda with the uc_address and it seems that there is no really easy way to get there (blame Ubercart really).
As for your questions: Integrating webforms with other modules is not that complicated, there are a couple of quirks, but it's nothing too crazy. If you create your own interface for adding fields, then of course you would be able to use that and plug it into the webform system from somewhere else.. I suppose it would be just some copy/paste work from the original webform module.. Adding separate fields using hook_form_alter is slightly more 'weird', but I'm sure that's also doable..
Anyway the real issue with integrating webforms to address fields and such stuff is the storage.. so basically you would have a webform like "extra address fields" that would be displayed somewhere with the ubercart-fields and then stored in the webform table again.. it's all a bit messy, but to be honest that's a better way to go than creating a new form system from scratch - people will always request new fields and stuff, so maybe it's a better idea to do some more work on how to integrate webforms with uc.
Puh.. looking forward to more opinions.
Cheers,
Martin
Comment #49
zkrebs commentedI left a note on the Webform Checkout Pane as well, welcome here Arski.
Until http://drupal.org/node/792550 is easily possible (Displaying Webform Data on Ubercart Templates), this module fills a gap. Its difficult to imagine why there needs to be a separate interface for UC to add fields, when everything else I am doing uses CCK.
Perhaps this module could focus on adding extra address fields, and for the rest of the infinite combinations of fields people might want collected, we use the webform module?
The only other possibility I am seeing is just writing a custom module, which after the last 2-3 months of research on this topic I almost feel like doing. But I don't feel like as a non-coder I will be prepared to make sure it upgrades and keeps working with every new release. Would much prefer that there's a standard way to add extra fields to the Ubercart checkout. IMHO this should be by design, but I appreciate all the work done by everyone else! Almost there, it seems like. I have spoken to numerous people who have passed UC by because of the lack of a simple/obvious way to do this. Would really like to see it happen, would definitely support UC.
And also, its kind of misaligned to say Drupal is infinitely* customizable, so then its a good choice to use Ubercart, but then once you use Ubercart, you can't use the standard Drupal tools to extend it effectively (CCK most notable). If this gap of logic was fixed, it would benefit a lot of projects. I looked around recently..
Not happening easily on Magento: http://www.magentocommerce.com/wiki/5_-_modules_and_development/customer...
Not happening easily on Virtuemart: http://forum.virtuemart.net/index.php?topic=68300.0
Haven't checked all the main shopping carts yet, but it appears as though this problem is usually solved by "creating a custom module". However, what if in Ubercart we could have a do-it-yourself, user friendly GUI for this? Just wondering.
Comment #50
zkrebs commentedhttp://drupal.org/node/854640#comment-3276014
Also, this is a CCK-Webform module. What if this was used on checkout instead?
Comment #51
panthar commented@MegaChriz
Sorry, I did not fully understand what you had done. The functionality to have it integrate directly into the address fields is awesome. Even still though, I would of been nice if we could of depreciated the old UC 2.0 functions from the 2.3 functions. Oh well, sorry if my comment came off harsh, was not trying to be, was just trying to figure out why we had a deliver/billing address's like how they are, and you explained it. Thank you.
@arski
Nice to meet you. Okay, so basically in a nutshell, I have never used your module (nor heard of it) until now. My experience with webforms has always been not needed due to CCK, but, I will have to check this module out in the coming days.
So for the moment, your a step ahead of me, having reviewed this code already.
I am not opposed to combining brain power / codebases, for possibly a more robust module that can do all these things, and allow for CCK-like integration in some way. Mind you, I am just a co-maintainer at the moment, and blackice is away (hopefully he will return soon so we can get his input as well).
Speaking of the "CCK" idea.. My guess is, due to the severe limitations of the pane(s), we would also have to directly contribute to some core UC modules to get to work in a sensible way-- Just guessing, as the panes are PANE to work with in any context like that. (pun intended).
So if I am following straight, your module can do all the form elements via a webform, but lacks in storing the data? Is it the webform module that has this limitation then?
And sorry everyone, I never expected this module to get THIS much attention and contributions so quick. I am trying, to keep up, however, we need to figure out a clear path, we got no real direction between all of us, so by all means, continue to put out your ideas and thoughts... I am sure blackice will want some input in this too..
Comment #52
arski commentedHey again,
@panthar: Nice to meet you too. Everything you said is right, I just want to comment on "your module can do all the form elements via a webform, but lacks in storing the data?" - not quite sure what you mean by that last part. The data is stored in the webform_submissions table, which OK might not be close to UC, but the data appears when you review your checkout, since the last release, when you view/edit forms, and very soon also in the form of tokens for invoices and emails..
@slavojzizek: That sounds like an amazing module to come, let's see what it will be able to do first. Up until now I haven't used webforms too much either as panthar noted, but sometimes using CCK nodes is just too much and one wants just a simple form submission interface. If this new module will be able to do that then I wouldn't mind porting uc_webform_checkout_pane over to that (as well as webforms).
Bottom line is that if we use webforms or CCK fields has no implications on the UC structure - the panes will still be a PANE to work with (nice pun) and UC won't become a well structured module over night.
Anyway, good thing we're all going somewhere :)
Comment #53
megachrizHi panthar,
Thanks for your comments. Actually, after reading your comment I was afraid that all my work would have been for nothing, so I'm sorry too if my comment was also a bit rude. Sometimes you don't realize how easy it is to misinterpret some text. :)
So, before I'm going to misinterpret your last comment, should I split the functionality's now? I mean, should I put the addresses code in a submodule now, together with the modifications I'm currently working on? Because you said 'I would of been nice if we could of depreciated the old UC 2.0 functions from the 2.3 functions.'
About webform integration: I think it's good idea, so we have more flexible ways of supporting multiple fieldtypes, but for now, I think it's importanter to get the new features in (at least for me). So, I hope you don't mind, but I would like to finish my current modifications first. :)
And, right now I'm also implementing the feature to get address fields in both panes by default. For this, I think it's required to store those fields in a separate table, because I need to check if the address field must get in both delivery and billing pane or only one of them. This requires to have two extra columns: 'delivery_pane' and 'billing_pane'. Since those extra columns don't apply on custom order fields (and the 'pane_type' column won't be needed anymore for the address fields), I think a separate table must be used.
This requires also a modification in the load_fields_from_db()-function in order to keep everything working. So the contents of that function will become something like this:
And this would result in an array with all the keys prefixed with 'field_'. Because this would need modifications elsewhere in the module (on several places things suddenly need to prefixed with 'field_'), I am removing the 'field_'-prefix from the database-columns. I don't see a good reason to keep it, except that the upgrade script will be a little bit harder to write. And (you probably hate me by now ;)) I am also changing some column names in order to fit it better to what they are. 'delta' will become 'weight', because the value is used as weight and not as delta. 'name' will be changed in 'label', because this term is also used on the form. I think I'll keep 'db_name', so it won't be confused with the previous 'name'-column. Or should I rename that also?
I promise, after my modifications, to check all the code of the module to make sure I didn't break something. :o
Comment #54
megachrizI'm almost done with my changes, I need to do a few more tests to see if everything still works as expected, so my patch won't come today, but probably tomorrow.
The most important changes will be:
- address fields come in both panes by default
- an table is added to store the address fields
- common API function are introduced: functions for adding fields and a function to generate the field. I know this will be probably replaced with webforms functions in the future, but I was almost finished with this when the comments of arski came up, so I will leave it in for now.
- database field names are changed (see #53 for my reasons)
Patch coming soon! :)
Comment #55
panthar commentedMegacriz,
Look forward to the patch! Not saying this is the 'wrong' way, so don't interpret it that way, I am just wondering why we actually need another database table, with regard to this comment: "And, right now I'm also implementing the feature to get address fields in both panes by default. For this, I think it's required to store those fields in a separate table". Seems to me it would make more sense to just keep a database prefix and one table, I am interested in your reasoning?
arski,
FYI, I really like the idea of webform integration more and more. I agree this is not something that comes to us over night, but I also agree this module combined could add a lot of power to ubercart if modeled correctly.
What I am still somewhat confused about is, what exactly does THIS module do differently than yours if the webform can store all this data already? What is the power of this module over your webform integration, is basically what I am still not understanding.
Lets see what megachriz has done when he releases the patch, and go form there. I know I will be busy for the next couple weeks (crunch time), but I will make sure to keep up on this thread, and hopefully throw in a couple updates.
Thanks
Comment #56
arski commentedHey,
From what I understand the main differences is that the uc_webform_pane module currently only allows to add new panes, while this module initially allowed to edit existing panes by new fields, which is currently not possible with my module. The extension that they're working on now to have separate new panes is exactly what uc_webform_pane does, that is true.
Anyway that's my take on the differences.
Cheers
Comment #57
megachrizHi panthar,
Let me explain why I thought another database table is needed.
From a custom order field we need to know:
- in which pane it will come (assuming multiple pane support will be added in the future)
From an address field we need to know:
- does it come in the delivery pane?
- does it come in the billing pane?
So a custom order field will come in one pane (not more, not less) and an address field will come in one or two panes. Because of this I thought an address field should be handled differently.
To check in which panes an address fields needs to come I thought adding a column 'delivery' and a column 'billing' would be needed. If 'delivery' is 1, the address fields comes in the delivery pane, if 'billing' is 1, the address field comes in the billing pane. If both are 1, the address field comes in both panes.
Having an addres field that has the same name in both panes is important
I found out that it is important that an address field has the same name for both panes. I noticed this will fix 'my billing information is the same as my delivery information' automatically. But I have another reason for this: I have also been working on to get the address fields in the original order panes, but I didn't managed yet to do that without hacking Ubercart core, so that feature isn't included in the patch (yet). However, with that feature you would have been able to add !ucxf_title in the address format on /admin/store/settings/countries/edit/formats and having the address displayed as expected: the feature would replace !ucxf_title with the delivery value of 'title' in the delivery address and the billing value of 'title' in the billing address. If an address field would have two different names (one for delivery and on for billing), the feature would have been less flexible or at least harder to write.
If you want, I can post that feature here also, but it currently only works with a (little) modification on Ubercart core.
I have thought about an other solution:
- an address field could have three different pane types: 'extra_delivery', 'extra_billing' and 'extra_delivery|extra_billing'. This way one table could be kept, but I thought having a pane type value 'extra_delivery|extra_billing' wasn't a very neat solution, but maybe this is still something to consider. What do you think, panthar?
About removing the prefix in the database columns
This change is a result of the added table. I kept a single function to load the fields belonging to a particular pane type. Because the tables have different fields I choosed to just bind the results in an array where the array keys are the same as the database column names. This would result in having an array something like this:
You see, this results in an array where the keys are prefixed with 'field_'. I thought it was a bit unnecessary to have that, And, also, on many places I suddenly needed to point to for example 'field_required' instead of 'required'. That was the main reason for me to scrap the prefix. But note that I probably didn't did this change if I didn't add the address table.
Let me know what you think about this, panthar. I don't think I do mind to rewrite it if an other better and neater solution is proposed.
I continue with completing my patch now, I hope it will be finished in a couple of hours.
Comment #58
megachrizSo, here is my patch finally.
The changes:
- address fields now come in both panes by default
- a table is added to store the address fields
- an API function for generating fields is introduced
- API functions for adding/editing/deleting additional fields introduced
- database field names are changed
- address fields are now added at the 'Address fields'-page
I understand you might not be happy about all the changes (for example the new table for address fields).
So, please review the code and tell me about what change could be improved and how it could be improved.
There might be a better/neater way to implement the feature 'address fields in both panes by default'.
By the way, I noticed the order pane handling could be improved. Editing the extra fields in the order information (/admin/store/orders/#/edit) is not working yet.
Patch info
- Patch is made upon the 6.x-2.x-dev version of 2 August 2010 (revision 1.1.4.3)
I now will continue with uc_addresses integration. It may take a bit longer before this is ready, because from next week I probably also have some other projects to work on (from the previous week until now I worked almost full-time on this project).
Comment #59
panthar commentedHey Megacriz,
I have looked over the code briefly, Thanks for the patch! FYI, I hope you can be a co-maintainer whenever blackice gets back.
So all in all, I really like changes you made (having the ability to put it into an address in the admin section is AWESOME), the only thing I am thinking can for sure be done better is the new database table. That is the reason for the pane_type field, that was there before to distinguish between for example, billing and delivery panes or any other panes. Either I can change it to work with two tablse again, or you can, but I honestly don't see the need for the 3rd database table, unless I am missing something? It's a waste to have two database tables with the same field.
arski,
So I have no idea, as I said I am really busy right now, and don't really have a lot of time for a couple weeks to concentrate on something like this type of integration. If you would like to look into porting your module to this module, and think it would be easy, please feel free to submit a patch to this form, and we can combine efforts. That would make 4 co-maintainers for this project.
I don't really have much else comment, what do you all think we should do...?
Comment #60
panthar commentedFYI added token support.
Since I have not checked in the new address database table version from #58 yet, pending megacriz/others opinions on using two database tables instead of one. However, in the mean time, this patch includes the patch megacriz provided in #58, but with token support DOES NOT CURRENTLY WORK for the extra address fields megachriz added, but DOES work for the EXTRA INFORMATION pane.
The token implementation requires a pane_type variable in the database for what to call the token, at least that was my original intention, and thats why I think adding a second database table specifically for address's makes this module more complicated than it has to be.
I think my original database structure should work fine for this task, and even makes things easier. Do you see what I am saying megachriz?
Comment #61
arski commentedHey panthar,
Well to be honest I feel that uc_webform_pane is somewhat more general, both in terms of the name and actual functionality, so I'm not sure if porting over to this module is a great idea. Basically, if I were to find a way to add webforms to address fields, then the two modules would become pretty much parallel, however, (no offense to you, you've done a great job setting all of this up) since uc_webform_pane is based on webforms it has slightly more options and flexibility in terms of fields and what you can do with them.
So basically I see two options that make most sense.. the first is that we go our separate ways and you continue down the "extra fields" path while i go for "extra panes" so to say.. or I try to integrate extra fields to address panes etc. via webforms and we ask the users which module they prefer etc..
Let me know what you think.
Cheers,
Martin
Comment #62
panthar commentedHey arski,
So I hate to repeat the same things, if not now, certainly the future. I am personally in favor of a port, if it's not to difficult. I wish someone else had some type of comment on this other than me....
In general though, it looks like were trying to accomplish the same thing... and I would very much welcome a port, should you feel it worth our time in terms of long term goals, which I do think are similar by all means.
I personally am in favor of combination, just don't have the time for a 2+ weeks as I was saying.
At the same time though, this module right here, could turn into the "CCK" of Ubercart, if we did this properly by combining all possibly use-cases, including your module's webform functionality. I tend to agree with previous comments on this thread about adding hooks in the future as well, how that would actually work is up in the air though, I know megacriz made some sort of API in this most recent patch, which is a start.
My original intention of this module was the php-side of things. I wanted to be able to add anything I wanted to the order via php, which is what got us to where we are now. There is more power in this than meets the eye, for example, lets say you have an external API you wish to call, and populate a extra-field's select element to be appended to the order. Now that is pretty cool aint it?
Comment #63
arski commentedHey,
I see where you're going with this and it's quite ambitious. However, I'm not sure we need to go this far.. What I want to say is that we already have webforms to build forms, and also possibly CCK-field-based form generators coming in the future which are both powerful tools. I personally hate that webforms have their own form API which doesn't correspond to Drupal's (and look at me, I'm working on a webform-based module), so I'm kind of unsure if we need yet another form API if you know what I mean.
For me, the main questions are building and storage.. (it's relatively easy to populate any field from anywhere..) from this side, webforms have been up to now the most usable, but if there will be a CCK-based form generator I would certainly jump on that ship because it would make the extra checkout pane modules even better compatible with everything else.
Bottom line is, I don't see much future in extending UC's admittedly dreadful pre-defined panes, but in providing a system that can build easy forms, integrateable as panes..
I'm not sure I'm making any good point saying all this.. If any of that makes any sense for you, I would be glad to hear your comments. And either way, let me know what are your main issues with a webform-related checkout pane module - I don't assume you had a lot of time to play around with that (neither have I with your module), so it's good establish a good understanding of what either of us is doing :)
Cheers
Comment #64
panthar commentedArki,
Just my 2 cents, but, after looking at your code, I think it could probably be ported with ease for the most part. I am not sure if you could use the same database table extra-fields uses, I am guessing with a little modification to the extra-fields table, we could accomplish it though.
1 day, 2 max if I was doing it. Which I might, if your not up for the task, I can attempt it in a few weeks, if your willing to give these ideas a try...
I am completely in favor of adding another radio button (or something) that integrates your module as part of the extra fileds module. This ultimately gives a user more choices, all in one modules instead of two, which I think we can both agree is good. The problem is still organization, in both our cases, we have essentially hack-around methods for producing what were doing... At least that's how I feel.... Which is why I feel that as a whole, we all should concentrate on a more abstract concept like a "Custom ubercart content", which is clearly where this module is heading after megachriz's last patch, just needs a better way of organizing multiple types of content, so module developers can plug into it, and a new table-like structure similar to how CCK operates. Doing something like this "CCK" idea could have some base-functionality built in (ie, your webform, and this code) + ability for others to create additional content modules. Maybe I am getting ahead of myself, but I seem to think that using CCK code-base as an example, it's not that difficult if we have a few of us working on it. Even cooler would be to just make use of the CCK system already in place, thus giving compatibility to every CCK module already made, but I am not sure how hard that would be.
Comment #65
arski commentedmhh, I must be honest I really don't see anything big or significantly different coming from this module. You are now doing exactly the same as me - adding fields to panes - however you do that by defining a new API, as I mentioned before. I don't see how I would port the webforms module in here without just copy pasting the whole code and basically using it, which doesn't make any sense - we could just keep my module and integrate your address field edits in there..
Obviously we're both reluctant to give up our respective modules, but we're really doing the same thing, only that webforms isn't building a new API, that's the bottom line as I see it and that's why I see no point in porting webforms in here (or well, in any other way than copying the whole module).
PS. I feel like we're really going around in circles and could use some feedback from other users/maintainers..
Comment #66
megachrizUC Form API vs Webforms vs CCK
I agree with arski that introducing another form API might not be the best way, so I hope the Webforms API can provide enough for our needs. The reason why I introduced a form API for generating fields was not because I thought introducing that was the best way, it was more the result of the refactoring I did. I felt it was better to put the code for generating fields should be in a separate function (it was first in uc_extra_fields_pane_load_pane_type()). This API function now makes it easier for me to get intregration with uc_addresses done. It will be no longer needed to call uc_extra_fields_pane_load_pane_type() in form alters anymore.
I'm totally okay if this field generation function would be replaced with a Webforms function, as long as everything keeps working.
If we would combining this module with CCK then there is no other way than make the extra fields as part of a node, because CCK is designed to work with nodes. Getting CCK to work with anything else than nodes looks very undoneable. So by using CCK, every (custom) checkout pane would be a node edit form (and extra address fields for uc_addresses would be that too).
Extra table for address fields
@panthar
I have to admit I wasn't 100% sure if adding the extra table was the best way. Should I make changes so that it works without the extra table? This would result in having a pane_type 'extra_delivery|extra_billing' (unless you see an other way). In my eyes, two rows for the same kind of field is not an option, because then it's unknown which delivery field is related to which billing field. This would cause that 'my billing information is the same as my delivery information' would not work (and it would also cause in some problems in the 'display address' feature I have been working on, unfortunately this feature currently only works with a little modification on Ubercart, see http://www.ubercart.org/forum/development/17550/hook_order_pane_alter_an...).
Sorry that I'm still talking about improvements, but I have some sort of deadline getting this functionality done as soon as possible (request from my client).
Which direction?
This is what I think we should do: both modules will be kept. This module, Extra Fields Pane, will throw away functionality for custom order information, as that is already done by UC Webform Pane. Instead, Extra Fields Pane will focus on the extra address fields. To gain the power of Webforms, this module will be depend on UC Webform Pane and will be using _uc_webform_pane_edit_form() to get the form elements for the address fields. Additionally, Extra Fields Pane will care that the extra address fields can be added on the Address Fields page (/admin/store/settings/checkout/edit/fields) and also care for ordering the address fields. Actually, exactly as it is done now in the latest patch (from a user point of view).
The biggest problem would probably be to provide a form for the extra address fields on /admin/store/settings/addressfields/add combined with a Webforms form.
You could say, the user should go to Webforms and define the address fields there, but from my point of view a user who wants an extra address field will expect to find that on the Address Fields page. He should not have to bother with Webforms at all.
My thought to fix this: Extra Fields Pane automatically adds a webform node, attached to uc_webforms_pane. The address fields form it presents should be a two step form (as that is how Webforms seems to work). First step is defining a field 'name' and 'type' (and maybe also 'required'), second step a form with the question in which panes it needs to go, combined with the Webforms form (/node/%nid/edit/components/new/%fieldtype).
Problems with this solution:
- uc_webforms_pane shouldn't add a checkout pane, because address fields should go in original panes. Maybe uc_webforms_pane needs a little modification to support this.
- how can address fields still belong to two checkout panes? Should we need to create two webform-nodes for that? Remember it is important that the name of the field in the delivery pane is the same as in the billing pane (if you add a field 'surname_prefix' for example, the field for the delivery pane will be automatically prefixed with 'delivery_' and for the billing pane this is 'billing_'). Also for the address display I want to the ability to add !surname_prefix (on /admin/store/settings/countries/edit/formats) and having the filled in value for the address field displayed correctly for both the delivery address and the billing address. With two different field names this would not work.
I hope we can come to an agreement. In the mean time I will continue to work with uc_addresses integration, so my client will be happy.
UC Addresses integration
Oh, before I forget, adding uc_addresses integration will require a modification to 'uc_extra_fields_values' or an extra table, because uc_addresses has it's own table to store values. Extra address information for uc_addresses should need to be saved separately (can not be done in uc_addresses table). As 'uc_extra_fields_values' currently only stores field values for orders, it can't store yet values for (uc_)addresses. So 'uc_extra_fields_values' will going to look like this:
- element_id int(10) unsigned
- element_type tinyint(1)
for example: 1 = order (uc_order), 2 = address (uc_addresses)
Primary key will be 'element_id' combined with 'element_type'.
Other solution would be to add an extra table, but that would require to update that table when a new address field is added.
Actually, I think having an extra column each time a field is added is not the best way. We better should point to the 'uc_extra_fields'-table, because if you now delete a field, any order information that was collected in the past, will be lost.
So, in that case the 'uc_extra_fields_values' will be like this:
- element_id int(10) unsigned
- element_type tinyint(1) -> order or address
- field_id int(10)
- value varchar(255)
Edit: primary key will be 'element_id' combined with 'element_type' and 'field_id'. (Or should we have a column 'value_id'?)
Seems far better to me. I'm going to change it into this, unless Webforms integration happens fast.
Comment #67
arski commentedHey MegaChriz,
Thanks very much for your comments.
I must say I agree with pretty much all of it as my feeling was also that it would be best for us to go down our separate paths with (possibly) some amount of cooperation. So, as you suggested, your module would continue providing additional features for the address fields and the uc_webform_pane would provide additional other panes.
To be honest I'm not sure if it makes sense and/or would be really easy to integrate webforms somehow as single fields inside the address checkout panes.. it feels like that will be quite a mess and I would like to ask you first if and how strongly do you feel it would be necessary and/or beneficiary to your project? I'll think about it some more in the meantime.
Cheers,
Martin
Comment #68
mrfelton commentedMust say, I have not read all the recent posts in great detail, and I don't really know how the afore mentioned uc/webform module works, but it does scare me a little - the idea of having order information stored by webform. What of someone clears the webform results? Does the additional order data get lost?!
Comment #69
megachriz@arski
The reason why we thought about using webforms integration was of providing more fieldtypes. Having more fieldtypes then there are now is totally unnecessary for my project, so I won't need webforms integration at all for my project.
Because research and implementations for webform integration will require a lot of time, I continue with following the current path for now. Maybe it's better to get to a working module first. At least, that counts for me. The deadline for my project comes very close now.
What will be the changes in my upcoming patch?
- The table 'uc_extra_fields_address' will be removed.
- The table 'uc_extra_fields_values' will be changed. This is because if you delete a custom field later, all previous order data for that field will be lost.
- UC Addresses integration (start)
In the first step to UC Addresses integration extra fillable addressfields will be available. It's possible the address display will not change, as that requires more work.
I hope to post the patch next week.
Comment #70
arski commented@mrfelton - well yes, these are usual webform submissions then.. but if someone clears them the rest of the order will remain in tact.. and I don't really see an issue here as someone can accidentally delete anything on your site - webform is no more/less sage than anything else.
@MC: OK, I figured this integration was probably not worth the effort at this stage. I will however look into it just to estimate how much work it will take and how convoluted the resulting structure will be.
Cheers
Comment #71
mrfelton commented@arski: Yes, anybody can delete stuff if they have the right permissions. But somebody may have permissions to clear webform data, but not have permissions for anything todo with Ubercart, let alone deleting orders... and with such a setup, there is a very real risk that order data could be deleted by mistake.
Order data should be absolutely tied to an order, and the only way to delete it should be by deleting that order, IMO.
Comment #72
arski commentedUmm, I see your point. Could you post this issue over at uc_webform_pane :) this is really not the place to discuss stuff like that, this issue has already been abused enough ;) Thanks!
Comment #73
mrfelton commented@arski: yes, I can and will, but I mentioned it here since there is a great long discussion about the future direction of this module, and I believe that this module should continue down the road of allowing the definition of new checkout panes, and not just limit it to the extension of the address panes, which seems to be the general feel at the moment. I don't think that webform should be used for this purpose at all, though I do think the webform checkout module still has a place.
Comment #74
arski commentedWell, what exactly are your arguments against webforms and for a new form API? I agree that there is the permissions issue that you mentioned and I will fix that soon, but other than that.. you say that "webform checkout module still has a place" yet also that "webform should [not] be used for this purpose at all".. so which is it?
Obviously I'm not in a position to decide where this module will go, but I think the maintainer of this module has also recently stated that there is no real point in defining a new form API if there are already acceptable alternatives, i.e. webforms.. I would however very much welcome you to write down your issues with uc_webform_pane (preferably also in the issue list over at that module) and I will do my best to accommodate you and others.
Cheers
Comment #75
mrfelton commentedWell, you may want to allow people to submit a webform, seamlessly alongside an order - perhaps for a survey, or other generic data collection. However, that is not the same as storing additional order information such as contact preferences, or details relating to the booking specifics of products in the order, or the order itself.
I also agree that defining a new forma API is not the way to go - but there must be a way to use one of these existing Form APIs without relying on their data storage mechanism. And, if there isn't there probably should be!
Comment #76
mrfelton commentedNote: I don't think there is a problem with uc_webform_pane (although I have not actually used it) - I just think the two modules provide different use cases, and there is a time and a place for each (see my previous comment).
Comment #77
arski commentedWell, I don't really see what's the difference between just a "webform" and "additional order information", provided it's available wherever orders are.. At the moment, uc_webform_pane offers additional tokens to invoices and notifications emails, and displays all the information on order (re)view pages.
Maybe just a technical hint or two for you so that you could see why I don't see a difference:
1. whichever API you use, you will have to store the data somewhere in a non-native UC table, meaning that you will have to create a new table, new fields, deal with those etc. (exactly what this module does and what uc_webform_pane does). As there is no way to extend ubercart's existing table, any module that extends the checkout fields will _have_ to create its own.
2. Now assuming you have a module that adds a form and stores the data somewhere, you will have to populate that data to UC order objects, which is done by a set of hooks. Again, no matter how the form is build and where the data is stored, no module will be able to do this in a different way other than extending the order object inside these hooks.
So, if you consider the above, I really think it doesn't matter if the module that extends checkout panes is called "webform" "form" or "field-o-mat", as long as it does what is required :)
Cheers
Comment #78
mrfelton commentedYes, I agree with both your points. The only point I was trying to make is that when it comes to storing additional information that is essential to an order or its fulfillment, this data should be tightly tied to the order. There should be no way to delete one set of data without deleting the other. If you can use the webform framework somehow to acchieve this, then that's great. I just wasn't sure if that would be possible. It doesn't matter to me what the module is called, or who writes it! (I have not really contributed to either). I only care that should I choose to use a module that offers the ability to store additional information along with an order, that that information is as safe as the order data itself :)
Comment #79
arski commentedYep, I totally agree with you on that one. Let's continue the discussion on how to do that in the issue you added at #878716: Tie webform closely to Ubercart order object. My apologies again to this module maintainers for slight abuse of this issue :)
Comment #80
panthar commentedYeah, I am kind of just soaking in what others have to say.
Megacriz, I will check in the code once you have made the db-table change...
....
Me and Arski had a long series of discussions on this, and I get the feeling like everyone has different ideas here.
There is a very real question here...
If you can do all these things with the webform, spend the few hours to do it for address pane(s) using the code pasted here by megacriz as a template?
I just get the feeling like, your 1 step away from doing the same thing as this module arki... And at the same point webform integration, if your module did not exist, would be a natural next step for this module. See what I mean?
Comment #81
arski commentedHey,
I suppose we're close but it's not a 1-hour job to merge the modules, so as MC suggested I feel it's best to do our particular tasks for now.
I will work on #878716: Tie webform closely to Ubercart order object next - it feels like an integral part to make uc_webform_pane integration in the form of additional address fields possible. Right now the configuration of uc_webform_pane is based fully inside the webform's native interface which makes things extremely odd for the user. Once I get that done, we will probably be already much closer to extending it to (sets of fields) as opposed to full new panes.
So what I can suggest is keep track of that thread and put any comments/suggestions/issues there :)
Cheers.
Comment #82
erikjohansson commentedHey guys, I was just about to hack this module as I discovered this discussion thread, and the patches makes this module work just the way I needed! A few tweaks to the views sub-module and I got that working too. (I used the patch in #60, then just renamed some table references)
Regarding the module merge, personally I wouldn't want the data for orders to be saved amongst the webform submission data.
I am looking forward to uc_addresses integration :)
Great work guys, I really appreciate this module.
Thanks,
Erik
Comment #83
megachrizHere's my fourth patch, finally.
In this patch the table 'uc_extra_fields_address' (introduced in my patch '831134-uc_extra_fields_pane-MC_003') no longer exists.
Instead, the address fields are now back in the table 'uc_extra_fields'. I explain how it works now and why it works like that.
About address fields
When you add an extra address field, you'll be asked in which panes you want it: delivery pane, billing pane or both panes (this was also the case in my third patch). When you choose to have it in the delivery pane only, the saved pane type will be 'extra_delivery' and when you choose to have it in the billing pane only, the saved pane type will be 'extra_billing'. Not so surprising. But when you want them in both panes, the pane type will be 'extra_delivery|extra_billing'. I could have added two rows for the address field, but that would cause showing the field up twice in the list and also that the link between the two was lost. Having no link between the delivery and billing field would have caused more problems. The feature 'my billing is same as my delivery' would not work in this case, but also an other feature where I was working on (but could not yet post, as it requires a modification of Ubercart).
But that's the reason one row in the table is used for an address field that's in both delivery and billing pane.
About the values table
Another big change in this patch is the 'uc_extra_fields_values' table. When starting with UC Addresses integration I realized I needed to store extra address fields values belonging to an UC Addresses address, but the values-table of Extra Fields Pane was only used for orders. I could have added an extra table, but in that case the table would also be there even when UC Addresses wasn't installed, so that's the reason why I thought the values table should be changed. Before you start to worry, I included an update script with the patch that will transform the values table perfectly (I tested it multiple times).
Instead of creating columns each time a field is added, the values-table now follows a different concept. It is now normalized. The table structure now will remain the same when adding and deleting fields. The table contains four columns:
- element_id
- element_type
- field_id
- value
The column 'field_id' is linked with the 'uc_extra_fields' table. So, when a new value is added Extra Fields Pane knows to which field it belongs. The column 'value' is where the value is stored in. The two other columns needs some more explaination.
Because now both values for orders as well as addresses are stored in the values table, Extra Fields Pane needs to know to which type a value belongs. The column 'element_type' is used for that. Current possible types are 'order info' (11), 'order delivery' (12), 'order billing' (13) and 'address' (21). The types indicating the value belongs to an order are split into three types because extra address fields sometimes wants to store two values (when the field is in both delivery and billing pane) and in that case Extra Fields Pane needs to know if the value belongs to the delivery or the billing address. If this would not have been the case, only one type for the order would have been needed. When the element type is 11, 12 or 13 (order), the column 'element_id' will contain the order ID. When the element type is 21 (address), the column 'element_id' will contain an UC Addresses address ID.
Other changes
I changed the token hooks implemenations, so that it will work with the extra address fields. I also made changes to the order handler function and the order hook, so that it will work together with my other changes.
About UC Addresses integration
I have made a start with UC Addresses integration, but it is far from complete. That's mainly because there aren't enough hooks available in UC Addresses to make the integration complete. When an address is added, there is no way for other modules to know which ID the new address got, so a change for uc_addresses is required. Editing an address for uc_addresses is no problem, because then the address ID is already known. That's the only thing that works with the provided patch, but it's quite unusable in this state.
There is a discussion about UC Addresses extensibility:
http://drupal.org/node/851658
On that thread the maintainer of uc_addresses suggests that his module could use some rewrite, so that extra address information can be added by other modules (in our case that would be extra address fields). I definitely do not have the time right now to do that also, so I hope someone who is following this thread could look into that.
What's next?
I now will continue with other work I'll have to do, so I don't think I will post a patch with new features soon. Only when I have introduced new bugs a new patch will come quickly (if I have enough time to fix them, of course). I have been working on new features that are currently unfinished mainly because they require modifications on UC Addresses and Ubercart. Those features are:
- ability to modify address display (at /admin/store/settings/countries/edit/formats)
- UC Addresses integration (almost complete)
- 'saved addresses' drop down (currently a bit messy, but it works)
If you want, I can post those features here as an optional extra module along with two patches that will modify UC Addresses and Ubercart.
Comment #84
maurizio.ganovelliHi all!
Wow ... i'm REALLY impressed for all work done in this last month on this module.
Special thanks to MegaChriz for his great work: I will commit your patches as soon as possible, then i'll try to complete/debug/test the code to move toward a stable 2.0 version.
Thank you all for your interest in this module.
Cheers.
Comment #85
ayalon commentedI tested the latest patch and it works well.
I found one problem:
If I use the last option (multiple checkboxes), the return value of the form is an array because its possible to select multiple checkboxes.
The patch of MegaChriz #83 is not capable of saving and restoring array to the database. It will throw a lot of errors on the checkout page.
function uc_extra_fields_pane_value_save($data) {}If $data['value'] is an array, the saving procedure fails.
Maybe one can handle this?
Comment #86
megachrizYou are right. Saving multiple checkboxes also didn't worked before, so I left out a fix for that, as I thought mrfelton was planning to fix it, but also because it's possible functions for defining and generating fields will be replaced with webform functions in the future, so I spend my time to fix other things of the module first.
Comment #87
czeky commentedHi, not sure if getting this right, but when I create "VAT number" extra field, its not saved with UC Addresses, or it should be? Not filled when selected from dropdown of saved addresses
thanx
Comment #88
megachrizYes, I'm still working on it. UC Addresses integration is not complete due to not enough hooks available in UC Addresses. As for the dropdown, I have been working on a solution which is not yet finished. This week I don't have much time to work on it, but I'm planning to finish this feature next week. When it's finished, you will first find it in the issue queue for review (as a patch).
As for UC Addresses integration, I thinking about deliver a temporary solution: a solution that alters UC Addresses, so that extra adres fields gets saved with UC Addresses after placing an order. I hope to post this temporary solution in the issue queue also next week, again as a patch.
Thanks for trying the module.
Comment #89
megachriz@czeky
I had found some time to work on the address selection feature and a temporary solution for UC Addresses integration earlier then expected. You can find the issues about this now in the issue queue of this module:
#913852: Fix 'saved addresses' dropdown on checkout page
#913876: UC Addresses integration
Comment #90
megachrizBecause a lot happened in this thread, it might be unclear what still needs to be done for the 2.x version of Extra Fields Pane.
In #6 of this thread a list was made of tasks needed to be done of Extra Fields Pane.
In the current dev the following tasks/features are finished:
- billing/delivery fields in original panes
- token integration
- ordering address fields
- 'my billing information is the same as my delivery information' (on checkout page)
- get address fields in both delivery and billing pane by default (but still have the option to show field in only one of them)
I have made up a new list for what I think that needs to be done, before we can release a first bèta version.
Tasks left:
- Fix 'saved addresses' dropdown, see #913852: Fix 'saved addresses' dropdown on checkout page
- UC Addresses integration, see #913876: UC Addresses integration and #851658: save aid within form_state when new addresses are saved
- Views integration, see #914806: Views integration for 2.x version
- new panes must have title/description configurable and translatable.
- field types, validation, visibility.
- fix checkboxes field type
- write an upgrade script for 1.x to 2.x
Potential features:
- #904050: Potential features: Address display
- #904054: Show additional address fields in original order panes
- #907446: Potential features: Display options
Because I now have opened separate issues for the 2.x version, I'm setting this issue back to active.
We now could use this thread as a shoutbox:
- to throw in new ideas for the 2.x-version which are not worth a separate issue
- and to give overall feedback on the 2.x version of this module.
Then another question:
The module file of Extra Fields Pane has grown a lot and contains about 1650 lines now. I wonder if we should move some code blocks to another file for better overview and less overhead? For example, should we move the 'API' functions to a different file? It was mentioned in #6.
Comment #91
megachrizI'm planning to fix some issues for Extra Fields Pane in the next two/three weeks:
- #914806: Views integration for 2.x version
- #913852: Fix 'saved addresses' dropdown on checkout page
I'm also going to look for possibility's to move some of the functions to separate files, to increase performance. The goal will be that the module file would mostly contain hook implementations only. This might require some rewrites.
Comment #92
EvanDonovan commentedJust found this issue - going to have to try using the Extra Fields Checkout Pane 2.x version now, since I want to be able to add an email address field to the existing billing information pane.
Are there any known issues I should look out for in using this on a production site?
Comment #93
EvanDonovan commentedJust tested this, and I think that it works well, but has some relatively minor UX and UI issues. Also, the code style could probably be improved some.
How much do you think needs to be done before 2.x can become the recommended release of the module?
Comment #94
megachrizHi EvanDonovan,
I'm currently busy with some improvements on Extra Fields Pane, those improvements are nearly done, so check back in a few days.
It's unsure when the 2.x will become stable, because for some of the features changes are need for both Ubercart and Ubercart Addresses. The fixes needed for Ubercart are for optional features of Extra Fields Pane, see #904050: Potential features: Address display and #904054: Show additional address fields in original order panes. The fixes needed for Ubercart Addresses are required to have full integration with Ubercart Addresses. If you don't use Ubercart Addresses, then that won't be problem for you. See #851658: save aid within form_state when new addresses are saved for more details about what need to be changed for Ubercart Addresses. You can find a workaround here: #913876: UC Addresses integration.
Bugs/shortcomings
- Support for checkboxes is not yet fully implemented for Extra Fields Pane, so that will not be useable (for now).
- Views support is not yet implemented (but that will be in a few days, I got this working already in my local copy).
- The selection box for selecting previous addresses is not working for extra fields, but I also managed to fix this in my local copy, so it should be available in a few days!
Thanks for trying the module.
Comment #95
megachrizOh, forgot to mention: adding an email address to the billing pane won't be a problem. Only if you want to add validation rules you should do this via a custom module with a hook_form_alter(). (See also #888128: Validation for the extra fields).
Comment #96
EvanDonovan commentedYes, the email address is working fine. I'm not going to bother with validation rules at this time, though I may add them later. Thanks for the update on Extra Fields Pane. I am going to create a patch shortly for the integration with the module I maintain, Ubercart/Salesforce Integration: #981074: Integrate with the Extra Fields Checkout Pane module for additional billing and delivery fields.
Comment #97
megachrizEvanDonovan, I didn't update Extra Fields Pane yet! Be careful by extending it's functionality now, because I made some significant changes!
Comment #98
megachrizI have updated Extra Fields Pane. I changed a lot of code to improve both performance and overview of the code.
I changed also some behaviour, so if anyone had written modules that depend or intregrate with Extra Fields Pane, take care with upgrading. Here are some examples of what was changed:
- The function uc_extra_fields_pane_load_fields_from_db() now returns an array with field objects instead of field arrays. This is done to be consistent with uc_extra_fields_pane_field_load() which returns one field as an object.
- There is now a class ucxf_field which contains methods for loading, saving and deleting, but also for generating the edit form and for generating the field. Two additional classes ucxf_customfield and ucxf_addressfield were added, because the needs for address fields differ a bit from that of custom order fields.
I was inspired by the backup_migrate module.
- The function uc_extra_fields_pane_generateField() has been removed, this functionality is now moved to the method generate() of the ucxf_field class, so you can now call $field->generate();
- All weight-related functions are moved to includes/weights.inc
- Most 'extra address'-related functions are moved to includes/addressfields.inc
- uc_extra_fields_pane_load_pane_type() is moved to includes/checkout_pane.inc
- uc_extra_fields_pane_order_handler() is moved to includes/order_pane.inc
The features #914806: Views integration for 2.x version and #913852: Fix 'saved addresses' dropdown on checkout page are now implemented.
Other small changes
- The table for custom order fields has now draggable rows (to rearrange the order).
- Improved security, I had added check_plain() at some places.
- Corrected theme_uc_extra_fields_pane() which didn't returned valid html (it returned '<tr><td><tr><td>' before).
Please let me know if everything still works for you. Although I have tested the module in several ways, there still is a possibilty I had broken something.
Comment #99
EvanDonovan commentedAre there many more API changes that you anticipate doing in the 2.x branch, prior to a stable release of it? I would like to be able to depend on its (very useful) functionality, but need to ensure that my means of integrating with it will not change.
Also, are there schema changes that would make upgrading from the previous dev release of 2.x to the current dev impossible?
Comment #100
megachrizHi EvanDonovan,
There were no schema changes in the last version of Extra Fields Pane, so upgrading should be no problem, unless you were using the unfinished display options feature: #907446: Potential features: Display options.
Note that I will implement update functions if the schema changes in the future.
I'm afraid I can't guarantee that there will be no more API changes in the future (prior to a stable release). For example, I'm not completely satisfied yet with that fields that are in both the delivery and billing pane has 'extra_delivery|extra_billing' as pane type. This is a thing that could change as soon as I have a better idea about it. I was thinking, users don't have the choice to let some of the fields not appearing on a Ubercart Addresses edit form (now all delivery and billing fields are given back to Ubercart Addresses). And having a pane type called 'extra_delivery|extra_billing|extra_address' doesn't sound ideal to me. I have not yet thought about how to improve this yet.
Since Ubercart Addresses integration is not yet complete, there is a possibility that an API change is needed when integration with Ubercart Addresses can be completed.
I understand it is hard to create modules that integrate or depend on Extra Fields Pane if the API is not stable. Should I inform you whenever I'm planning to make changes to it?
Comment #101
EvanDonovan commented@MegaChriz: I don't want you to be too burdened by my work - I would say as long as the changes that you are making do not affect the schema, then don't worry about notifying me. If you have committed changes to the schema, just ping me in #981074: Integrate with the Extra Fields Checkout Pane module for additional billing and delivery fields. I'll test out the new version you made shortly.
I think PHP allows you to iterate over objects as if they were arrays, so my integration should probably still work. If it doesn't, then I'll probably just cast to array first.
Comment #102
megachrizSince there is now an alpha version available, all the proposed features in this issue should be fixed or they shouldn't be needed anymore. Any feature requests should be posted in a new issue.