Hi, I posted this patch in another issue: http://drupal.org/node/122576
But I think it needs to go in a separate patch to get more attention.

This patch allows relativity children nodes to be rendered in a view that's been already created with views module.

Comments

JohnG-1’s picture

Review:

This patch to relativity.module creates new Rendering option for children nodes of type ... , to display children as a (views.module) View - it even gives you a list of your views to choose from.

  • +1 : It does not require any other patches to the current 5.x-1.x-dev branch of relativity.module or relativity_views.inc
  • +1 : It displays a separate table (or list, etc) of children for each child node-type.
  • +1 : The selector widget offers a list of your current views to use.
  • -1 : It doesn't come with a default view to use straight out of the box ... ;)
  • +1 : It generates a column of 'Remove' links in the table, appends the 'Create new ...' and 'Attach existing ...' links to the view footer (replacing the Link Operations of old).
  • -0.1 Quibble: the method for hiding the Link Operations does not appear degrade properly: 'Remove', 'Create ...' and 'Attach ... ' links are not available if the display option is not a view. (Like I care! ;)
  • +10 The big advantage of using views is you can display child node information in addition to the title; by adding fields to the view eg Last updated, Comment count, or CCK fields.

Overall : <gold> ***** </gold>

Some tips on building the view:
If you set the view Arguments thus, you build a single generic view that you can use for displaying all your child nodes each in their respective child_type_table (one-size-fits-all :).

  1. First argument : Relativity: Parent Node ID - Display All Values
    This shows only children of the current node.
  2. Second argument : Node: Type - Return Page Not Found
    This allows a view Filter [Node: Type] to select which child_type_tables are displayed / hidden. (returns the view [Empty text] for hidden child_types)
  3. Third argument : Node: Type - Display All Values
    This shows the children in the correct child_type_tables.

Page Views work fine but I have not managed to get everything working in a Block View yet (still arguing with the Argument Handling Code ...).

darius’s picture

Very nice work!

Three issues before I apply this patch:
1) There's been some minor code changes in CVS HEAD, so the patch does not apply cleanly anymore.
2) Please use 2 spaces, not tabs for indentation (see Drupal coding style guide)
3) Quoting:

-0.1 Quibble: the method for hiding the Link Operations does not appear degrade properly: 'Remove', 'Create ...' and 'Attach ... ' links are not available if the display option is not a view. (Like I care! ;)

This is very important! We should care about graceful upgrades. For a lot of people using views for displaying a few children is an overkill. Backwards compatibility is a must here. If views are not installed or used, this patch would cripple existing functionality. This change should not be too hard to implement, though.

4) I would prefer the views related code to be implemented in a separate function (maybe move to relativity_views?). The theme function is already too cluttered as it is.

Thanks for all the good work.

darius’s picture

Status: Active » Needs work

Very nice work!

Three issues before I apply this patch:
1) There's been some minor code changes in CVS HEAD, so the patch does not apply cleanly anymore.
2) Please use 2 spaces, not tabs for indentation (see Drupal coding style guide)
3) Quoting:

-0.1 Quibble: the method for hiding the Link Operations does not appear degrade properly: 'Remove', 'Create ...' and 'Attach ... ' links are not available if the display option is not a view. (Like I care! ;)

This is very important! We should care about graceful upgrades. For a lot of people using views for displaying a few children is an overkill. Backwards compatibility is a must here. If views are not installed or used, this patch would cripple existing functionality. This change should not be too hard to implement, though.

4) I would prefer the views related code to be implemented in a separate function (maybe move to relativity_views?). The theme function is already too cluttered as it is.

Thanks for all the good work.

JohnG-1’s picture

StatusFileSize
new60.87 KB

I'm attaching a patched (relativity_4_0.patch) copy of the whole relativity.module so people can test it without going through the rigmarole of applying the patch themselves ...

@ darius 3 - 'Quibble' - yes of course you are right this does need to be addressed for non-views users. On the point about view tables being overkill for lists of one or two children I also agree, but the view can be set to return a list format (or teaser, or full node even) instead of a table ... views gives you more list theme/configuration flexibility than Relativity could realistically ever provide.

@ darius 4 - 'views_relativity.inc' - +1 this idea: it would get around problem 3 ... is it just a matter of moving the added code across or would the views stuff need to be rebuilt in to separate functions to be placed in the views.inc ? what happens if you have 2 copies of "function relativity_links" ...?

JohnG-1’s picture

Minor bug appears when relativity_4_0.patch is applied.

  • If TypeA is an allowed child of TypeB, and the Rendering Option for TypeA (under TypeB) is set to a View;
  • If I then unselect TypeA from the (Regular Settings -> Options for TypeB nodes ->) Allowable Child Node Types;
  • the View Table (empty in my case) and functional Operator Links for TypeA are still displayed on TypeB nodes.

I guess this means that some rendering option variable is not being reset properly when the Allowable Child Node Types is modified ... I have NOT tested this with an unpatched version of Relativity, but I think I would have noticed it before if the bug is not brought to light by the patch.

