Skip to content

Conversation

@dankrause
Copy link
Contributor

Implements #16 and fixes #18

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be:

permissions = self.permissions.get(method, [])

@budowski
Copy link
Owner

Awesome implementation dude! :-)
I've wrote down some notes on the commit...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add support for a singular permission instead of just a list? e.g.

if not isinstance(permissions, list):
    permissions = [permissions]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Adding this.

@budowski
Copy link
Owner

Also - we need to verify these changes don't mess up the users.py module (that it'll still operate normally).

@dankrause
Copy link
Contributor Author

I have tested with a trivial application that uses the users module, and it didn't break anything. That's not saying much though. I think we really need to address that test suite next.

@dankrause
Copy link
Contributor Author

This should address the temp code that was left behind, as well as allow single permission objects instead of requiring lists.

@dankrause
Copy link
Contributor Author

Looks like I totally neglected to implement this in users.py - I'll get that soon.

@budowski
Copy link
Owner

Another small comment - if doing a multi-PUT - there is no validation that the user actually owns those updated models (this check should somehow be done by the Permission instance)

@dankrause
Copy link
Contributor Author

Converting the user handler to use the permission system is going to take some time. I'm going to get some of it done tonight, but it may be a couple more days before I find the time to finish it.

@dankrause
Copy link
Contributor Author

This is going to be more work than I thought. The users handler embeds other routes within it, such as "me", "login", "verify", and "reset". To avoid lots of special cases, I'm going to break them out into separate routes and stuff them in a multi-route. After that is done, I may be able to use our regular rest handler for users, and get rid of A lot of duplicate code.

@budowski
Copy link
Owner

Hmmm, yeah - sorry for the work load :-/
Tell me if you want a hand in this

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.

Doing a POST with an already existing id will overwrite the old instance

2 participants