08 July 2015 - Fix multi-scope methods; add leave_moderator

[voussoir]

Changes

• Add support for multiple-scope methods by using lists for has_scope check
• Use above to fix add_wiki_ban and add_wiki_contributor
• Add [modcontributors, modwiki] relationship scope for above
• Add modcontributors relationship scope as seen in Fix OAuth scopes in _modify_relationship in internal.py #451
• Fix WikiPage.remove_editor raising an AssertionError when using OAuth
• Add refresh key and tests for modwiki
• Add refresh key and tests for modwiki+wikicontributors

Concerns

• You'll notice that test_scope_modwiki shows me first loading the wikiread scope. This is required to perform subreddit.get_wiki_page, else it 403s. I would consider this a bug / shortcoming, because it means we're implicitly adding a scope dependency that isn't required by reddit. I'm not sure why modwiki doesn't allow you to load the page in the first place.
• I'm debating whether I should add a leading underscore to leave_status. On one hand, the user shouldn't be calling it directly, they should use leave_moderator. On the other hand, this method has all of the docstring info, so I feel it shouldn't be privatized. I mean, privatizing doesn't stop them from reading it, it's just a formatting thing.

 

Notes

• The leave_moderator method had to go through a few evolutions to get to where it is now. Many of the reddit forms accept {'r': 'reddit_api_test'}, but leavemoderator and leavecontributor insist on requiring the subreddit's fullname. I tried everything to add this method to either the relationships section or the _methods section, but in the end I had to make it separate.

Critique.

[bboe]

Can you switch these so they remain in a sorted order?

[bboe]

I would consider this a bug / shortcoming, because it means we're implicitly adding a scope dependency that isn't required by reddit. I'm not sure why modwiki doesn't allow you to load the page in the first place.

I don't think we're adding any dependencies. Getting pages simply requires a separate scope.

Overall this looks great! Thanks as always for the effort.

[voussoir]

Can we make this private

Alright, no problem. Extra docstrings added.

 

I don't think we're adding any dependencies.

No, we certainly are. Here I am bypassing it:

Getting pages simply requires a different scope

This is true, but add_editor isn't supposed to get the page, it's supposed to add an editor -- no wikiread required for that. Personally I don't consider this the highest of priorities, but it's worth noting.

* I think that last part came out sounding a little sarcastic. Sorry about that, I mean well.

[bboe]

I see. So we need a lazy get_wiki_page method? Would that solve it?

[voussoir]

Yes, r.get_wiki_page will need to call the WikiPage constructor instead of request_json. Do you remember if there's any particular reason you used request_json for that? I wonder what kind of parameters can be used for getting a wik page.

Secondly, wiki pages will need to pass this hasattr check without causing a lazyload and receiving 403. This check was implemented before Redditor had the fastname attribute, so _fast_name originally would have been an indicator that it was a Subreddit object. I'm assuming this should be replaced with an isinstance. Otherwise we can give WikiPages a _case_name attribute, but that seems like a hack.

As far as I can tell, that will be all it takes for add_editor to pass.

[voussoir]

I'm very close to fixing the WikiPage scopes bug, but I've run into a problem.

I wanted to replace hasattr with isinstance(cls, Subreddit), but importing that creates a circular dependency (init -> decorators -> objects -> init.AuthenticatedReddit as AR).

The easy way out is to give WikiPage a _case_name so that it passes the hasattr, but that's pretty hackish. We could also make it so that this check is not performed when the requested scope is something like modwiki, which can never be applied to a Subreddit object, but I don't know if that'll open the door to outlining a bunch of "special cases" for each scope.

How would you solve this?

[bboe]

I think I tried the exact thing you did before and came up with the same error. To be honest the restrict_access decorator is very hacky, so a hacky solution on top of that isn't too bad.

What would probably be better is to rewrite it to not use a decorator, but instead do an access check at by calling some method at the beginning of the currently decorated methods. Alternatively when we move to OAuth only, we could dynamically only make the functions available when the set of credentials allow that method to be used.

[voussoir]

Oh, it looks like this works:

    if hasattr(cls, 'reddit_session'):
        # Defer access until necessary for RedditContentObject.
        # This is because scoped sessions may not require this
        # attribute to exist, thus it might not be set.
        from praw.objects import Subreddit
        subreddit = cls if isinstance(cls, Subreddit) else False

I don't usually use imports in places like this, but it's probably better than abusing the casename. Do you have anything against it? Otherwise I'm keeping it.

I wouldn't mind replacing restrict_access with smaller, more specialized checks that can make better choices. Maybe it makes too many assumptions.

[bboe]

If that works, then let's go with it for now.

[voussoir]

• r.get_wiki_page now uses the object constructor
• WikiPage is now Refreshable
• Added missing wikiedit restriction to edit_wiki_page
• restrict_access now uses isinstance instead of hasattr.
• Removed the now-unecessary wikiread login from test_scope_modwiki
• Fixed test_edit_wiki_page which relied on r.get_wiki_page in its older form.

I also discovered some very strange behavior in which reddit will 403 wiki pages through OAuth that you can access through Password Auth, even if you have all of the scopes enabled. This only happens when the subreddit has the wiki in "disabled" mode, which means only moderators can see it. As far as I can tell, this is not PRAW's fault.

[bboe]

I also discovered some very strange behavior in which reddit will 403 wiki pages through OAuth that you can access through Password Auth, even if you have all of the scopes enabled. This only happens when the subreddit has the wiki in "disabled" mode, which means only moderators can see it. As far as I can tell, this is not PRAW's fault.

Maybe you can file a bug with reddit on that one? Great work overall.

[bboe]

Should we make a ModSelfMixin to put these functions into?

[voussoir]

Any insight as to why removing this was necessary?

It points to add_editor with an inverse flag, similar to hide / unhide. If you stack a decorated method on top of another decorated method, you fail this check. All of the leave_moderator methods point to the single _leave_status which holds the decorator, for instance. This is another reason restrict_access will need a makeover when password auth leaves.

 

I don't know if there's any reason to support directly passing the fullname

The thought process there was that if you only pass in the subreddit name, then praw will have to perform a get_subreddit, and then fetch the fullname attribute. If the user knows the fullname they could skip that request.

Granted, the act of leaving moderator and contributor positions is probably infrequent enough that it's not very important to save those 2 seconds (1s when we go full oauth), but maybe somebody's time is _really_ precious. It's nice to have it done in the minimum number of steps.

I don't really mind removing that if it's an eyesore. You could bypass it by creating the Subreddit object manually and forcing the fullname to whatever you want.

 

• Added ModSelfMixin
• Added leave_moderator and leave_contributor to Subreddit._methods
• Changed _leave_status to accept the url parameter instead of status string parameter.
• leave_moderator and leave_contributor within test_scope_modself now use the subreddit and fullname parameters so they each get tested.
• Reinstated the subreddit.get_wiki_page line to ensure it's being tested.
• Added more assertions to test_scope_modself to make sure the user is no longer a moderator.

 

Hey, can we add some way to make objects.Refreshable.refresh bypass the cache and hard refresh the object? That would make the tests a lot easier so I don't have to get funky with letter casing. Just a boolean flag for hardreset would be great, but I'm not sure how I would implement it. I'm really bad at using r.evict.

It looks to me like you intended for this to happen at some point, because there's stuff about self.reddit_session._unique_count, but it's not highly used.

[bboe]

Refresh should always do a hard reset. Can you point out where it's not working? Take a look at its tests. I took a look at refresh implementation. It looks like Redditor and Subreddit refresh do not incorporate the unique value into their requests. Those will need to be updated for it to work properly on those objects.

Edit: Mobile was freaking out, sorry for the close and reopen.

All of the leave_moderator methods point to the single _leave_status which holds the decorator, for instance.

Oh right that makes sense!

The thought process there was that if you only pass in the subreddit name, then praw will have to perform a get_subreddit, and then fetch the fullname attribute. If the user knows the fullname they could skip that request.

Yes, I agree having some performance would be nice. However, I would assume that in most cases if they have the fullname, they probably also have the subreddit object. Perhaps we can keep it simple at first, and if someone asks for that optimization then we can add it later?

[bboe]

I'll make the changes to merge this, so I can then merge my PR in.

[voussoir]

No, wait, I'm going to push in just a second like half an hour apparently.

• Removed note from docstring that says it will return cached content
• Applied uniq to Redditor, Subreddit, and WikiPage refresh methods
• Rearranged the _unique_count incrementer due to a bug in which an object would not hard-refresh if it was the first object to be refreshed.
• Removed cache bypasses thanks to new refresh method
• Fixed test_subreddit.test_subreddit_refresh() which relied on the refresh method using cached data.
• Simplified _leave_status to not use the fullname
• Now that fullname is gone, the test for leave_contributor will use the object version of the method to make sure that works too.
• Removed default value from _leave_status, and no more type checking. Also removed the corresponding assertRaises from the test.
• Fixed _leave_status docstring for statusurl.
• Fixed Multireddit author property. Creates a Redditor object from the name in the "path" attribute.

Rearranging the _unique_count required me to rerun a bunch of cassettes. It's for the greater good.

[bboe]

Will wait. Thanks for the prompt message.

[voussoir]

Okay, I've edited my previous comment with some more stuff that I did. Took the opportunity to fix a Multireddit property too.

[voussoir]

Clarify which classes it didn't work on before

Alrighty. I also added that WikiPage is now refreshable, which I should have done before.

 

Why are you so bad at cleaning up your docstrings?

That's what you meant to say, anyway.

 

Is it important that they were removed

I know the GitHub highlighter makes it look that way, but the attributes were simply moved above the superconstructor, not removed. It was causing a getattr recursion error in my new Author attribute fix because they hadn't been set.

 

Should these assertions be temporary?

Well, if we don't check those assertions and just let it fly, the user might be very confused when the author of their multireddit is listed as "user" or something silly. I thought that having the assertions would make it immediately clear that something is broken so they can report it to us and get it patched up. Without them, the "breakage" might be much more quiet.

What's your philosophy on that?

 

Why'd you switch these around?

This only happens if its the ONLY object I refresh during that session. AKA I think the unique_counter doesn't work if it's zero. On second thought, maybe I should have gone into init and set the starting value to 1? I haven't noticed this behavior anywhere else.

[bboe]

Is it important that they were removed

I actually asked if it was important if they were moved. But you answered my question anyway. 👍

That's what you meant to say, anyway.

No I would never say that. You're doing some amazing work and it's completely normal to overlook some things.

Regarding assertions: I think they're great for testing something that you're not sure of. Maybe we want to leave them in for a period of time to see if anyone complains about the code failing. This is great if we're not certain we're doing something correctly. A good example of that is in the processing of MoreComments, where in the past I've made assertions that I don't expect to see certain things, and eventually someone found such instances. However, if you're sure it works as-is currently, then I don't think they're necessary. It would be difficult to protect against upstream changes everywhere, and when it happens the bugs will surely come in.

This only happens if its the ONLY object I refresh during that session.

Interesting. In that case, I would suggest initializing the value to 1 so that the order isn't important in refresh. Also, with issues like this, it's generally good to write a test that reliably fails with a zero initial value (maybe you can do that in a separate PR so this one doesn't continue to grow). That way someone doesn't get the bright idea in the future to initialize it again to 0.

[bboe]

I'm going to merge since the tests are all passing. Any updates can be made in a new commits. 👍

[voussoir]

I was just kidding with the docstring comment, don't worry about it.

I guess I'll remove the asserts. They're not hurting anything by being there, but you're right that the code doesn't need to be littered with asserts for every little thing.

I changed the unique_counter init to 1, and it works the way I intended it to. That should have been included in the PR instead of the old way. If I make another push to this branch, can you re-merge it, or would I have to include this in a future PR?

Edit: Or you can just make that change in the PR you're working on. Be prepared to rerun some cassettes

[bboe]

I'd suggest making a separate PR for the init change along with a test that should fail with an init value set to 0. If you want to remove the assertions you can do that there as well.