A small gap I noticed when using this module on a client site with a layout manager like display suite was that there is no check on the nodes view mode when the code in hook_node_view runs. This manifested itself by nodes rendering in a non-full layout (eg: custom teaser, sidebar, whatever) either causing the parent page to become password protected when a referenced node with protection was rendered in a sidebar or the protected node would render in a limited view mode (like search result).

I think it's unfair to impose a patch that assumes you use a layout manager, but I did write a small patch that supports DS so that I can pick which view modes to enforce password protection on. It'd be great if someone could either extend or adapt as required, but I have no immediate expectations or requests for this to be merged in or not.

Thanks

CommentFileSizeAuthor
#10 protected_node_doesn_t-2331517-10.patch10.51 KBgrimreaper
#7 protected_node_doesn_t-2331517-7.patch4.24 KBgrimreaper
#5 protected_node_doesn_t-2331517-5.patch4.24 KBgrimreaper
#1 protected-node-check-view-modes-2331517.patch2.51 KBjohan.gant

Comments

johan.gant’s picture

johan.gant commented
StatusFileSize
new protected-node-check-view-modes-2331517.patch2.51 KB
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Active » Needs review

Hello,

Thanks for your contribution. I will test it this weekend.

I pass the status to needs review to trigger the automated tests.

I think you can filter on view modes like you have done without relying on DS.

Status: Needs review » Needs work

The last submitted patch, 1: protected-node-check-view-modes-2331517.patch, failed testing.

izus’s picture

izus commented

hi,
i didn't think in depp to this but let's keep in mind that usually people protect a node because of data sensibility of some of it's fields. it may lead to bad surprised if some view mods display them.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs work » Needs review
StatusFileSize
new protected_node_doesn_t-2331517-5.patch4.24 KB

Hello,

Here is a patch that doesn't require Display Suite for what you need to do.

If the admin does not select the view modes he/she wants to be protected, all node view modes are protected.

@johan.gant : The warnings from your patch come from the fact that you have inverted haystack and needle in your in_array in your check function.

@Izus : do you want that I implement tests for this new feature ?

Status: Needs review » Needs work

The last submitted patch, 5: protected_node_doesn_t-2331517-5.patch, failed testing.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs work » Needs review
StatusFileSize
new protected_node_doesn_t-2331517-7.patch4.24 KB
izus’s picture

izus commented
Status: Needs review » Needs work

Thanks both for the work done.
as suggested by @Grimreaper, i think that tests are a "good to have" here. so that we wil feel confortable with other new patches :)

johan.gant’s picture

johan.gant commented

@Grimreaper sorry about that, clearly wrote the original patch in haste and didn't spot the order of params. Thanks for the revised patches too, very useful.

@izus: your point about being careful with which viewmodes expose certain fields is valid. However, I think it's reasonable to limit this to a caveat for people who do use layout engines like ds or panels instead of not providing the flexibility in the first place.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs work » Needs review
StatusFileSize
new protected_node_doesn_t-2331517-10.patch10.51 KB

Hello,

I have added two tests : one for allowed user, one for not allowed user. Both can see a node in an unprotected view mode.

For assertresponse method the $this->group is useless (not taken into account).

Thanks for the review.

  • izus committed ccc9c7b on 7.x-1.x authored by Grimreaper 
    Issue #2331517 by Grimreaper, johan.gant: Added Protected node doesn't...
izus’s picture

izus commented
Status: Needs review » Fixed

Hi,
Thanks both for comments and code.
I merged #10 but here are some extra notices, i'm not sure if they are all relevant.

  1. +++ b/tests/protected_node.view_mode.test
@@ -0,0 +1,107 @@
+   * Test that a promoted protected node can be seen on the front page

    We may want to be precise here : we are testing the /node page. 

  2. +++ b/tests/protected_node.view_mode.test
@@ -0,0 +1,107 @@
+    $this->drupalGet('');

    i guess this is more precise
    $this->drupalGet('node');

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hi,

Thanks for merging.

If you want to modify the code to be more precise, ok, I see no problem.

Status: Fixed » Closed (fixed)

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