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

Smart waiver #247

Merged
merged 4 commits into from
Mar 28, 2017
Merged

Smart waiver #247

merged 4 commits into from
Mar 28, 2017

Conversation

sclark
Copy link
Member

@sclark sclark commented Mar 24, 2017

Prevent users from cheating or skipping the safety video

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 15.407% when pulling b44e8b8 on smart-waiver into a5b7e67 on new-features.

@pkoenig10
Copy link
Member

pkoenig10 commented Mar 24, 2017

FYI, better to rebase on top of your develop branch rather than merging. That way the commit history doesn't get cluttered with merge commits from feature branches. This is something that can easily lead to regressions.

@pkoenig10
Copy link
Member

This is a bit of a hack. I'm fine with this fix for now because you need to get something that works for build week. But we should track this issue so we can implement a proper fix in the future.

<!-- This file lives in public/cheating.html -->
<div class="dialog">
<h1>You tried to do something suspicious</h1>
<p>Please go back and try again, this time without trying to cheat the system...we'll be keeping an eye on you!</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the error message to indicate why the user is seeing this page (skipping the waiver video). And the cute error message isn't really necessary, its better for users to use something clear and straightforward.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

@sclark
Copy link
Member Author

sclark commented Mar 24, 2017

I like the merge commits, but seems like we rebase in this repo, so I will also rebase in the future

@sclark
Copy link
Member Author

sclark commented Mar 24, 2017

This may be hacky and should probably be made configurable, but almost anything on the client side can be manipulated, so this was the quickest, most sane way of doing things. Will keep issue #249 open for future consideration.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 15.407% when pulling 9a34d15 on smart-waiver into a5b7e67 on new-features.

@sclark
Copy link
Member Author

sclark commented Mar 24, 2017

@pkoenig10 when pulling in updates from another branch, I understand the rebase and merge idea. When merging pull requests, is there a tradition of rebasing and merging in this repository?

@sclark
Copy link
Member Author

sclark commented Mar 24, 2017

If not, I prefer to make a merge commit so we can see that the code came from a different feature branch, but if there is a tradition, I wouldn't want to change it.

<h1>You tried to do something suspicious</h1>
<p>Please go back and try again, this time without trying to cheat the system...we'll be keeping an eye on you!</p>
<h1>You tried to do skip the saftey videos</h1>
<p>Please watch the entire saftey video before completing the waiver!</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Safety" is misspelled on both lines.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting sloppy...Fixing this now.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 15.407% when pulling 0cc0c1d on smart-waiver into a5b7e67 on new-features.

@sclark sclark merged commit f5d6be7 into new-features Mar 28, 2017
@sclark sclark deleted the smart-waiver branch March 28, 2017 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants