Local login by maparent · Pull Request #172 · assembl/assembl

maparent · 2017-09-19T16:35:58Z

No description provided.

vincentfretin · 2017-09-20T08:59:54Z

        assert req
        if getattr(req, "lang_prefs", 0) is 0:
+            # lang. prefs remain effective even without shared login
            user_id = req.authenticated_userid


while you're at it, use authenticated_userid(req)
The syntax req.authenticated_userid is deprecated.

vincentfretin · 2017-09-20T09:01:21Z

            "default": True
        },

+        # Does the Idea panel automatically open when an idea is clicked? (and close when a special section is clicked)


Is this a bad copy-paste? The comment doesn't seem related to this new config.

vincentfretin · 2017-09-20T09:04:35Z

+  redirectToJoin() {
+    const slug = { slug: getDiscussionSlug() };
+    browserHistory.push(get('join', slug));
+  }


I think you need to push to customBrowserHistory created in index.jsx
You should export the history variable from index.jsx and use it here.

vincentfretin · 2017-09-20T09:07:12Z

        application_url=application_url,
        get_route=get_route,
        user=user,
+        loggedin_userid=request.authenticated_userid,


authenticated_userid(request)

vincentfretin · 2017-09-20T09:14:55Z

Other than that, LGTM.

vincentfretin · 2017-09-20T09:16:27Z

execute npm run format to format with prettier and eslint --fix the js code in v2. This is the same as using the prettier-atom plugin for atom.

maparent · 2017-09-20T11:56:50Z

Adjusted to the comments. Good catch about req.authenticated_userid being deprecated.
One thing before we merge: The join page needs to be made prettier. I think someone else will be better positioned to do it.
Also, I think everyone is aware that this only works as such for v2 discussions, and other limits discussed by email. (Adding them to jira while I'm at it.)

vincentfretin · 2017-09-20T13:40:08Z

@@ -1,9 +1,10 @@
 import React from 'react';
+import { customBrowserHistory } from '../../index';


customBrowserHistory is not currently exported in index.jsx. I don't see the change in your PR. You need to add export customBrowserHistory in index.jsx.

That's right... why didn't webpack complain? (I even tested!)

webpack will not complain, eslint should have. Try npm run eslint

ok, that was helpful, thank you. Have a look now?

Ok, should be good.

ayyazdaniaryan · 2017-09-21T12:00:26Z

    const connectedUserName = getConnectedUserName();
+    const alreadyLoggedIn = !!getLoggedInUserId();
+    const loginOrJoinUrl = alreadyLoggedIn ? get('join', slug) : getContextual('login', slug);
+    const onClickRedirect = alreadyLoggedIn ? this.redirectToJoin : null;


I think instead of null, the standard way is to return false

ayyazdaniaryan · 2017-09-21T12:04:05Z

        application_url=application_url,
        get_route=get_route,
        user=user,
+        loggedin_userid=authenticated_userid(request),


Shouldn't it be effectiveuserid here instead?

loggedin_userid is explicitly about knowing that the person is logged in even though they have no role in the discussion. Adding a comment.

ayyazdaniaryan · 2017-09-21T12:07:48Z



+def is_join_route(request):
+    # TODO: Get a real shared route system for react.


Agreed. Absolute TODO.

ayyazdaniaryan · 2017-09-21T12:13:03Z

+
+
+@view_config(route_name='join', request_method='POST')
+def do_join_discussion(request):


As the ultimate response of this view is to redirect to either a login page or throw an error, we should utilize the new error page that was created for exactly these cases. Instead of returning HTTPUnauthorized or others, should redirect to error page, and pass the error in.

Downstream, that view can respond to the appropriate Error with a client-friendly view (this is not done for all cases, only 404, 401 and 500 I recall)

You have an error page now? Then I think that the proper thing to do is to have HTTPUnauthorized go to that error page! Pyramid allows that. Let's discuss it.

Yup, I discussed it with you a long while back, when logged in users wanted to access a private discussion. Yes, let's discuss when you have a moment.

ok now that we have a generic error page, this code stands.

…r so login does not leak between discussions

…ubscribe

…t to join

maparent · 2017-10-08T12:53:51Z

@vincentfretin BTW, I was revisiting authenticated_userid: It's actually the security function that's deprecated, not the request attribute. One of us should revert this.

vincentfretin · 2017-10-09T07:30:45Z

Oh 💩

vincentfretin · 2017-10-09T07:31:52Z

We'll see that when we know what do to with this branch. It's on standby for the moment.

paolina92 · 2018-04-26T13:13:09Z

@maparent What do we do with this PR?

vincentfretin reviewed Sep 20, 2017

View reviewed changes

maparent force-pushed the local_login branch from c3ad160 to 4dcb422 Compare September 20, 2017 11:55

vincentfretin reviewed Sep 20, 2017

View reviewed changes

maparent force-pushed the local_login branch from 79c2e1d to bfcb5a7 Compare September 20, 2017 13:54

ayyazdaniaryan reviewed Sep 21, 2017

View reviewed changes

maparent added 13 commits September 22, 2017 08:09

Use effective_userid instead of authenticated_userid. Allows to filte…

c759c84

…r so login does not leak between discussions

Send user_login_id to react irrespective of effective_userid

2c6d092

We now have valid re-logging in, needs to fall through to maybe_autos…

cf6bd52

…ubscribe

first_visit_to_discussion

7d08af6

push some calcualations from react context to common context. Redirec…

bd88300

…t to join

jinja join page

8a6cc05

Join or redirect according to user login status

d368fb5

authenticated_userid as function

e3d2f6f

correct comment

d96c5c9

use customBrowserHistory

7c1d723

npm run format

c914e09

export customBrowserHistory

debe0de

effective_userid after rebase

37d846b

maparent force-pushed the local_login branch from bfcb5a7 to 37d846b Compare September 22, 2017 12:13

paolina92 and others added 5 commits September 25, 2017 15:15

front-end first pass

ca97b5e

Add test route in order to test the view

df7bf4c

Add the join jinja2 page

799bfb9

response to comments

cf5f0b0

Update version

d1eb74f

ayyazdaniaryan added 4 commits September 26, 2017 17:22

Add join page CSS to main path

893a648

Finalize looks of join page (TBD)

98e46d4

Add the POST route to form

12ee983

Use customBrowserHistory instead of the default browserHistory now

741aae1

ayyazdaniaryan force-pushed the develop branch from e665304 to 33afa7d Compare April 16, 2018 12:38

maparent force-pushed the develop branch from 33afa7d to d651479 Compare April 16, 2018 12:43

		@@ -1,9 +1,10 @@
		import React from 'react';
		import { customBrowserHistory } from '../../index';



		def is_join_route(request):
		# TODO: Get a real shared route system for react.



		@view_config(route_name='join', request_method='POST')
		def do_join_discussion(request):

Uh oh!

Conversation

maparent commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentfretin commented Sep 20, 2017

Uh oh!

vincentfretin commented Sep 20, 2017

Uh oh!

maparent commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maparent commented Oct 8, 2017

Uh oh!

vincentfretin commented Oct 9, 2017

Uh oh!

vincentfretin commented Oct 9, 2017

Uh oh!

paolina92 commented Apr 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants