Home » Forums » Ubercart » Development

Views SKU integration for uc_product.module - patch supplied

Submitted by bengtan on Mon, 04/07/2008 - 21:52
bengtan
Posts: 8
 Joined: 01/25/2008

Hi,

Please find attached a patch which adds a couple of minor views integrations with the Product SKU.

The first is that the SKU can be displayed as a node link.

The second is a views filter by SKU.

I also tried doing a price filter, and was almost successful, but not quite. I ran into some weird mysql query issue. I wouldn't mind giving price filters another shot, but I think I'm going to need to bounce the problem off somebody.

Anyway, this patch is against 5.x-1.0-rc2. My development system is currently 1.0-beta2, so I didn't actually test against 5.x-1.0-rc2, but that part of code hasn't changed between beta2 and rc2, so it should work.

Feel free to commit this to rc2, or leave it for the next feature release, as you see fit.

If there are any problems, or anyone wants to help me chase up the price filters, let me know.

Thanks all.

AttachmentSize
ubercart-5.x-1.0-rc2-views-sku-patch1.txt1.21 KB
permalink #1 Fri, 04/11/2008 - 17:35
Ryan

Posts: 5617
 Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

Thanks a lot for the patch. We'll check it out on Monday... makes me wonder if we shouldn't just leave Views integration up as a separate project so we can roll out new versions faster than we might the Ubercart core. Sticking out tongue

permalink #2 Fri, 04/11/2008 - 18:29
TR
Posts: 971
 Joined: 11/05/2007
Bug FinderFAQ ModeratorGetting busy with the Ubercode.

I've seen a number of other projects that include Views integration as a separate module - Drupal Userpoints for example (I was just looking at this the other day). I think it's a good idea to keep the views stuff out of core if it can be provided easily as a separate module - we have to keep up the fight against bloat here, and Views integration is only useful if you have Views installed, and you might not even use it then...

--

<tr>.

permalink #3 Mon, 04/14/2008 - 16:41
Ryan

Posts: 5617
 Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

TR - any thoughts on whether a change like this is too disruptive for an RC version? It's either now in RC or it won't get done until Drupal 6.

permalink #4 Mon, 04/14/2008 - 20:37
bengtan
Posts: 8
 Joined: 01/25/2008

My two bits ...

The patch is quite innocuous. After patching, these were the immediate effects (in my experience).

If someone is already displaying the SKU in a views table or list, the SKU will turn from static text into a nodelink (because views picks the default option which is to display as a nodelink).

In their views filters dropdown, there will be a new SKU filter. Since they obviously weren't using the filter before, it has no effect on their existing views.

That's pretty much it. The patch doesn't actually include any new code - it just fills in definitions which utilises exising views handler code.

So, in practical terms, I expect the patch to have negligible impact.

From a project management point of view, however, it is new functionality, however small, and some would argue new functionality doesn't belong in RC releases. A line has to be drawn somewhere otherwise you'd be forever adding trivial enhancements to RC's.

Anyway, those are just my thoughts.

The decision is yours, not mine, and I'm fine with whichever way you choose.

permalink #5 Tue, 04/15/2008 - 08:21
Ryan

Posts: 5617
 Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

I was thinking even whether or not I could justify pulling all the Views stuff out into a separate module. That seems firmly out of RC territory, but there also won't be a chance to do it for Drupal 5 if I don't do it now.

Adding in your functionality to the existing core shouldn't be a problem, as we'll have to do new minor releases every time we want to add more Views integration if we don't pull it out into its own code.

permalink #6 Tue, 04/15/2008 - 09:07
TR
Posts: 971
 Joined: 11/05/2007
Bug FinderFAQ ModeratorGetting busy with the Ubercode.

I'm anxious to see 1.0 finished! In general, adding new code at this point is dangerous because it's not going to get a very broad testing before 1.0 come out. Something as silly as a typo (or a missing parenthesis Smiling ) may not show up until a few weeks down the road, right after 1.0 is released. In which case you will have to immediately release 1.01 Sad

