[D7] Views Fields On/Off [#2394329]

This module allows fields in a View to be turned on or off at view-time by the user. When configuring a fields View, add a Global: On/Off Form. Then select the fields the user may turn on or off.

When the user checks or unchecks the fields in the exposed form area of the View and clicks Apply, the unchecked fields will not be shown in the View display. Hiding fields here also hides them from Views Data Exports of the current display.

pareview shows some visibility and naming issues, but since this is a Views handler, I am patterning it off of how Views formats its own class names and method visibility.
http://pareview.sh/pareview/httpgitdrupalorgsandboxfranksj2392641git

https://www.drupal.org/sandbox/franksj/2392641

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/franksj/2392641.git views_fields_on_off
cd views_fields_on_off

Manual reviews of other projects:
https://www.drupal.org/node/2435083#comment-9748305
https://www.drupal.org/node/2452925#comment-9748339
https://www.drupal.org/node/2452633#comment-9748371

Comments

Comment #1

PA robot CreditAttribution: PA robot commented 16 December 2014 at 17:23

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Comment #2

kyuubi CreditAttribution: kyuubi commented 19 December 2014 at 03:24

Hi,

Interesting module first of all, thanks for your contribution :)

Automated Review

There are some issues identified in the automated review, even aside from the visibility ones.

http://pareview.sh/pareview/httpgitdrupalorgsandboxfranksj2392641git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch. However there is no default branch set. (also stated in pareview)

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template. Some required sections are not present like introduction and requirements. Please follow the README template for further help.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

In your hook_views_pre_view your running a bit of code without first checking for the existence of the special field (which is done bellow), meaning this code run in all views. I would wrap it all in the check you have further down.
Make better use of Drupal API by using drupal_get_query_parameters instead of $_GET
There is an empty query function in views_fields_on_off_form.inc that has no code and no return.

Comment #3

kyuubi CreditAttribution: kyuubi commented 19 December 2014 at 04:08

Status:

Active

» Needs work

Changing status.

Comment #4

franksj CreditAttribution: franksj commented 19 December 2014 at 19:24

Hi kyuubi,

Thank you so much for spending some time to look at the module!

I updated the README so it follows the template.

I replaced _GET with drupal_get_query_parameters(). That was a great tip and I hang my head in shame for not having done that to begin with!

On point 1, the code before checking for the field: The code that runs before the field check gets the display ID of the view, which we need in order to determine whether the field exists. We have to have that display ID so that we know which display to check, since one display could have it and another display potentially might not.

On point 3, that's a good catch, too. I added comments now describing why that empty void function is necessary, which is because the handler isn't a real field. If parent::query() runs, the view won't render, and it's a void, so we need to override it but not actually do anything. But you're absolutely right - it should not have been a mystery as to why that was. I should have commented it.

I've updated and pushed based on your review. Thank you again for checking it out for me!

Comment #5

franksj CreditAttribution: franksj commented 19 December 2014 at 19:26

Status:

Needs work

» Needs review

Comment #6

JulienF CreditAttribution: JulienF commented 22 December 2014 at 08:10

Status:

Needs review

» Needs work

Automated Review

You still have issues to be fixed. Why using the raw form_state input ?

http://pareview.sh/pareview/httpgitdrupalorgsandboxfranksj2392641git

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes: Does not cause module duplication and/or fragmentation.

Master Branch
Yes: Follows the guidelines for master branch. However there is no default branch set. (also stated in pareview)

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md
Yes: Follows the readme template guidelines.

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.

