Remove request.user from wiki2

[luhn]

WIP, I need to fix the testing tutorial. Right now it gives an error on setting identity, I need to add a dummy security policy to the tutorial. (Useful thing for the tutorial to cover anyways.)

But wanted to get the discussion going because I feel like people will have opinions. I can see both sides. In my opinion, the tutorial should be as "pure" Pyramid as possible. request.user is a nice convenience that a lot of people will opt for, but I think it's important to emphasize request.identity for new users, as that's the construct the framework uses.

Thoughts?

[mmerickel]

I think I'm ok with this change. In my own apps there is always a difference between identity and user and I intend to teach it that way but it's probably it's better to try and simplify things where possible for the tutorial. @bertjwregeer ?

[stevepiercy]

Is the question, for newbies, whether it is better or not to make the distinction between identity and user?

Is the distinction a feature that attracts developers to Pyramid versus other frameworks?

I prefer completeness. I might not understand the concept when I first read it, but I have many times come back to Pyramid docs and tutorials when my tiny brain is ready to comprehend the concept. That has a lot of value.

Side note, whatever is decided, the other wiki tutorial and Quick Tutorial should be updated as well.

[luhn]

Is the question, for newbies, whether it is better or not to make the distinction between identity and user?

Since "user" isn't something provided by Pyramid, the distinction is whatever the application says it is. For wiki2, identity and user are one and the same, so I don't think request.user imparts any extra knowledge, except maybe how to use custom request properties. imo it's preferable to emphasize that request.identity is how you access the result of SecurityPolicy.identity.

Side note, whatever is decided, the other wiki tutorial and Quick Tutorial should be updated as well.

Agreed, if this goes through those will be next.

[stevepiercy]

For wiki2, identity and user are one and the same, so I don't think request.user imparts any extra knowledge, except maybe how to use custom request properties. imo it's preferable to emphasize that request.identity is how you access the result of SecurityPolicy.identity.

Ah, now it clicks with me. See! I told you I have to read things multiple times. Thanks for the clarification.

[digitalresistor]

I think I'm ok with this change. In my own apps there is always a difference between identity and user and I intend to teach it that way but it's probably it's better to try and simplify things where possible for the tutorial. @bertjwregeer ?

I am okay with this change too for the tutorial. In my case the identity object will always be something that is "user" like, and I will no longer have a request.user or analog thereof (my user is a wired object instead...)

I think this is a great change that will help simplify the tutorials.

[luhn]

So working on the tests, have some questions about best practice. It involves routes/URL dispatch, which I don't work with much.

To mock DummyRequest.identity, I need to run Configurator.set_security_policy. To get that Configurator, I usually use pyramid.testing.testConfig. When I set this up, all the tests in tests/test_views.py break because they depend on routes defined in the application. What's the best way to handle this?

• Manually define routes needed for each test
• config.include('tutorial.routes') to get all the routes defined on the test config
• Use the registry from the full app when creating test config (i.e. testConfig(registry=app.registry))
• Something else?

Here's my current work: ab15b5c

[luhn]

Looking at warehouse, which is my go-to for practical Pyramid, it looks like they mock DummyRequest.route_path.

[mmerickel]

First of all, what's wrong with dummy_request.identity = ...? That's not working I take it? I would probably want that to work in the same way as the original dummy_request.user = ... without needing to change the security policy.

Secondly, about the structure of the tests, it's a good question - one that may be good to add a pattern for. I tailored the new pytest fixtures toward functional and integration tests without a lot of consideration for patching the config per-test. As you saw, the app has scope='session' and thus should not be re-configured in individual tests.

I guess my question would be, are you thinking you want to start from scratch and build your own app in the test like you've attempted to do so far with pyramid.testing.testConfig, or would you rather want to patch the app that's being loaded from the ini? I suspect for things like this it's hard to say which is best.

If we'd like to patch the config loaded from the ini then I think the approach I've defined falls apart a bit because it starts from the assumption that it's calling main and thus abstracted from the actual config object inside main. The alternative is probably not unlike what you've started which is to define your own piece-meal config that you build per-test.

[luhn]

Yeah, I get AttributeError: can't set attribute when attempting request.identity = user. Same with request.authenticated_userid... it's been that way even before 2.0. The only way to set them is registering a DummySecurityPolicy.

The more I think about it the more I think testing with a blank testConfig and manually setting the routes per test is the way to go. It seems too unwieldy to have the entire app config loaded for each integration test. I think warehouse's strategy of mocking is good too, but imo it's basically the same as testconfig.add_route and I prefer the latter.

[mmerickel]

I agree with you that the dummy_config seems nice for solving this issue. It provides a fixture in the test suite that you can expand on with a version of your app that is ephemeral and good for patching. This is a pattern we should add to the cookiecutter prior to release with a proper docstring.

[luhn]

Already, ready for review.

I removed app_request. It wasn't used anywhere and I'm not sure how useful it is over DummyRequest. The change is in its own commit, so it's a simple revert if y'all disagree.

[stevepiercy]

The code LGTM. After @mmerickel approves, I would like to run through the tutorial to ensure that the narrative docs align with the code (references to line numbers in included code, etc.) before this PR is merged.

Thank you!

[mmerickel]

I do not want to emphasize that testing request.identity is None is a thing people can do. We added request.is_authenticated and it should be used to avoid generally making assumptions about the meaning of identity being None. I noted it in a couple spots but you can find the rest.

The other issue is that none of the test fixture changes have been backported to the cookiecutter, which needs to be done before this is merged.

[mmerickel]

I removed app_request. It wasn't used anywhere and I'm not sure how useful it is over DummyRequest. The change is in its own commit, so it's a simple revert if y'all disagree.

Well the app_request is basically identical to a real request coming through the router. It has extra request methods / properties attached to it. It depends on what you need to test, but I felt this was a nice thing to have. It also has the advantage of having the threadlocals pushed for the few apis that rely on get_current_request() or get_current_registry(). If you use the dummy_request fixture without the dummy_config then these threadlocals are not pushed.

I don't know what the best pattern is for the fixtures, I'm glad you're looking at them. A PR to the cookiecutter would help focus on the fixtures.

[stevepiercy]

FYI, I started a PR to migrate from travis/appveyor to GHA in the cookiecutter on master. I'll backport it when it is approved.

Pylons/pyramid-cookiecutter-starter#94

[luhn]

The other issue is that none of the test fixture changes have been backported to the cookiecutter, which needs to be done before this is merged.

I'm not familiar with cookiecutter. I'm looking at https://github.com/Pylons/pyramid-cookiecutter-starter/blob/latest/%7B%7Bcookiecutter.repo_name%7D%7D/%7B%7Bcookiecutter.repo_name%7D%7D/base_tests.py, but it doesn't seem to have much in common with the tutorial tests

[mmerickel]

You're on the right track... you're looking at the default branch (latest) and the ones you want are in the master branch pending release alongside 2.0.

https://github.com/Pylons/pyramid-cookiecutter-starter/tree/master/%7B%7Bcookiecutter.repo_name%7D%7D/tests

[stevepiercy]

For an explanation of branches and versions in pyramid-cookiecutter-starter, see https://github.com/Pylons/pyramid-cookiecutter-starter/tree/master#versions

[luhn]

Aha, yeah, that looks much more familiar!

[luhn]

I've added cookiecutter changes over at Pylons/pyramid-cookiecutter-starter#102

[stevepiercy]

I don't have an opinion about app_request, except whether it is actually used and needed in these contexts, both in the cookiecutter and the wiki tutorial. If it is not needed, then I'd say do the minimum required and remove it. Optionally add a note in the testing chapter with the spirit of @mmerickel's comment to provide a comparison of methods.

[luhn]

Brought in line with new cookiecutter (and tests pass), so this should be g2g.