Allow vetoing forgot password requests #2893

Merged
merged 1 commit into from Mar 2, 2014

Conversation

Projects
None yet
2 participants
@chillu
Member

chillu commented Feb 24, 2014

No description provided.

@simonwelsh

This comment has been minimized.

Show comment Hide comment
@simonwelsh

simonwelsh Feb 24, 2014

Contributor

Docs? Tests?

Contributor

simonwelsh commented Feb 24, 2014

Docs? Tests?

@chillu chillu referenced this pull request in BetterBrief/silverstripe-opauth Feb 24, 2014

Merged

Allow disabling of "forgot password" feature #9

@simonwelsh

This comment has been minimized.

Show comment Hide comment
@simonwelsh

simonwelsh Feb 24, 2014

Contributor

The docs should cover how to set the error message the user gets too.

Contributor

simonwelsh commented Feb 24, 2014

The docs should cover how to set the error message the user gets too.

@chillu

This comment has been minimized.

Show comment Hide comment
@chillu

chillu Feb 24, 2014

Member

Alright, added docs on class PHPDoc. I don't think error messages need to be documented, that's just normal form and extension behaviour, and one specific use case.

Apart from the (admittedly critical) Member extension points, this would be the first one with tests, while there's a few dozen untested ones in the system. Given how straightforward they are, that the underlying Object->extend() is already tested, and the difficulty/verbosity of actually writing such a test, I don't think they're needed here.

Member

chillu commented Feb 24, 2014

Alright, added docs on class PHPDoc. I don't think error messages need to be documented, that's just normal form and extension behaviour, and one specific use case.

Apart from the (admittedly critical) Member extension points, this would be the first one with tests, while there's a few dozen untested ones in the system. Given how straightforward they are, that the underlying Object->extend() is already tested, and the difficulty/verbosity of actually writing such a test, I don't think they're needed here.

@simonwelsh

This comment has been minimized.

Show comment Hide comment
@simonwelsh

simonwelsh Feb 24, 2014

Contributor

A validation failure through this hook behaves differently from an incorrect password or email address, so I think it should be made clear that an error message should be set.

Also, I've already come up with a case where the if statement incorrectly fails. Two extensions, first one returns 0 (or some other false-y, non-null value) second returns false. The min() returns the 0.

Contributor

simonwelsh commented Feb 24, 2014

A validation failure through this hook behaves differently from an incorrect password or email address, so I think it should be made clear that an error message should be set.

Also, I've already come up with a case where the if statement incorrectly fails. Two extensions, first one returns 0 (or some other false-y, non-null value) second returns false. The min() returns the 0.

@simonwelsh

This comment has been minimized.

Show comment Hide comment
@simonwelsh

simonwelsh Feb 24, 2014

Contributor

Also, in the pull request for opauth, you mention it'll require master yet this is against 3.1. Which branch did you mean for this to get in to?

Contributor

simonwelsh commented Feb 24, 2014

Also, in the pull request for opauth, you mention it'll require master yet this is against 3.1. Which branch did you mean for this to get in to?

@chillu

This comment has been minimized.

Show comment Hide comment
@chillu

chillu Feb 25, 2014

Member

Thanks for the feedback, Simon!

I've corrected the requirement to "requires 3.1.4 or newer", in anticipation of the next release after the security-related 3.1.3. Wanted to avoid guessing releases by saying "requires 3.2", but you're right, its inaccurate.

Changed the min() logic to using in_array() in strict mode to distinguish between 0, null and false correctly.

Added method-level docs about the impact of the extension point, is that what you had in mind?

Member

chillu commented Feb 25, 2014

Thanks for the feedback, Simon!

I've corrected the requirement to "requires 3.1.4 or newer", in anticipation of the next release after the security-related 3.1.3. Wanted to avoid guessing releases by saying "requires 3.2", but you're right, its inaccurate.

Changed the min() logic to using in_array() in strict mode to distinguish between 0, null and false correctly.

Added method-level docs about the impact of the extension point, is that what you had in mind?

@simonwelsh

This comment has been minimized.

Show comment Hide comment
@simonwelsh

simonwelsh Feb 25, 2014

Contributor

Yup, something like that :)

I'm fine with this being merged once 3.1.3 is tagged.

Contributor

simonwelsh commented Feb 25, 2014

Yup, something like that :)

I'm fine with this being merged once 3.1.3 is tagged.

simonwelsh added a commit that referenced this pull request Mar 2, 2014

Merge pull request #2893 from silverstripe-iterators/pulls/forgot-pas…
…sword-veto

Allow vetoing forgot password requests

@simonwelsh simonwelsh merged commit 41cdacb into silverstripe:3.1 Mar 2, 2014

1 check passed

default Scrutinizer: No new issues — Travis: Passed
Details

@chillu chillu deleted the silverstripe-iterators:pulls/forgot-password-veto branch Mar 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment