Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addition of the ability to set/unset submission as original content #1133

Merged
merged 27 commits into from
Nov 10, 2019
Merged

Addition of the ability to set/unset submission as original content #1133

merged 27 commits into from
Nov 10, 2019

Conversation

kungming2
Copy link
Contributor

@kungming2 kungming2 commented Nov 8, 2019

Feature Summary and Justification

Added two methods to the SubmissionModeration class called set_original_content() and unset_original_content(). These methods allows a moderator to set a post as "Original Content (OC)", a tag that is similar to the existing NSFW and Spoiler tags. Note that moderators can mark a post as "OC" on both Old and New Reddit; but submitters have to use New Reddit in order to mark their own posts as such. The feature itself has to be enabled in a subreddit's settings first.

Though the admin announcement says the tag is only for New Reddit, the feature was backported to Old Reddit and is viewable on the old site as well.

This endpoint is not officially on the main API page, and the admin response of a year ago was to consider adding it to the API, but that was never done officially. Therefore there is a chance that this endpoint will be deprecated without warning in the future but it currently works and the feature has been fully operational on both desktop versions of Reddit for over a year and a half. The feature does not yet appear on the mobile app.

A submission with this tag will have another attribute is_original_content set to True. If that submission's OC tag is subsequently removed, its attribute is_original_content will become set to False. Generic submissions may not have is_original_content as an attribute.

Please note that unlike the NSFW tags, this has not been integrated by Reddit into search. Therefore, while nsfw:true works, oc:true will not return any results from a search query.

I have also tested this endpoint several times with my bot u/AssistantBOT.

Timeline

References

@kungming2
Copy link
Contributor Author

kungming2 commented Nov 8, 2019

Huh, the check failed. What did I do wrong?

Edit: Maybe it's the under-indentation.

@leviroth
Copy link
Contributor

leviroth commented Nov 9, 2019

Getting to the logs for this is a bit annoying, but if you click through the "Details" on a check you eventually get to https://travis-ci.org/praw-dev/praw/jobs/609481225?utm_medium=notification&utm_source=github_status which shows the commands that failed - look for the red lines.

You should install the Python code-formatting tool black and run it on your code to fix some formatting issues. I suspect that will also fix the flake8 issues.

@kungming2
Copy link
Contributor Author

kungming2 commented Nov 9, 2019

@leviroth Ah, I see now! I will try and fix it in a little bit. Thanks!

Edit: Using Black Playground, finally got it to pass Travis.

@kungming2
Copy link
Contributor Author

Okay, next noob question: How do I get it to pass coveralls?

@leviroth
Copy link
Contributor

leviroth commented Nov 9, 2019

You need to add a test that runs your new code. You should probably put the test in tests/integration/models/reddit/test_submission.py, which you can also look to for examples. You should also look to https://praw.readthedocs.io/en/latest/package_info/contributing.html#testing for some advice on how to set up and run the tests.

@kungming2
Copy link
Contributor Author

kungming2 commented Nov 9, 2019

@leviroth I've looked through the documentation and everything and honestly I'm just lost with how to integrate a proper test.

Is it safe to add credentials to setup_reddit() in IntegrationTest?

@leviroth
Copy link
Contributor

leviroth commented Nov 9, 2019

You shouldn't put your credentials directly in the code, since we don't want you posting those to GitHub. You should set them as environment variables. If you're using Linux/bash, you can do that with the syntax here.

I suppose, since this is a moderation command, you'll need to have set up a post in a subreddit where you have mod powers. It probably makes sense to make a test subreddit for this, and maybe even a test account.

Once you have a post that you can mark as OC, your tests should probably look like the spoiler and unspoiler tests here and here.

Copy link
Contributor

@jarhill0 jarhill0 left a comment

Choose a reason for hiding this comment

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

Hi @kungming2, thanks for the PR! While @leviroth helps you write tests, I've made some comments about the documentation of the new methods. What you have in terms of documentation looks good, and it only needs some minor improvements. Thanks for working on this!

CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
praw/models/reddit/submission.py Outdated Show resolved Hide resolved
praw/models/reddit/submission.py Outdated Show resolved Hide resolved
praw/models/reddit/submission.py Outdated Show resolved Hide resolved
praw/models/reddit/submission.py Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
@jarhill0 jarhill0 dismissed their stale review November 10, 2019 18:07

Documentation looks good

@jarhill0
Copy link
Contributor

I just dismissed my review because I'm happy with the documentation. Thanks for being so responsive! I'll leave it to @leviroth to review the rest of the PR and merge it when it's ready.

@kungming2
Copy link
Contributor Author

I just dismissed my review because I'm happy with the documentation. Thanks for being so responsive! I'll leave it to @leviroth to review the rest of the PR and merge it when it's ready.

@kungming2 kungming2 closed this Nov 10, 2019
@kungming2
Copy link
Contributor Author

kungming2 commented Nov 10, 2019

@leviroth Heya, so I added the tests and they failed when I ran setup.py test with the following:

self = <tests.integration.models.reddit.test_submission.TestSubmissionModeration object at 0x105410b90>

    def test_set_original_content(self):
        self.reddit.read_only = False
        with self.recorder.use_cassette(
            "TestSubmissionModeration.test_set_original_content"
        ):
>           Submission(self.reddit, "dueqm6").mod.set_original_content()

tests/integration/models/reddit/test_submission.py:388: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
praw/models/reddit/submission.py:552: in set_original_content
    self.thing._reddit.post(API_PATH["set_original_content"], data=data)
praw/reddit.py:527: in post
    "POST", path, data=data or {}, files=files, params=params
praw/reddit.py:581: in request
    method, path, data=data, files=files, params=params
/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/prawcore/sessions.py:185: in request
    params=params, url=url)
/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/prawcore/sessions.py:116: in _request_with_retries
    data, files, json, method, params, retries, url)
/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/prawcore/sessions.py:101: in _make_request
    params=params)
/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/prawcore/rate_limit.py:34: in call
    self.delay()
/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/prawcore/rate_limit.py:50: in delay
    time.sleep(sleep_seconds)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

args = (0.0422666072845459,)

    def _sleep(*args):
>       raise Exception("Call to sleep")
E       Exception: Call to sleep

tests/conftest.py:23: Exception

Doesn't seem to be an issue with the endpoint since I ran the methods separately on a different script on the test post I specified and they worked perfectly fine.

Added the `is_original_content` attribute to the typical attributes.
@leviroth
Copy link
Contributor

Ah, you should add the line @mock.patch("time.sleep", return_value=None) as is done on test_delete.

The point of this is to explicitly mark tests that involve than one API call (plus one call to get authenticated). I think the OC code has to do this because it needs to know the subreddit, so PRAW first has to use the API to figure out which subreddit corresponds to the submission id.

@leviroth
Copy link
Contributor

If you run this test locally, you should see that it creates some JSON files. After checking to make sure that your credentials aren't in those files, you should commit and push them as well. They're used to record your tests' HTTP traffic so that they can be replayed without actually having to talk to Reddit's server.

@kungming2
Copy link
Contributor Author

kungming2 commented Nov 10, 2019

@leviroth Where can I find said JSON files?

Edit: I'm guessing the JSON files are not being created because the tests are failing still.

After patching the sleep issue, I get this. I see that it's saying I have more than positional argument for the method but I formatted it the same as all the other tests. Sorry for the noob-ness of my questions!

args = (<tests.integration.models.reddit.test_submission.TestSubmissionModeration object at 0x1086c9890>, <MagicMock name='sleep' id='4433669520'>), keywargs = {}
extra_args = [<MagicMock name='sleep' id='4433669520'>], entered_patchers = [<mock.mock._patch object at 0x108313910>]
exc_info = (<class 'TypeError'>, TypeError('test_set_original_content() takes 1 positional argument but 2 were given'), <traceback object at 0x108dd7640>)
patching = <mock.mock._patch object at 0x108313910>, arg = <MagicMock name='sleep' id='4433669520'>

    @wraps(func)
    def patched(*args, **keywargs):
        extra_args = []
        entered_patchers = []
    
        exc_info = tuple()
        try:
            for patching in patched.patchings:
                arg = patching.__enter__()
                entered_patchers.append(patching)
                if patching.attribute_name is not None:
                    keywargs.update(arg)
                elif patching.new is DEFAULT:
                    extra_args.append(arg)
    
            args += tuple(extra_args)
>           return func(*args, **keywargs)
E           TypeError: test_set_original_content() takes 1 positional argument but 2 were given

.eggs/mock-3.0.5-py3.7.egg/mock/mock.py:1330: TypeError

Added JSON files for the `SubmissionModeration` methods `set_original_content()` and `unset_original_content()`.
@kungming2
Copy link
Contributor Author

kungming2 commented Nov 10, 2019

@leviroth I think I figured it out based on reading some of the earlier commits to the test. def test_send_removal_message(self, _): instead of def test_send_removal_message(self):. I've also uploaded the JSON files from the cassettes folder. I don't believe there's any credential information in those files.

Finally everything passed! Hopefully everything is good.

@leviroth
Copy link
Contributor

I forgot about that underscore quirk; glad you figured it out.

@jarhill0 The tests look good to me, but I don't have the permissions to merge this or do the required review.

@kungming2
Copy link
Contributor Author

@leviroth Thanks for all your advice through this process!

@jarhill0
Copy link
Contributor

Oh, I thought you had permissions to make the approving review. In that case, I'll take a look at it myself in a bit.

@kungming2
Copy link
Contributor Author

@jarhill0 An update: I realize now that submitters can mark their own submissions as OC but only on New Reddit. They can't do so on Old Reddit.

So my text needs to be changed. How about from:

This method can only be used by moderators of the subreddit that the submission belongs to, if the subreddit has enabled the beta feature in settings.

to

This method can be used both by the submission author on the desktop redesign and moderators of the subreddit that the submission belongs to, if the subreddit has enabled the beta feature in settings.

Would this be okay and clear enough?

@jarhill0
Copy link
Contributor

Interesting. I just played around with it and it seems that the subreddit setting controls whether users can mark their own posts as OC, and moderators can mark them regardless of the setting. Can you confirm this behavior?

Since both users and moderators can perform this action, maybe this method shouldn't belong to the SubredditModeration class. I don't know off the top of my head how other actions that can be performed by an OP or a moderator, such as marking as NSFW, are implemented in PRAW, but that would be a good thing to look up.

@kungming2
Copy link
Contributor Author

kungming2 commented Nov 10, 2019

it seems that the subreddit setting controls whether users can mark their own posts as OC, and moderators can mark them regardless of the setting. Can you confirm this behavior?

Interesting. So from what I can tell, you have to enable the subreddit setting in order to be able to mark anything as OC as a mod on Old Reddit; the option appears to be always available to a moderator on New Reddit under the dropdown menu for a submission. (tested on r/translator) I can also confirm that the setting governs whether users can mark their own posts as OC.

I placed it in the SubredditModeration class because the most similar methods, namely nsfw() and spoiler(), which are also methods that can be used both by the submission author and moderators of the subreddit that the submission belongs to, are both already located in the SubredditModeration class and they are placed in the same location on New Reddit (under a moderator drop-down, along with the mod-only methods sticky(), distinguish() and lock()).

To me, SubredditModeration seems like the most appropriate place to put the method, and if this is not placed there and into a different class, set_original_content() and nsfw() and spoiler() should be placed together for consistency since they all have basically identical behavior.

@jarhill0
Copy link
Contributor

I placed it in the SubredditModeration class because the most similar methods, namely nsfw() and spoiler(), which are also methods that can be used both by the submission author and moderators of the subreddit that the submission belongs to, are both already located in the SubredditModeration class

Yep, I see that now.

I would make the description read

This method can be used by moderators of the subreddit that the submission belongs to. If the subreddit has enabled the Original Content beta feature in settings, then the submission's author can use it as well.

@kungming2
Copy link
Contributor Author

@jarhill0 Done!

Copy link
Contributor

@jarhill0 jarhill0 left a comment

Choose a reason for hiding this comment

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

OK, everything that I looked at previously looks ready to merge now. I just took a look at the tests for the first time and I'm requesting changes there. Once that's done, I will merge the PR. Thank you!

tests/integration/models/reddit/test_submission.py Outdated Show resolved Hide resolved
tests/integration/models/reddit/test_submission.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jarhill0 jarhill0 left a comment

Choose a reason for hiding this comment

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

Terrific! It's ready for merging, as soon as it passes CI.

@jarhill0 jarhill0 merged commit 00455ef into praw-dev:master Nov 10, 2019
@jarhill0
Copy link
Contributor

Thanks for your contribution, @kungming2! Great work on this PR.

@kungming2
Copy link
Contributor Author

Thanks @jarhill0 ! Since this was my first pull request ever, hopefully the next time I contribute things will be smoother. :)

@kungming2 kungming2 deleted the patch-2 branch November 10, 2019 22:54
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.

None yet

3 participants