Closed robbiejackson closed 9 years ago
Re labels for edit being New Password, the problem is that because of how Nik has coded the solution for setting the Required indicator on labels, this will show up as the New Password being required. Which it isn't. I can't think of any sensible way of changing the functionality so that the Required doesn't appear in the label for this case. If we could do that, then New Password would be better, but if we can't, I think it's better just sticking with Password, and the placeholder text there makes it appear that it's already present. Also I was wondering about getting the user to reenter his password for some of these operations - particularly the delete. Let's discuss in the scrum.
Totally agree with this:
Also I was wondering about getting the user to reenter his password for some of these operations - particularly the delete.
We should double check if the user is who it says to be. Also many web sites do this doble check to change passwords or do another critical operations.
OK, this has now got fixed all the issues from previous code reviews. I haven't included the aspect of forcing the admin user to re-enter their password when they're executing certain actions - this is dependent upon what Christian agrees with the clients. The code here includes the code changes for OSRA408 because the branch was taken off that, but a summary of the files different between OSRA408 and this one OSRA409 is below: M app/controllers/hq/admin_users_controller.rb A app/views/hq/admin_users/edit.html.erb M app/views/hq/admin_users/index.html.erb M spec/controllers/hq/admin_users_controller_spec.rb A spec/features/hq/edit_admin_users_spec.rb A spec/views/hq/admin_users/edit_spec.rb M spec/views/hq/admin_users/index_spec.rb
What the client decided about this functionality?
@gmunumel : we'll have a meeting with the client once we finish rails part. I expect in max 2 weeks!?
@robbiejackson: can you please rebase this so commits: AdminUser #new and #create done, plus small fixes to #index
will dissapear from this PR.
Thanks
This has now been rebased after the merge of OSRA408 (new/create). New screenshot too:
:+1: