I noticed some variables were removed here: http://cgit.drupalcode.org/protected_node/commit/?id=c308e , but I wasn't sure what replaced them. How is the random password being created now?

And I wasn't sure what this code in protected_node.module is doing...?

if ($_protected_node_emails && $missing_password && variable_get('protected_node_random_password', FALSE)) {
          // Not missing anymore.
          $missing_password = FALSE;
          drupal_set_message('A random password was generated in order to send the email about this page. Remember that changing the password will prevent users you just emailed from accessing this page.', 'warning');
     }

I tried this configuration: "Enable protected node email support" checked and "Generate a random password if necessary" checked. Content type set to "Always protected", no global passwords. Try to create a new node without setting a password and the validation fails with the error message "To protect this page, please enter a password." Shouldn't it also say something like "...or enter an email address so someone is notified of the password"? Additionally, what do you think of the option to allow a random password to be generated without marrying that setting to the email feature? I guess in this case you could only get to the password through rules?

Sorry for all the questions, I'm trying to understand what the module is doing. Thanks!

CommentFileSizeAuthor
#9 protected_node-email_help-2533306-9.patch6.12 KBdigitgopher
#3 protected_node-random-pw-2533306-3.patch6.91 KBdigitgopher

Comments

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Category: Support request » Bug report

Hello digitgopher,

I think you just found a bug that I introduced with my commit.

OK, now I see why, it "worked" before, because in:

function _protected_node_node_create_or_update($node) {
  // Ugly but we want to keep some variables between the validation and
  // insert/update.
  global $_protected_node_emails;
  global $_protected_node_random_passwd;

So my IDE saw the variables in protected_node_node_validate() unused but they are used later. That's because of this kind of ***** that I want to start a 7.x-2.x branch which will consist of an almost complete rewrite of the module.

Anyway, before I revert my commit, your idea to separate the random generation feature from the email feature is interesting, but it implies that even the node author will not know the password, it is ok?

So if you want to provide a patch, I will be happy to review it. And also if you could write a test about it that will be cool, and a test about the random password generated with rules it will also be cool.

Thanks.

izus’s picture

izus commented

yes, as this is a bug, we should have a test to never see it happen again :)

digitgopher’s picture

digitgopher commented
Title: Handling of the random password generation feature » Random password feature doesn't save a password
Status: Active » Needs review
StatusFileSize
new protected_node-random-pw-2533306-3.patch6.91 KB

I decided not to try separating these features right now :) #2536710: Separate email feature from random password feature

What I did do:

  • Added the variables back, including as global in the validate function for it to work.
  • Added #states to the email config fields (don't need to see them if the email feature is not turned on)
  • Added a test for the random pw feature

Thanks, and let me know what you think.

  • Grimreaper committed 3882c3c on 7.x-1.x authored by digitgopher 
    Issue #2533306 by digitgopher, Grimreaper, izus: Random password feature...
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Category: Bug report » Task

Hello,

Thanks for the patch, this is now merged. I have done a little further in the test of the random password.

In the #states, there was an indentation problem.

I didn't commited the improvement of the email features settings form. I love the #states, I thought of that several times but always forgot or busy with something else to made it, so thank you very much.

But I wonder why changing the description with t('Check this box to enable the protected node email support (NOTE: Random password support available as part of this feature).') as there is a checkbox dedicated to the that at the end of the fieldset. Is it to make it clear? If yes, I wonder if this lack of clearity should be fixed somewhere else like the textarea where you enter the email adresses on the node edit form.

I wait your responses (digitgopher, izus) before merging that part.

Changing to task as the bug is solved.

digitgopher’s picture

digitgopher commented

Ah. I see the spacing error now. Sorry about that. Thanks for the extra check in the test case, that is good.

I added that "Random password support available as part of this feature" text in case someone is searching for that feature but can't find it because it is now hidden when email is not selected. I do not know what you mean by clarifying on the node edit form email text area, as that textarea is only available when the email config box is selected.

Status: Needs review » Needs work

The last submitted patch, 3: protected_node-random-pw-2533306-3.patch, failed testing.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Ah yes, because it is hidden by default, ok. Good for me.

I thought about the email textarea because I think not every contributors on website have access to the protected node settings :). And on the node edit form there is no mention to a generated password.

digitgopher’s picture

digitgopher commented
Status: Needs work » Needs review
StatusFileSize
new protected_node-email_help-2533306-9.patch6.12 KB

Alright, here is an updated patch for the email settings/text improvements. At this point we are kind of hijacking this thread though...I probably should have started a new one...

  1. States with indentation
  2. Left the note on the master email setting as we discussed above, with a slight change in wording.
  3. After thinking about it, I agree there needs to be a mention of the random pw on the node edit form. I conditionally added a line of text to explain this to the user.
  4. Made another change to the email help text. It was assuming that the email would include 'a link back to this page and optionally the password', and this isn't necessarily true because this can be configured. It has to be up to the site admin to let their node authors know what info the email includes.

What do you think?

Status: Needs review » Needs work

The last submitted patch, 9: protected_node-email_help-2533306-9.patch, failed testing.

grimreaper’s picture

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

Hello,

Thanks for the patch this is merged now.

2 things to notice:
- I renamed the variable in $random_password_help_text, no camelcase in D7
- don't forget space around if statements: if (variable_get('protected_node_random_password', FALSE)) {

I noticed that the email fieldset didn't have title. I added one.

Status: Fixed » Closed (fixed)

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