Allow display name editing for all but picture-only accounts #17637
Conversation
ewjordan
requested review from
Bjvanminnen and
islemaster
and removed request for
islemaster and
Bjvanminnen
|
[Old - confirmed this is the correct behavior] Hold off on review, I re-read the spec and misinterpreted it slightly. |
dashboard/app/models/user.rb
Outdated
| return false unless teacher_managed_account? | ||
| all_pictures = sections_as_student.all? {|section| section.login_type == Section::LOGIN_TYPE_PICTURE} | ||
| any_pictures = sections_as_student.any? {|section| section.login_type == Section::LOGIN_TYPE_PICTURE} | ||
| all_pictures && any_pictures |
There was a problem hiding this comment.
Is the any_pictures only necessary for the case where we have 0 sections? Could this instead just be all_pictures && sections_as_student.length > 0?
There was a problem hiding this comment.
Yes, that should be fine. At this point it might be irrelevant, because I realized that the spec was slightly ambiguous, and I followed the first part of it ("just in picture password sections"), which conflicts with the second part and the examples given, so I'm clarifying the desired behavior now.
There was a problem hiding this comment.
Ok, clarified, and we're good to go with the current behavior, but I will make this simplification.
| # True if the account is teacher-managed, is in at least one picture section, and | ||
| # is not in any non-picture sections | ||
| def secret_picture_account_only? | ||
| return false unless teacher_managed_account? |
There was a problem hiding this comment.
might be even easier to follow if we just make the first line
return false unless teacher_managed_account? && sections_as_student.present?
another simple way to express this could be
sections_as_student.map(&:login_type).uniq == [Section::LOGIN_TYPE_PICTURE]
Really any of these is probably fine.
There was a problem hiding this comment.
.present? doesn't seem to work on arrays (we want to check that the array has elements, and we have a style rule preventing checking the length).
The second option is interesting, though, I'll see if that works right.
| def secret_picture_account_only? | ||
| return false unless teacher_managed_account? | ||
| any_sections = !sections_as_student.empty? | ||
| any_sections && sections_as_student.all? {|section| section.login_type == Section::LOGIN_TYPE_PICTURE} |
There was a problem hiding this comment.
If I may overthink this for a moment. Slightly fewer characters?
return false unless teacher_managed_account?
return false if sections_as_student.empty?
sections_as_student.all? {|section| section.login_type == Section::LOGIN_TYPE_PICTURE}Also, we're not dipping into the db twice by including sections_as_student more than once here, right?
Update: We're going to go with the logic implied by the first part of the original spec, that we only restrict editing display names if a user is only in picture sections (so in the example below, where a user is in 1 picture and 3 word sections, they would be allowed to edit their display name).
Original spec:
We originally took away this ability for all teacher-managed accounts. But apparently we only want it for students that are just in picture password sections. So now we want the logic to be if students are in no picture sections, they can edit their display name.
Examples:
If user is in 1 picture section and 3 word sections, they should NOT be able to edit their name.
If user is in 0 picture sections and in 1 word section, they should be able to edit their name.