Problem/motivation:
I have the requirement of showing different text on the enter password page per content type.

Tokens are very nice, but they don't help in this use case. Currently this is only a global setting, and I think it would be good to add it to the content type settings.

I don't know if anybody needs any of the other fields also:

  • Password page title
  • Password page general information (is this even needed?)
  • Password field label

Perhaps a more complete configuration reorganization is needed in a 7.x-2.x or 8.x branch, whichever is next :)

CommentFileSizeAuthor
#3 protected_node-content_type_description-2535490-3.patch10.15 KBdigitgopher
#1 protected_node-content_type_description-2535490-1.patch3.19 KBdigitgopher

Comments

digitgopher’s picture

digitgopher commented
Status: Active » Needs review
StatusFileSize
new protected_node-content_type_description-2535490-1.patch3.19 KB
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Thanks for your patch. Here is my review.

  1. +++ b/protected_node.fork.inc
@@ -97,7 +97,10 @@ function protected_node_enter_any_password() {
+  if (!$description) {

+++ b/protected_node.redirect.inc
@@ -75,9 +75,12 @@ function protected_node_enterpassword() {
+  if (!$description) {

    I think it should be if ($description != '') {

    Because if the variable does not exist, it will be instanciated to ''. 

  2. +++ b/protected_node.settings.inc
@@ -913,6 +913,13 @@ function protected_node_node_type_form_alter(&$form) {
+    '#default_value' => variable_get('protected_node_description_' . $form['#node_type']->type, ''),

    Should it be $form['protected_node']['protected_node_description_' . $form['#node_type']->type]?

When it will be ok I will apply the patch to test.

@Izus: do you think we need an automated test for this new feature? I am in favor of an automated test just in case.

digitgopher’s picture

digitgopher commented
StatusFileSize
new protected_node-content_type_description-2535490-3.patch10.15 KB

Grimreaper,
Thanks for the review.

As for 1), do you want to change the existing checks as well?

As for 2), what is the reason you suggest this? I was following the convention I saw there, but may have missed something else.

I didn't change these yet, I will let you do that or explain more. However I did make tests :)

digitgopher’s picture

digitgopher commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 3: protected_node-content_type_description-2535490-3.patch, failed testing.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs work » Fixed

Hello,

Forget about my previous review, it was bad with the web browser.

I made some adjustments for example the test show an extra space added when showing the node title.

This is now merged. Thanks.

grimreaper’s picture

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

I just remember that the documentation needs to be updated: https://www.drupal.org/node/2176651

digitgopher’s picture

digitgopher commented
Status: Active » Fixed

Updated the Per Node Type Settings documentation page and the Global Settings documentation page for this feature.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Good for me. Thanks.

Status: Fixed » Closed (fixed)

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