The detailed description of the project is available at the project page: http://drupal.org/sandbox/diolan/1987506.

Below are some implementation details:
The module implements the Views Display Extender plugin that is available for any displays that uses fields. This plugin allows users to configure the criterion to merge several rows into one. The hook_views_pre_render is used to filter the rows before they are rendered by the Views module.

I tried to do similar thing (without filtering) using the "Views" and "Views Field View" module (http://drupal.org/node/1972592). This worked well to merge values, but doesn't remove the rows with the same values. This approach also requires additional SQL requests (made by Views Field View) and affects the performance.

Project page

http://drupal.org/sandbox/diolan/1987506

Git repository

git clone http://git.drupal.org/sandbox/diolan/1987506.git views_merge_rows

Reviews of other project applications

Comments

dclavain’s picture

dclavain commented
Status: Needs review » Needs work

FILE: /var/www/drupal-7-pareview/pareview_temp/views_merge_rows.info
line 8 Files must end in a single new line character

FILE: /var/www/drupal-7-pareview/pareview_temp/views_merge_rows.module
8 Format should be "* Implements hook_foo().", "* Implements
hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
29 Function comment short description must end with a full stop
46 Functions must not contain multiple empty lines in a row;| found 2 empty lines

FILE: /var/www/drupal-7-pareview/pareview_temp/views_merge_rows.views.inc
8 Format should be "* Implements hook_foo().", "* Implements
hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
8 Function comment short description must end with a full stop

FILE: ...l-7-pareview/pareview_temp/views_merge_rows_plugin_display_extender.inc
--------------------------------------------------------------------------------
FOUND 26 ERROR(S) AND 1 WARNING(S) AFFECTING 16 LINE(S)

You need to correct these small bugs. For more information http://ventral.org

diolan’s picture

diolan commented
Status: Needs work » Needs review

Thank you!

I have fixed most of the errors listed by http://ventral.org.

The only errors that are left are of the type "is not in lowerCamel format, it must not contain underscores". I can't do anything with these errors as my class if derived from the class in Views module, that uses underscores in method names. I have to use exactly the same name for the overridden methods.

There are some places, where I can still use the lowerCamel format (e.g. protected methods), but I am not sure that this will be right. This will lead to several styles to be used in a single file.

Please review the module once again and let me know if there is still something to be fixed.

hardcoding’s picture

hardcoding commented
Status: Needs review » Needs work

Hi diolan,

Please add an README.txt for instructions.

Delete your master branch and add a 7.x-1.x branch. Also read this article.

diolan’s picture

diolan commented
Status: Needs work » Needs review

Done.
The new link to git repository is:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/diolan/1987506.git

ankitchauhan’s picture

ankitchauhan commented
Status: Needs review » Needs work

Hi diolan

The automated review detected some coding style issues. You can check it here http://ventral.org/pareview/httpgitdrupalorgsandboxdiolan1987506git.

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner. Don't forget to add all your reviews to the issue summary and to add the "PAReview: review bonus" tag.

diolan’s picture

diolan commented
Status: Needs work » Needs review

Now it contains only the underscore name errors (I have described this at #2):

http://ventral.org/pareview/httpgitdrupalorgsandboxdiolan1987506git

FILE: ...l-7-pareview/pareview_temp/views_merge_rows_plugin_display_extender.inc
--------------------------------------------------------------------------------
FOUND 9 ERROR(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------
12 | ERROR | Class name must begin with a capital letter
12 | ERROR | Class name must use UpperCamel naming without underscores
16 | ERROR | Public method name
| | "views_merge_rows_plugin_display_extender::options_definition_alter"
| | is not in lowerCamel format, it must not contain underscores
32 | ERROR | Public method name
| | "views_merge_rows_plugin_display_extender::get_options" is not
| | in lowerCamel format, it must not contain underscores
65 | ERROR | Protected method name
| | "views_merge_rows_plugin_display_extender::views_merge_rows_options_form"
| | is not in lowerCamel format, it must not contain underscores
117 | ERROR | Protected method name
| | "views_merge_rows_plugin_display_extender::views_merge_rows_options_form_submit"
| | is not in lowerCamel format, it must not contain underscores
126 | ERROR | Public method name
| | "views_merge_rows_plugin_display_extender::options_form" is not
| | in lowerCamel format, it must not contain underscores
137 | ERROR | Public method name
| | "views_merge_rows_plugin_display_extender::options_submit" is
| | not in lowerCamel format, it must not contain underscores
149 | ERROR | Public method name
| | "views_merge_rows_plugin_display_extender::options_summary" is
| | not in lowerCamel format, it must not contain underscores

diolan’s picture

diolan commented

Can somebody please review my module? I think this module can be very useful. It can handle a number of requested issues of the views module.

I can't get the review bonus for this module, because Views module dictates the coding style for my module!

Should I add and submit some useless module, just to get the bonus and pass the review? Or will this module eventually be reviewed?

pyxio’s picture

pyxio commented

I need something like this so I will test it and report back. cheers kevin

stefan lehmann’s picture

stefan lehmann
Primary language German
Location Hobart
commented
Status: Needs review » Reviewed & tested by the community

Hey there,

Wow, that's an awesome module. I really would like to see this getting picked up into the official repository. I tested it and it works like a charm.

I believe the correct git clone command should be:
git clone http://git.drupal.org/sandbox/diolan/1987506.git views_merge_rows

As you already said the automatic review returns some errors, resulting from class names given by the views module. So all good here.

Manual review: I really couldn't find any problems with your code. Looks all very clean to me.

The only thing I could maybe complain about is how the merging of fields is handled, but maybe I'm missing something here. If fields are merged into one row, I would expect that the merging code eliminates duplicates. As an example, I created a view for nodes with the fields title and type. If I select "Merge values of this field" for both fields I get multiple identical Strings in the second field like : "Page, Page, Page etc" .. I think my expectation would be that "Page" would only be displayed once. But I'm not 100% sure if there might be reasons against this behavior. :-)

However what I said before is no blocker, I suppose and the module looks very good to me. I can't see ant reason why I shouldn't put "Reviewed and tested by community" on it. Hopefully somebody else has another look then. Good luck with that.

Thanks for this contribution.

diolan’s picture

diolan commented

Stefan Lehmann, thank you for your review. I have added the options to merge only unique values as well as an option to count unique/all merged values.

I see the status is already RTBC, but I can't promote the sandbox for a full project yet. Do I suppose to do some additional steps?

diolan’s picture

diolan commented

I am also going to add the possibility to sort the merged values, but I want to see if this module is needed by the community first.

diolan’s picture

diolan commented
Issue summary: View changes

corrected a typo in the description field

diolan’s picture

diolan commented
Issue tags: +PAreview: review bonus

Added review bonus

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed

manual review:

  1. views_merge_rows.info: any particular reason why you depend on the Views UI? Many people do not want the UI on their production sites, but I guess your module still works for already configured views?
  2. "'#title' => 'Separator:',": all user facing text must run through t() for translation.

But that are not application blockers, so ...

Thanks for your contribution, diolan!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

diolan’s picture

diolan commented

klausi,
Thank you for your review and feedback. You are right, the module doesn't need views_ui after it is configured. I have fixed the .info file and added the call to t() function.
I have also released the rc1. Everybody who needs this module welcome to http://drupal.org/project/views_merge_rows. Any feedback and/or feature request are appreciated.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Added reviews of other project applications and corrected the git link.