Git Merge Request #355: Phone Verification Attemp Limit (merged)

Merging...

Merged

Something went wrong. Please, merge manually

Checking if merge is possible...

Something went wrong. Please, merge manually

Guillermo Cruz wants to merge 3 commits from /u/guillermocruz/allura/ to master, 2021-04-16

Set a limit for phone verification attempts

Commit Date  
[e04d6b] (gc/phone-rate-limit) by Guillermo Cruz Guillermo Cruz

removed unnecessary config in tests, set a default value in case phone.attempts_limit is not in config file and converted setting to integer

 2021-04-16 19:19:16 Tree
[65fcca] by Guillermo Cruz Guillermo Cruz

Added another test and fixed attempt message

 2021-04-15 21:24:56 Tree
[53c26d] by Guillermo Cruz Guillermo Cruz

Set a limit for phone verification attempts

 2021-04-15 21:06:48 Tree

Discussion

  • Dave Brondsema

    Dave Brondsema - 2021-04-16
    • can you provide a default value in the code like attempt_limit = config.get('phone.attempts_limit', 5) so anyone who doesn't have it set yet will have it work ok? Otherwise they'll get errors. Also I think then it won't have to be specified in all the different phone tests (just the new one)
    • until the next Allura release, we want to support python 2 still, so we shouldn't use f-strings
    • when I tried a real test I set phone.attempts_limit = 1 in the .ini file to encounter the error real soon, but it didn't happen. I think because the config file is all text strings. So you need to convert the limit to int before comparing to the user count. It'd be good to have the test reflect this too like 'phone.attempts_limit': '5' (string)
     

  • Guillermo Cruz - 2021-04-16
    • Removed unnecessary config settings I added in tests.
    • Set a default value in case phone.attempts_limit is not present in config file.
    • Added conversion to integer in case is set as a string.
     

  • Dave Brondsema

    Dave Brondsema - 2021-04-16
    • Status: open --> merged
     

Log in to post a comment.