Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Activation Code Expiry UI #151

Open
simoneeconomo opened this issue May 27, 2011 · 20 comments
Open

Activation Code Expiry UI #151

simoneeconomo opened this issue May 27, 2011 · 20 comments

Comments

@simoneeconomo
Copy link
Contributor

Is it necessary to have words like "hour" or "hours" (or "minutes" and so on...) inside the text input? What happens if someone accidentally writes "48 hour" instead of "48 hours"? I'm just not sure that is the more robust way to customize the expiry. Any comments?

@brendo
Copy link
Member

brendo commented May 28, 2011

I disagree and think it is the most robust way as it gives the developer complete control. Having some set values will always come back to a situation that requires something unique, which would require changing of the extension code.
Given how often this setting has to be set, and the fact its done by a dev and not an author, I'm happy to leave it as.

@simoneeconomo
Copy link
Contributor Author

Having some set values will always come back to a situation that requires something unique, which would require changing of the extension code.

There are many other ways to ensure unique values, without relying on words that: a) could be mistyped, thus affecting the expected behaviour; b) will require translation in other languages, so to conform with the rest of the backend.

Given how often this setting has to be set, and the fact its done by a dev and not an author

Authors can mistype words too. ;) Moreover, in order for a system to work predictably, users should be able to foresee what happens if they break the rules. In fact:

What happens if someone accidentally writes "48 hour" instead of "48 hours"?

I'm not able to answer to this question. I can imagine "hour" won't be recognised as it's related to the value "1", but then which is the default value? Will an error be thrown upon saving instead? Looks like this extension still needs to explain a lot that should be clear by using its UI.

@brendo
Copy link
Member

brendo commented May 28, 2011

There are many other ways to ensure unique values

Oh, I didn't mean unique as in only one, I meant if we just make a select box and have 1 hour, 2 hours, 1 day etc, there will be a use case where someone needs to have the code expire in 6 hours. This was a complaint from the first Members extension that the code was locked to 2 weeks expiry (and it often comes up now about the Cookie expiry too)

What happens if someone accidentally writes "48 hour" instead of "48 hours"?

The same result, strtotime returns the correct result if a user misses the 's'.

Looks like this extension still needs to explain a lot that should be clear by using its UI.

I thought the current approach was consistent with Symphony's UX. Open to your ideas though

@simoneeconomo
Copy link
Contributor Author

An idea could be an input field for numeric values plus a selectbox with the following options: "minutes / hours / days / etc." This should ensure the same flexibility as the current approach, but avoids the confusion of having an input field that forces one to adhere to a particular syntax.

brendo pushed a commit that referenced this issue May 28, 2011
@simoneeconomo
Copy link
Contributor Author

Any news on this? While I know it's too late for Members 1.0 I'd like to discuss different approaches (like mine) for Members 1.1.

@brendo
Copy link
Member

brendo commented May 30, 2011

IMO it's a downgrade in functionality, but sure, we can discuss and gauge feedback from the 1.0 release.

@simoneeconomo
Copy link
Contributor Author

Just to clarify: there's no downgrade in functionality as the flexibility remains the same. The only differences between the current approach (taglist-like) and the one I proposed (input field + selectbox) are:

  • You don't have to type "hours" or "minutes" or "days" etc., as these values are available in the selectbox.
  • The interface becomes translatable, while the current approach makes it difficult to translate keywords like "hours" etc.
  • Users are less prone to errors, as they only need to type numbers (thus, no typos)

@allen
Copy link

allen commented May 30, 2011

Simone, it’s evident that you feel very strongly about this. If you are happy enough to make the change, I’m sure Brendan will be more than happy to merge the change in when the extension updates next.

@cz
Copy link
Contributor

cz commented May 30, 2011

What happens if you need to mix days, hours, and minutes? Like if you want to set it to 2 days, 6 hours, and 45 minutes?

@simoneeconomo
Copy link
Contributor Author

Then we need a better alternative because mine didn't take mixes into account. The underlying problem still remains, though.

@simoneeconomo
Copy link
Contributor Author

The underlying problem still remains, though.

As an example, I accidentally wrote "24 hours 1 hour 45 minutes" (I thought I typed "24 days") and no errors were thrown. What happened? Which is the computed value? 25 hours, 1 hour? And so on...

@cz
Copy link
Contributor

cz commented May 30, 2011

How about a text input with autocomplete for keywords?
On May 30, 2011 3:52 PM, "eKoeS" <
[email protected]>
wrote:

The underlying problem still remains, though.

As an example, I accidentally wrote "24 hours 1 hour 45 minutes" and no
errors were thrown. What happened? Which is the computed value? 25 hours, 1
hour? And so on...

Reply to this email directly or view it on GitHub:
#151 (comment)

@allen
Copy link

allen commented May 30, 2011

I think we're over engineering the problem here.

If we want to eliminate potential user error and keep a simple enough interface, then we just need to maintain a single unit of time measurement, rather than supporting a range.

Bear in mind that 1 day == 24 hours == 1440 minutes. The lower the denominator, the more precise you can make your measurement.

Next we look at the actual purpose of the field. We're talking about activation expiry time. In my opinion, the most common range should fall between an hour to a day. Any less than an hour, then you're not giving enough time for the user to respond and take action. Any more than a day then you're defeating the purpose of having a code expiry to combat brute-force exploits.

If a user want values outside of the bell curve (80/20 philosophy), it's still possible. 30 minutes can be represented by 0.5 hours. A week is 168 hours.

@simoneeconomo
Copy link
Contributor Author

Thanks for your replies.

we look at the actual purpose of the field. We're talking about activation expiry time. In my opinion, the most common range should fall between an hour to a day. Any less than an hour, then you're not giving enough time for the user to respond and take action. Any more than a day then you're defeating the purpose of having a code expiry to combat brute-force exploits.

I admit I was a bit afraid of talking about real-life values 'cause someone could argue that it's not the Symphony way, but what Allen says is exactly what I thought. Better than an input plus a selectbox is an input and a single unit of time. Using hours means that minutes are difficult to target (30 minutes = 0.5 hours, but 20 minutes = 0.333 hours!). At the same time this is also educative: bizarre values other than "30 mins" shouldn't be encouraged.

On the other hand if we want more flexibility, then using minutes is the right choice. We're going to get really high values (e.g. 1440 minutes for 1 day, as Allen said), but it's a good compromise in my opinion.

@brendo
Copy link
Member

brendo commented May 30, 2011

If we look at the Dynamic XML DS and the Youtube/Vimeo fields, they have expiry times which are all reflected as a input field for the number of minutes, perhaps this should be considered?

@allen
Copy link

allen commented May 31, 2011

Okay, I'm good with that. I'd vote for consistency in this instance and go with 'minutes' as the standard unit.

@simoneeconomo
Copy link
Contributor Author

Brendan, if you are too busy I can submit a pull request with the mentioned changes. Just let me know.

@brendo
Copy link
Member

brendo commented May 31, 2011

Go for it, reckon you can get them in the next 12 hours?

We can push back 1.0 for a day or so while @michael-e and @hypnosis update the translation to include the copy changes.

There will need to be updater logic to convert the current expiry text to minutes, (although I think the database should still store it as '30 minutes' rather than just '30'

@simoneeconomo
Copy link
Contributor Author

Yikes, I don't think I have enough time today. If there's time to include this change in Members 1.0, then I think it's better if you make these changes yourself. Otherwise, if this stuff goes in Members 1.1, I can save you some time and send a patch.

@simoneeconomo
Copy link
Contributor Author

Okay, I think I've got almost everything working, except for the update script that doesn't seem to be executed. Shouldn't the following condition suffice?

Edit: never mind, shame on me!

simoneeconomo added a commit to symphonycms-it/members that referenced this issue Jun 9, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants