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

Hide value for Password fields. #39

Merged
merged 4 commits into from
Mar 7, 2019

Conversation

jhtimmins
Copy link
Contributor

@jhtimmins jhtimmins commented Mar 6, 2019

Since the input field has this format:

<input type="password" id="form-contact-e" name="e" required value="">

I added two tests, one for the beginning of the string and one for the end, to get both type and value. How would you feel about moving the value tag to the front of the input so this can be consolidated into one test? This will also make it less brittle against other fields with empty string values.

Closes #30

@tomchristie
Copy link
Member

Let's instead have the password test case be on its own, maybe?...

Something like this perhaps?...

def test_password_rendering()
    class PasswordForm(typesystem.Schema):
        password = typesystem.String(format="password")
    form = forms.Form(PasswordForm, values={'password': 'secret'})
    html = str(form)
    assert 'secret' not in html

@tomchristie
Copy link
Member

Note that the existing test as it stands is testing the password value, for an unpopulated form. Need to be testing a form with values= set to some input data.

@tomchristie
Copy link
Member

Run scripts/lint to resolve any linting issues. https://travis-ci.org/encode/typesystem/jobs/502767667#L322

@jhtimmins
Copy link
Contributor Author

jhtimmins commented Mar 6, 2019 via email

@jhtimmins
Copy link
Contributor Author

Oops, git mistake. I removed the old test but it didn't get committed. One sec.

@tomchristie
Copy link
Member

I've added Closes #30 to the description here, so it'll auto-close once this is merged.

@tomchristie
Copy link
Member

Ace, thanks!

@tomchristie tomchristie merged commit 527ee72 into encode:master Mar 7, 2019
@tomchristie tomchristie mentioned this pull request Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants