-
Notifications
You must be signed in to change notification settings - Fork 37
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
REDBOX 257 - User demographic data #583
Conversation
34fd991
to
59e4d4b
Compare
1b0bd36
to
5f41840
Compare
username = None | ||
verified = models.BooleanField(default=False, blank=True, null=True) | ||
invited_at = models.DateTimeField(default=None, blank=True, null=True) | ||
invite_accepted_at = models.DateTimeField(default=None, blank=True, null=True) | ||
last_token_sent_at = models.DateTimeField(editable=False, blank=True, null=True) | ||
password = models.CharField("password", max_length=128, blank=True, null=True) | ||
grade = models.CharField(null=True, max_length=3, choices=UserGrade) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we need blank=True
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think null=True
is enough for all of them TBH. I don't think they should ever be blank.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
models side LGTM, needs some input from @rachaelcodes or @KevinEtchells for the FE side
widgets: ClassVar[Mapping[str, forms.Widget]] = { | ||
"grade": forms.Select(attrs={"class": "govuk-select"}), | ||
"profession": forms.Select(attrs={"class": "govuk-select"}), | ||
"business_unit": forms.Select(attrs={"class": "govuk-select"}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we sort these alphabetically for user ease?
django_app/tests/conftest.py
Outdated
@@ -53,8 +60,15 @@ def jemima_puddleduck(): | |||
|
|||
|
|||
@pytest.fixture() | |||
def mrs_tiggywinkle(): | |||
return User.objects.create_user(email="[email protected]") | |||
def mrs_tiggywinkle(business_unit: BusinessUnit) -> User: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth renaming this to something like user_with_demographic_info
for test readability?
{% endfor %} | ||
<div class="govuk-button-group"> | ||
{{ govukButton(text="Update") }} | ||
{{ govukButton(text="Skip", href=url('documents')) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brunns everything looks good to me, except I don't think it's a good idea to have 2 primary action buttons. Are you happy for me to make the "Skip" button grey?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go for it - or just suggest it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just pushed the change :-)
93150f4
to
4d3f294
Compare
Context
Collect demographic data on our users.
Changes proposed in this pull request
Add grade, business unit and profession to the data model.
Allow upload of business unit data as CSV files via the admin interface.
If a user hasn't given us this data, pass them via a form on sign on.
Guidance to review
Relevant links
Things to check