Fix max attempts message - #24
Merged
Merged
Conversation
bradenmacdonald
force-pushed
the
fix-max-attempts-message
branch
3 times, most recently
from
April 29, 2015 04:08
62d1f8e to
35fa0a3
Compare
bradenmacdonald
force-pushed
the
fix-max-attempts-message
branch
from
April 29, 2015 04:53
21b7eed to
b1db769
Compare
Member
Author
|
Kelketek: I figured out how to fix the Travis build!! I do not understand why it was suddenly broken, but bumping the Firefox version has fixed it :) Also, this is ready for your review. |
Member
|
@bradenmacdonald Thank you for the changes on this. The extra documentation makes things a great deal more clear. I remember trying to puzzle through what all the messages were for and getting frustrated, so this will certainly help. This works for me, and the code seems clean enough, so I'm giving it a 👍 |
bradenmacdonald
added a commit
that referenced
this pull request
Apr 30, 2015
Fix max attempts message
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes OC-667 (upstream bug is a comment in SOL-423): The "Message (max # attempts)" component was not working.
Explanation:
get_message()was called beforenum_attemptswas incremented, so themax_attempts_reachedmessage would never be used, sincenum_attemptswas always less thanmax_attemptsat that point.Notes:
I wanted to just do a simple fix, which involves moving the
get_message()method to after wherenum_attemptsis updated, but the implications of that move were not at all clear due to the complexity of the "submit" handler, and uncertainty about when the various messages are even supposed to be display. So I added a bunch of new tests and documentation to clarify the expected behavior of each type of message (I hope I got it right), and I also simplified the "submit" handler (in Python at least) while I was at it, in order to make the code easier to understand.Details:
get_message()submit()handler somewhatattemptedwhich was not usedConfusion: