Moderation streams

[LilSpazJoekp]

Feature Summary and Justification

This feature provides the ablilty to stream from subreddit.mod methods.
Added Moderator stream for the following:

• edited
• mod log
• modqueue
• reports
• spam
• unmoderated
• unread
• modmail_conversations

[LilSpazJoekp]

Sorry for the commits from my previous pr, I wasn't able to figure out how to not have them attached to this pr.

[LilSpazJoekp]

I also want to note that the modmail_conversation() method will not work without 7f2adcf as method modmail.conversations() will not accept params as an argument from stream_generator.

[bboe]

@LilSpazJoekp I'll try to get this cleaned up for you.

[bboe]

Okay, I cleaned up your branch. Please make sure you pull it down before making any changes. Please add a changelog entry for each added item, and we're going to need some tests for each as well. Thanks for the efforts!

[LilSpazJoekp]

I'll need some help writing the tests, I haven't written unit tests before.

[jarhill0]

Is there something specific you'd like? Or more just general guidance?

[LilSpazJoekp]

I haven’t used Betamax before and not sure how to write the code to generate the cassettes.

[jarhill0]

Take a look at the tests for SubredditStreams:

praw/tests/integration/models/reddit/test_subreddit.py

Lines 1363 to 1439 in 82d98c5

	class TestSubredditStreams(IntegrationTest):
	@mock.patch("time.sleep", return_value=None)
	def test_comments(self, _):
	with self.recorder.use_cassette("TestSubredditStreams.comments"):
	generator = self.reddit.subreddit("all").stream.comments()
	for i in range(400):
	assert isinstance(next(generator), Comment)
	@mock.patch("time.sleep", return_value=None)
	def test_comments__with_pause(self, _):
	with self.recorder.use_cassette(
	"TestSubredditStreams.comments__with_pause"
	):
	comment_stream = self.reddit.subreddit("kakapo").stream.comments(
	pause_after=0
	)
	comment_count = 1
	pause_count = 1
	comment = next(comment_stream)
	while comment is not None:
	comment_count += 1
	comment = next(comment_stream)
	while comment is None:
	pause_count += 1
	comment = next(comment_stream)
	assert comment_count == 17
	assert pause_count == 2
	@mock.patch("time.sleep", return_value=None)
	def test_comments__with_skip_existing(self, _):
	with self.recorder.use_cassette("TestSubredditStreams.comments"):
	generator = self.reddit.subreddit("all").stream.comments(
	skip_existing=True
	)
	count = 0
	try:
	for comment in generator:
	count += 1
	except RequestException:
	pass
	# This test uses the same cassette as test_comments which shows
	# that there are at least 400 comments in the stream.
	assert count < 400
	def test_submissions(self):
	with self.recorder.use_cassette("TestSubredditStreams.submissions"):
	generator = self.reddit.subreddit("all").stream.submissions()
	for i in range(101):
	assert isinstance(next(generator), Submission)
	@mock.patch("time.sleep", return_value=None)
	def test_submissions__with_pause(self, _):
	with self.recorder.use_cassette("TestSubredditStreams.submissions"):
	generator = self.reddit.subreddit("all").stream.submissions(
	pause_after=-1
	)
	submission = next(generator)
	submission_count = 0
	while submission is not None:
	submission_count += 1
	submission = next(generator)
	assert submission_count == 100
	@mock.patch("time.sleep", return_value=None)
	def test_submissions__with_pause_and_skip_after(self, _):
	with self.recorder.use_cassette("TestSubredditStreams.submissions"):
	generator = self.reddit.subreddit("all").stream.submissions(
	pause_after=-1, skip_existing=True
	)
	submission = next(generator)
	assert submission is None # The first thing yielded should be None
	submission_count = 0
	submission = next(generator)
	while submission is not None:
	submission_count += 1
	submission = next(generator)
	assert submission_count < 100

Your tests will probably look something like this (you could even copy, paste, and modify).

The most important parts are the lines like:

with self.recorder.use_cassette("TestSubredditStreams.comments"):

This specifies which Betamax cassette to use so that the network interaction can be replayed consistently.

This docs page has information about recording new Betamax cassettes, which you will have to do when writing new integration tests.

[bboe]

@LilSpazJoekp do you still have interest in getting this in?

[LilSpazJoekp]

@bboe I do. My apologies, I've been busy with reddit and life stuff and haven't had the chance to get this done.

[LilSpazJoekp]

@bboe This pr is ready.

[PythonCoderAS]

Overall solid changes. However, in order to address the decrease in coverage, I think you can implement this piece of code in the subreddit test files:

    def test_random(self):
        sub = Subreddit(self.reddit, "mod")
        submodstream = SubreddditModerationStream(self.reddit, sub)
        submodstream.modmail_conversations() # just to cause the untested line to run
        assert submodstream.subreddit = Subreddit(self.reddit, "all")

[LilSpazJoekp]

@PokestarFan shouldn’t https://github.com/LilSpazJoekp/praw/blob/45d2793d52d77647b2aa2cdfa7b68fcf5a550cd8/tests/integration/models/reddit/test_subreddit.py#L1506 do the same thing?

[LilSpazJoekp]

@PokestarFan Never mind I see why. I’ll get that added.

[PythonCoderAS]

@LilSpazJoekp my test is a unit test not an integration test so I think it can be seperated.

[PythonCoderAS]

Good changes, this should increase coverage back to optimal levels.

[PythonCoderAS]

@LilSpazJoekp merge with upstream to deal with the conflict or else it won’t resolve and we can’t merge.

git remote add upstream <praw-dev-git-url>
git pull --all
git merge upstream/master
# copy-paste lines
...

[PythonCoderAS]

@LilSpazJoekp why was this closed?

[LilSpazJoekp]

Did not mean to close it.

[PythonCoderAS]

Looking good, but those tests may need to be ran with a username/password combo.

[LilSpazJoekp]

They all pass with no environment variables set.

[PythonCoderAS]

But what if we build new tests?

[LilSpazJoekp]

See this change here

[PythonCoderAS]

So what if we provide all 3 will that break the script?

[LilSpazJoekp]

But what if we build new tests?

Either provide the prawtest_refresh_token environment variable or take out the setup_reddit method from the test.

So what if we provide all 3 will that break the script?

Nothing will happen because the setup_reddit method in that test is the only thing that uses the refresh_token placeholder.

[LilSpazJoekp]

This will also resolve #1224 because prawtest_refresh_token can be used a environment variable.

[jarhill0]

Now that tests are passing I'll take a look soon (in the next day or so) to look over the changes. Thanks!

[jarhill0]

Thanks for your persistence! This PR is looking nearly complete now that tests have been written and cassettes have been recorded. I've read through the changes and requested some changes. Please let me know if you have questions, need clarification, or if I made any mistake. Looking forward to merging this feature!

[LilSpazJoekp]

@jarhill0 those corrections have been made and committed.

[jarhill0]

Thanks for the changes. Things are looking good!

I'm not comfortable merging in a change to the way we authenticate test cassettes alongside with a standard new feature, though I am certainly in favor of adding this change as its own PR in a way that is compatible with existing tests.

So, at this point I would like to request that the change in tests/conftest.py and the related override in tests/integration/models/reddit/test_subreddit.py be removed from this PR and the cassettes re-recorded to work without them. For re-recording the tests, there are four options, any of which I'm equally happy with:

1. If you'd like, I can record the tests on my alt account, which authenticates with username and password.
2. You could temporarily disable 2FA on your account just to record the tests (this is probably a bad idea, though it is an option).
3. You could create an alt of your own and mod it in a subreddit and create at least 10 streamable actions per type (since your tests expect at least 10), and use that to record the cassettes.
4. Now that we know that the tests pass, re-record the cassettes by passing in your password followed by a colon and then the 2FA code. If any cassettes fail, delete those ones and re-record with the new 2FA code.

If it's more trouble than it's worth for you to fiddle around with any of options 2–4, I am more than happy to record the cassettes for you. And as I said, I am interested in getting support for testing even with 2FA and refresh tokens and I'd love to see that as a next PR from you.

Thanks!

[LilSpazJoekp]

All it does is just adds a new placeholder; it doesn’t change the auth process for any of the other tests. The placeholder is benign to the other tests because the default reddit instance doesn’t use it and is only used when an overriding reddit instance is used. If you’re still not comfortable with that, would it be possible to merge a pr for adding the refresh token before this one? Reason being is the cassettes are much smaller when large subs are used (due to higher activity).

[jarhill0]

it doesn’t change the auth process for any of the other tests

My concern is that it should be possible for any PRAW contributor to re-record any cassette if needed, and using different auth schemes for different cassettes disrupts that. That's why I want to hold off on introducing refresh token-based authentication in this particular PR.

If you’re still not comfortable with that, would it be possible to merge a pr for adding the refresh token before this one?

Yes, I would be happy to do that!

[LilSpazJoekp]

I will get that pr started then!

[LilSpazJoekp]

Are you talking about renaming the non-stream methods? If so, yeah I would agree that they could be named differently, but that would be a breaking change for many bots. If not, I feel it would be best to keep the name of the non-stream method to imply what they will be streaming. Just like the streams for comments and submissions are named after their non-streaming counterpart.

[jarhill0]

Are you talking about renaming the non-stream methods?

No, just the streaming methods.

I feel it would be best to keep the name of the non-stream method to imply what they will be streaming. Just like the streams for comments and submissions are named after their non-streaming counterpart.

This is a good point, which is why I'm hesitant to say that a rename is required. That said, I worry especially about the generality of the name unread, which (arguably) makes sense in the context of subreddit.mod.unread but less sense in subreddit.mod.stream.unread because it's not clear really what is being streamed or what is unread. Maybe that could be changed to old_mod_mail or something to the effect?

I'm not really happy with renaming the stream methods because it breaks continuity with their non-streaming counterparts. But neither am I happy with using these names as they feel confusing and non-descriptive.

[LilSpazJoekp]

Hm, yeah I'm split to. I'm kinda leaning towards keeping the name of the non-streaming counter part to stay consistent with the other stream methods.

[LilSpazJoekp]

@jarhill0 I created pr #1233 to add the new placeholder for refresh tokens.

[jarhill0]

Two minor things. Since you're happy with the streaming method names (which we discussed a few days ago), I'm happy with them too.

[jarhill0]

And, lastly, if you could squash these commits and then rebase them on top of master, that would be terrific.

[jarhill0]

Things look good!

[jarhill0]

This was a good journey, and it motivated #1233, which is a very useful feature. Thanks for working so hard on this, and I'd love to see more contributions from you in the future! 🎉