Sorry, Omar :(

JohnG-1’s picture

Version: 5.x-1.x-dev » master

I would like to see this patch go forward but as a non-programmer I'm a little bit intimidated by some of the things that need doing. Anyway here's my thoughts on each change that Omar's patch makes. Some don't seem necessary, others would need someone cleverer than me to get them working from the relativity_views.inc file. Caveat: Please bear in mind that I have no idea what I'm talking about ;)

  1. @@ -633,6 +633,19 @@
    is safely wrapped in "if(module_exists(views)) ..." ?
  2. @@ -1104,12 +1117,14 @@
    is now unnecessary because darius' p.patch allows the Link Operations to be disabled from the Display Settings ?
  3. @@ -1272,6 +1287,64 @@
    the big addition to theme_relativity_links function. I don't know how this can be moved into a discreet function so it can go into _views.inc. I suspect that the theme functions would benefit from being broken down into a series of smaller functions in order to allow _views.inc functions to hook in. I need help with how to do this ?
  4. @@ -1293,19 +1366,26 @@
    I think is part of the above task. The new function relativity_handle_view_links should move to _views.inc easily ?
  5. @@ -1390,7 +1470,7 @@
    only seems to call drupal_get_destination() to return the user to the parent node after creating a new child ... I don't see why this is added ?

Would anyone else be interested in having a look at this?

darius’s picture

John, thanks for your efforts. I will get to this very soon.

owahab’s picture

JohnG: thanks for your continous efforts in relativity module.

@@ -633,6 +633,19 @@
is safely wrapped in "if(module_exists(views)) ..." ?

You're probably right, it should be done that way but let me think out loud: this will only be necessary if the user chosen a view to render children type foo (this action requires views to be enabled otherwise the user won't see the list of views) then after hitting save, the user disabled views module.
I don't know but I feel this will give the user a hint why the view isn't rendered.
Probably the best practict is to have a "else" statement to handle this case.

@@ -1104,12 +1117,14 @@
is now unnecessary because darius' p.patch allows the Link Operations to be disabled from the Display Settings ?

That's right.

@@ -1272,6 +1287,64 @@
the big addition to theme_relativity_links function. I don't know how this can be moved into a discreet function so it can go into _views.inc. I suspect that the theme functions would benefit from being broken down into a series of smaller functions in order to allow _views.inc functions to hook in. I need help with how to do this ?

Well, the function actually is doing a foreach and i didn't see a better way to do the rendering away from the foreach.
Will re-think about this.

@@ -1293,19 +1366,26 @@
I think is part of the above task. The new function relativity_handle_view_links should move to _views.inc easily ?

True.

@@ -1390,7 +1470,7 @@
only seems to call drupal_get_destination() to return the user to the parent node after creating a new child ... I don't see why this is added ?

This should have gone in a separate patch: why not send the user to the parent node when the follwing use case happens:
1. The user clicks "attach ..." link.
2. Fills the new node from.
3. Click submit.

What happens now is the user going to the node view screen which didn't seem to be reasonable.

jastraat’s picture

This is exactly what I was looking for! What is the status on this patch?

owahab’s picture

Assigned: Unassigned » owahab
goose2000’s picture

Got the full patched mod today, will test. Want to say so far, this is the
best, easiest to understand and use parent-child node builder I have
come into. Nice work.

goose2000’s picture

Will this become part of relativity.module ? I guess it didn't make it in the last release.

darius’s picture

Status: Needs work » Needs review
StatusFileSize
new6.39 KB

OK, this important issue needed some restarting, so here we go.

I took Omar's patch, updated it, cleaned it up, and applied to 5.x-2.0-dev (you might have to wait for 24 hours before it gets packaged automatically for download). The CVS tag is DRUPAL-5--2. I am attaching the updated patch, too, so you can also just apply it to official release 5.2.

This patch needs some serious testing and reviewing. It might not work at all yet, since I did not do much testing. So, if anyone can help testing, please do so. If you the review is long and detailed, maybe it is even better to open a new issue.

Thanks.

goose2000’s picture

Hi, I started with the patch this AM, Good and Bad:

Good:
patch applied fine.
the option to use views did appear in the select box
It showed the view, with some problems.

Bad:
Errors, but thinking the better way to test is to setup a simple and new
test case before I go pasting errors all over. I'll set that up now...

IIS 5, PHP 5.1, MySQL 5.0, Drupal 5.1

John A

goose2000’s picture

Hi, I set up a fresh parent - child relation between to new CCK nodes, with the child having a few custom text fields added.

Made a view for the child. It included Title, and the 3 custom text fields (name, organization, FTE). Tested the view to see it work on its own, all fine. Then went and applied this to my display options in the admin relativity module, for the parent to display the child.

When I load the page I get some errors:

# warning: Invalid argument supplied for foreach() in C:\Inetpub\wwwroot\drupal\modules\node\node.module on line 504.
Hmmm# warning: implode() [function.implode]: Bad arguments. in C:\Inetpub\wwwroot\drupal\modules\node\node.module on line 508.
# user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 query: SELECT n.nid, n.vid, n.type, n.status, n.created, n.changed, n.comment, n.promote, n.sticky, r.timestamp AS revision_timestamp, r.title, r.body, r.teaser, r.log, r.format, u.uid, u.name, u.picture, u.data FROM node n INNER JOIN users u ON u.uid = n.uid INNER JOIN node_revisions r ON r.vid = n.vid WHERE in C:\Inetpub\wwwroot\drupal\includes\database.mysql.inc on line 172.

And below this, you can see the view, table format. But it shows it 3 times. So there were only 3 child nodes that would show, but it (the view table) was shown 3 times. So If I add another, I expect there will be 4 tables shown.

Loop issue, hmm. - Thanks for continuing this - John

goose2000’s picture

One other good note; after switching back to show 'just title' all functions appear to be working normal. Degrades well.

darius’s picture

Assigned: owahab » darius
StatusFileSize
new8.63 KB

A new patch. Cleaned up some more code, seems to work better now. To get rid of the warnings, I commented out the following field from the views function -- can someone tell me what it does (it's Omar's code)?

/*
  // the $view will maintain its field contents, so I wanna make sure
  // I will add the extra field only once..
  static $fields_count;
  $fields_count = count($view->field);
  $view->field[$fields_count] = array(
    'vid' => count($view->field),
    'tablename' => 'node',
    'field' => 'nid',
    'label' => '',
    'handler' => 'relativity_handle_view_links',
    'sortable' => 0,
    'defaultsort' => 0,
    'fullname' => 'node.nid',
    'queryname' => 'node_nid',
  );
*/

Also, could someone test this new patch with two different children types, maybe setting one type to display as a view, and another - as simply an old list of titles?

owahab’s picture

darius,
First I'm sorry for being away the past time.
Second, this chunk of code adds "Attach/Remove" column to the rendered view.
I will try to test the new patch.

goose2000’s picture

I have been using this from CVS:

1.43 5 days ago darius Fixing views patch (issue 130870)

It is working good! I can have either views or title lists.

kirilius’s picture

Any idea when the patch will become part of the release?

darius’s picture

You can download it at http://ftp.osuosl.org/pub/drupal/files/projects/relativity-5.x-2.x-dev.t...

Once a few people say it works, I might add an "official" release with this patch.

kirilius’s picture

I tried it (downloaded from the above link) and it did not work for me. I have a few questions:

- Is the view specification from the initial post mandatory? I mean do I always need 3 parameters? I just need to understand why do I need them. What is going to happen if I leave only the first one?

- The initial post said that there was a problem with the block view. Is that still true?

What I am trying to achieve:
1) I have an A-type node with multiple child nodes of type B-node
2) B-node is an image (created using CCK, imagefield and Imagecache)
3) In the full view of the A-node I want to see it's contents followed by a couple of image thumbnails of the B-node children + a link "more"
4) That link will lead to a page where all B-node children are listed along with the link operations (remove, etc.)

1 and 2 are fine
3 and 4 will be easily implemented with a combination of a Block and Page View. So I created the view, told Drupal to create both block and page, checked "More link", added a thumbnail field, then added the tree arguments as described in the initial post and... I see nothing under the A-node contents?

Is this because of the block-view support is still missing or I am doing something wrong?

In fact I already achieved something very close to that using the advice given here: http://drupal.org/node/151412

The part that I couldn't do using that method was the built-in link operations in the view + some other minor details. Besides the solution added in this patch is much more generic and elegant.

Any idea when the block-view support will be added?

Thank you!

owahab’s picture

First: the patch sends three parameters to the called view which does not imply that you "should" have three parameters defined in your view.
Second: I think what you're trying to do can not be achieved without views but you will still need to tweak the view a little bit.
So you can create a view as mentioned in http://drupal.org/node/151412 and then use the -dev relativity to show that view below nodes of type A.
I have tested that and it worked.

kirilius’s picture

So that view will work in a block?

kirilius’s picture

One more question. In the initial post you write:
"+1 : It generates a column of 'Remove' links in the table, appends the 'Create new ...' and 'Attach existing ...' links to the view footer (replacing the Link Operations of old)."

Where are these links added? Which "table" is that? Is that the table from the custom view output? If this is true, what types of views are supported? Will the bonus pack grid view work?

I am kinda lost here... ;-)

Thanks for your help!

darius’s picture

These features were removed from the patch. We have to iron out the existing views functionality first. If the features are needed, open a new issue, please.

kirilius’s picture

Thanks, I will!

kirilius’s picture

Is there any schedule for releasing this patch?

kirilius’s picture

So is there any plan?

darius’s picture

The patch is released (get the 2.x-dev version), but the branch is not tagged as official release yet. You can still use it, though. It's all quite arbitrary on the module level anyway.

In general, things are slow because I am away till September, and even then I'll be quite busy till October. Unless Omar takes charge in my absence, there will not be much new developments till October.

kirilius’s picture

Thanks a lot, I am really looking forward to the stable version!

owahab’s picture

Status: Needs review » Fixed
Anonymous’s picture

Status: Fixed » Closed (fixed)