-
Notifications
You must be signed in to change notification settings - Fork 2
Petition detail view and CAS login #42
base: master
Are you sure you want to change the base?
Conversation
jlyon1
left a comment
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.
Overall, looks good 💯. A few minor changes around the relationship between petitions and signatures, as well as thoughts on authentication.
On the detail view: Looks good, however it is a little small on mobile, due to the display not collapsing or resizing text. This does appear to be an issue on the site as a whole. Example on iphone X:

Other points that should be fixed at some point but may be outside the scope of this PR:
- The ID can be set and can be negative, negative IDs cause an error on the site
- There is a field labeled '300' on the admin interface which actually represents the number of signatures
- Admins can assign signature to petitions, this should not be allowed.
|
|
||
|
|
||
| class StudentCASBackend(CASBackend): | ||
| def user_can_authenticate(self, 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.
A debug mode would be helpful here so that we can test with different users or bypass cas entirely
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 searched around, and there doesn't seem like an easy way to do this. For now, I added that if debug mode is true, then the user is set to a superuser, which at least avoids accessing the database to set permissions.
I think what you suggested is a good idea, but I think we can create an issue for this, as opposed to including it in this PR.
Summary of major changes:
django-ng-cas) and WTG's identity service.Minor changes:
xenialdistribution. Travis builds started failing, and this change fixed that.Closes #2 and #31.
This is a preliminary design for the detail view, so feedback on the style and how it can be improved would be great.