Coding style & Drupal API usage
[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

Comment #7

franksj CreditAttribution: franksj commented 22 December 2014 at 17:27

JulienF,

I am using $form_state['input'] because I'm building a Views exposed form. I'm following the patterns of the Views handlers used by the Views module. For instance, look at views_handler_filter_equality lines 38-43. Is there a better way I should be doing this, other than the way Views implements its own handlers with exposed forms?

The $relationships setting is used to set the labels of the fields so that they don't include the name of the relationship in the exposed form. That's commented in there, too, and is patterned on how Views' own handlers display field names.

Comment #8

franksj CreditAttribution: franksj commented 22 December 2014 at 17:28

Status:

Needs work

» Needs review

Comment #9

artsakenos CreditAttribution: artsakenos commented 24 December 2014 at 07:27

Hello franksj, this is an interesting contribution, please check the following:

Automated Review
⊠ Please double check the pareview page:
http://pareview.sh/pareview/httpgitdrupalorgsandboxfranksj2392641git
and review your code according to the coding standard, codesniffer is still launching few warnings.

Individual user account
☑ Follows the guidelines for individual user accounts.

No duplication
☑ Does not cause module duplication and/or fragmentation
I found no other out of the box module or solutions to perform this operation.

Master Branch
☑ Follows the guidelines for master branch.

Licensing
☑ Follows the licensing requirements.

3rd party assets/code
☑ Follows the guidelines for 3rd party assets/code.

README.txt/README.md
✎ The content itself is ok and follows the guidelines for in-project documentation and/or the README Template.
But the layout is a bit confusing. I would do as suggested in the linked documents: no empty lines after the title and two empty lines after each block.
Right now there is all sort of margins. Please review it.

Code long/complex enough for review
☑ Follows the guidelines for project length and complexity.

Coding style & Drupal API usage
All the code is well commented and organized, except this section; I am not sure it is there for a reason for these assignments or it is just a placeholder for some modification you intend to do, see views/views_fields_on_off_form.inc:92

    foreach ($display_handler->get_handlers('relationship') as $relationship => $handler) {
      if ($label = $handler->label()) {
        $relationships[$relationship] = $label;
      }
      else {
        $relationships[$relationship] = $handler->ui_name();
      }
    }

This seems to have no effect on the relationships handling, which I tested by the way and works perfectly. Please remove if not necessary or explain it better.

Also,
I would make the title of the block translatable
'#title' => 'Show Fields',

Secure Code
I see no security issues.

Project Description
✎ The project page is ok. I would just write two or three lines to highlight the importance of this module. Just an example of why it could be useful.

I tested the module with relationships also, I see no blocking issues, imho it could go to RTBC as soon as you make a cleaning.

Comment #10

franksj CreditAttribution: franksj commented 24 December 2014 at 19:26

Thanks, artsakenos!
I implemented each of your changes. Thank you very much for going over it for me.

Codesniffer is down to only the $form_state['input'], which is necessary and commented, and the function/class names casing and visibility, which are all standard for a Views Handler.

Thank you again to everyone who looked at this. I really appreciate it!

Comment #11

Eugene Fidelin CreditAttribution: Eugene Fidelin commented 9 January 2015 at 13:43

Status:

Needs review

» Needs work

Though module's code looks good - the approach looks strange for me.

1. Why have you decided to use a field for rendering such a form?
Fields are related to the entities that are displayed in a view and not to the view configuration.

So you add something as a "field" but then do not render this as a normal view "field"

I think it is more relevant to use View "Header" and/or "Footer" (or maybe "Exposed form") to add "Views Fields On/Off" form.

2. Other thing is that fields hidden by the user become visible after clicking on a pagination link.

Comment #12

franksj CreditAttribution: franksj commented 12 January 2015 at 19:07

Eugene,

Which commit were you using to test this? I can't reproduce the pagination issue, both using and not using AJAX.

Thanks!

Comment #13

evilehk CreditAttribution: evilehk commented 31 January 2015 at 05:54

Status:

Needs work

» Reviewed & tested by the community

This module helps with a popular use case. Thanks for contributing! The above comment from Eugene regarding fields relating to entities is not always true in the case of views. Thus the views "global" namespace. I do see the argument, however, that because the "Global: On/Off Form" does not appear with the other fields that that is a use case for implementing as an attachment-like view display or as a header or footer. I consider that a recommendation and not a release blocker.

I was not able to recreate any issues that were reported with paging. Paging maintained the selected fields. I even tried with AJAX and different version of jQuery. All worked as expected. I would love to see this module out in the wild. Marking RTBC.

Automated Review

The issues reported are mostly due to the consistent implementation of views handlers. Others are explained in comments and the README.txt file. Overall the code is very clean.

http://pareview.sh/pareview/httpgitdrupalorgsandboxfranksj2392641git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch. Default branch is set appropriately.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements

Coding style & Drupal API usage

List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

Coding style decisions explained in comments and readme.

This review uses the Project Application Review Template.

Comment #14

franksj CreditAttribution: franksj commented 23 March 2015 at 03:26

Issue summary:

View changes

Comment #15

franksj CreditAttribution: franksj commented 23 March 2015 at 04:04

Issue summary:

View changes

Comment #16

franksj CreditAttribution: franksj commented 23 March 2015 at 04:45

Issue summary:

View changes

Comment #17

franksj CreditAttribution: franksj commented 23 March 2015 at 04:46

Issue tags:

+PAreview: review bonus

Comment #18

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 25 March 2015 at 20:47

Status:

Reviewed & tested by the community

» Fixed

Looks good to me after a manual review.

Thanks for your contribution, franksj!

I updated your account so you can 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 stay 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.