I think making Ubercart more accessible for Views is a good thing - this patch is just the tip of the iceberg, isn't it?

--

<tr>.

permalink #7 Tue, 04/15/2008 - 09:14
Ryan

Posts: 5617
 Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

Aye, that's why the heart of the question... is removing the current Views integration in core to its own module worthwhile this late in the game? In otherwords, making a separate project on drupal.org called Ubercart Views Integration and putting our existing code and this code (and others) into it.

This will break the implementation for anyone already using Views w/ Ubercart, but I'm not sure how much. Will they simply have to install the new module and it'll work? Or rather will they have to rebuild any View already in use? ... Which would be awful. Smiling

The alternative is to keep what's in core there and just start a new project for all future integration. This sort of sucks, particularly when there's improvements to core features - like SKUs having a link option.

permalink #8 Tue, 04/15/2008 - 11:16
TR
Posts: 971
 Joined: 11/05/2007
Bug FinderFAQ ModeratorGetting busy with the Ubercode.

I certainly wouldn't rip anything out of core right now, even though I think Views integration might be better off as a separate module (if possible). Architectural changes will just have to wait, unless you're willing to delay 1.0 another few months. I don't think it's a *mistake* to have it in core, just that it might be better off on its own since it's an advanced site-building feature not used by most people. People using Views have the knowledge needed to cope with a change like that in the future, people who aren't shouldn't notice it.

I'm not opposed to putting this patch in right now.

My big concern, however, is having a well-tested, stable 1.0, and I don't think that can happen while the codebase is a moving target.

I'm not real familiar with Bazaar, but with other revision control systems I've used you can define a release across different versions of different files - it doesn't have to be the latest version of each file. If Bazaar supports that, I would freeze the RC at the latest versions; 1.0 would differ from the RC only for those files that need a bugfix. Development should continue, however, so by the time 1.0 comes out there might be quite a few differences between 1.0 and the latest Bazaar versions. But it's important to freeze the RC because people aren't generally testing the latest Bazaar - they're testing the current RC. (Already, there are a number of fixes/changes/additions since the RC2 that haven't been subjected to general testing.) So at some point, if you want a well-tested 1.0, you're going to have to stop making changes to the RC, even little ones. That doesn't mean improvement has to stop, it just gets into the next minor release.

--

<tr>.

permalink #9 Tue, 04/15/2008 - 11:20
bengtan
Posts: 8
 Joined: 01/25/2008

Here's a middle of the road approach used by some modules, which you may or may not want to consider.

Move the views integration stuff into a separate module, but still keep it in the same tarball.

For example, the event module has an event.module and a contrib/event_views/event_views.module.

That way, the views integration code is part of ubercart core, but isn't turned on for sites which don't use views.

Then, you can post a requirements note in the release notes to enable the new ubercart_views module immediately upon upgrading ubercart core.

 

 

On a separate note ... if you are moving views code around, it shouldn't break any views as long as there is no change in the code ie. purely cut-and-paste with no edits.

Views grabs its definitions via a hook_views_table() and then a hook_views_table_alter(). If you move code into a separate file or module, views will just pick them up again from the new location via the hooks.

As a safeguard, in the release notes, you may recommend that websites clear their views cache immediately after upgrading ubercart. This is done via Administer / Site Building / Views / Tools and is part of views itself.

permalink #10 Wed, 04/16/2008 - 12:06
Ryan

Posts: 5617
 Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

Ahh, yeah, thanks for the input and clarification, bengtan. Smiling

I think I would still consider breaking it out to its own project so it can have faster releases than Ubercart as a whole. However, what you propose may be a good intermediate step moving from D5 to D6.

In the meantime, I tested your patch with this View. Works great! I'll get it committed with one or two other Views tweaks. Thanks a lot for your help!