Conversation view shows posts that are not part of the conversation [#1511686]

Going back through #1484278: Views handlers broken (still use D6 API) to make sure all the points were addressed, I realized that this one was missed from #14:

The conversation view needs to be changed to add a filter so it only shows statuses with the "user" type.

This is a consequence of changing the "participant" argument query. (This issue will also need to be backported.)

This is a little more complicated than it sounds because Views 3.x will use a different form when exporting than the default views currently use in the files. One solution is to implement this first in D6 and copy it to D7 (it should work without changes I think) or we could convert all the default views files to the 3.x syntax. Converting to the 3.x syntax is better since it will be more forward-compatible, but there are some changes to the default exports in the files. I believe these are (mostly?) indicated with comments. Really they should be moved to a hook_views_default_views_alter() implementation.

I'm okay with taking the easy way out for this issue, but if so let's create another issue to address the syntax porting question at a later date.

Comment	File	Size	Author
#9	conversation-views-fix-1511686-9.patch	26.73 KB	mathankumarc
#7	conversation-views-fix-1511686-7.patch	28.42 KB	mathankumarc

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

mathankumarc CreditAttribution: mathankumarc commented 3 April 2012 at 04:46

Why cant we go for features? Since it has lot of benefits like set of views will be available as modules etc..

Not only for views, we can use features for flag, rules and other integration also. For 7.x-1.x we will just fix the conversation view, for 7.x-2.x we can use features

Comment #2

IceCreamYou CreditAttribution: IceCreamYou commented 3 April 2012 at 05:31

First of all Features doesn't have anything to do with this issue. The default conversation view needs to have a filter added to it, that's it. It would be the same change whether or not Features was tracking the views, it would just be a different way of exporting the views.

Second, components exported by Features can't be altered using alter hooks in code. (I mean, technically they can, but Features will show the components as overridden rather than default.) Every default view that Statuses provides needs to be altered (mostly by submodules) so Features won't work in this case.

Third, Features introduces an unnecessary extra dependency, additional overhead, confusing version numbers, and a more complex upgrade process. It trades developer convenience for end-user pain, which is never a compromise I want to make. Features is designed for bundles of configuration, not for use with complex, full-fledged modules. It was hard enough for me to make the decision to make Views itself a dependency -- I want Statuses to work as independently from other modules as possible.

It's true that using Features would reduce manually tracking changes to the views, but it's just not worth the trade-off.

Comment #3

mathankumarc CreditAttribution: mathankumarc commented 3 April 2012 at 06:00

I suggested features for writing views files in 3.x syntax, however I got your point on using the features.

As you said porting will not be a problem(in either way), if we wrote the views files in 3.x syntax. Can we use this issue to rewrite the views files or a new one? Because to add new filter we need to use 3.x syntax within 2.x, that will not look nice.

I already modified two views(few changes) with 3.x syntax

#1503418: rels_and_me handler is broken (Modified the validation)
#1350292: Tags view doesn't adopt correct vocabulary (Added user id validation)

Comment #4

IceCreamYou CreditAttribution: IceCreamYou commented 3 April 2012 at 08:52

By 3.x syntax I mean that the Views 3.x export code has more objects instead of arrays (in other words there are some places where 3.x and 2.x export code can't be mixed I believe). But yes it's fine to use this issue for switching to the Views 3.x syntax. You've contributed enough now that you can make decisions like that without my approval, and I've just given you the remaining maintainer rights you didn't have yet (ability to edit the project page and create new releases). You can commit small changes without my approval as well, though please continue opening an issue for them to make it easier for me to track changes.

Comment #5

mathankumarc CreditAttribution: mathankumarc commented 3 April 2012 at 22:49

Sure. Will do it.

Comment #6

IceCreamYou CreditAttribution: IceCreamYou commented 9 September 2012 at 07:59

Splitting the Views 3 syntax conversion into #1779234: Convert default Views exports to Views 3 syntax

Comment #7

mathankumarc CreditAttribution: mathankumarc commented 4 October 2012 at 14:31

Status:

Active

» Needs review

File	Size
conversation-views-fix-1511686-7.patch	28.42 KB

Added the filter and In the mean time converted the conversation view syntax to views 3.

Additional note, got column name conflict in participant argument handler and fixed that too.

Comment #8

IceCreamYou CreditAttribution: IceCreamYou commented 4 October 2012 at 20:55

Status:

Needs review

» Needs work

Thanks for working on this.

+++ b/includes/views/handlers/statuses_views_handler_argument_participant.inc
@@ -10,11 +10,12 @@
+    $this->ensure_my_table();
     $argument = $this->argument;
     $this->query->add_where(0, db_or()
-        ->condition("sender", $argument)
+        ->condition("$this->table_alias.sender", $argument)
         ->condition(db_and()
-          ->condition("recipient", $argument)
-          ->condition("type", 'user')));
+          ->condition("$this->table_alias.recipient", $argument)
+          ->condition("$this->table_alias.type", 'user')));

I have never gotten a clear answer on whether $this->table_alias is supposed to be a public API or not, so I try to use $alias = $this->ensure_my_table(); ...->condition("$alias.column"); these days.

+++ b/includes/views/statuses.views_default.inc
@@ -419,405 +391,217 @@ function statuses_views_default_views() {
+  $view->human_name = '';

Presumably we should give the view a "human name"?

+++ b/includes/views/statuses.views_default.inc
@@ -419,405 +391,217 @@ function statuses_views_default_views() {
+  $handler->display->display_options['fields']['nothing']['alter']['text'] = '<div id="statuses-item-[sid]" class="[classes]">
+    [sender_pic]
+    <div class="content">
+      <div class="statuses-participants">[name] [private]</div>
+      [message]
+      [attachment]
+      <div class="statuses-details">
+        <div class="statuses-links">[edit] [delete] [ops] [repost]</div> [created]
+      </div>
+      <!-- meta -->[comment-box]
     </div>
-    <!-- meta -->
-  </div>
-</div>',

The view needs to be exported with the minimum number of modules enabled (basically just the core Statuses module) because some other modules alter the Views output and that will result in broken views for people who don't have those modules enabled.

Comment #9

mathankumarc CreditAttribution: mathankumarc commented 13 October 2012 at 17:23

Status:

Needs work

» Needs review

File	Size
conversation-views-fix-1511686-9.patch	26.73 KB

Here is the updated one.

I have never gotten a clear answer on whether $this->table_alias is supposed to be a public API or not, so I try to use $alias = $this->ensure_my_table(); ...->condition("$alias.column"); these days.
I think its a public one. Here is the code from views/includes/handlers.inc

/**
   * The alias of the table of this handler which is used in the query.
   */
  public $table_alias;

/**
   * Ensure the main table for this handler is in the query. This is used
   * a lot.
   */
  function ensure_my_table() {
    if (!isset($this->table_alias)) {
      if (!method_exists($this->query, 'ensure_table')) {
        vpr(t('Ensure my table called but query has no ensure_table method.'));
        return;
      }
      $this->table_alias = $this->query->ensure_table($this->table, $this->relationship);
    }
    return $this->table_alias;
  }