Second funnel app #254

iambibhas · 2018-07-11T09:55:07Z

First part of completing #230.

iambibhas · 2018-07-31T09:03:10Z

funnel/__init__.py

 mail.init_app(app)
+
+lastuser.init_app(funnelapp)
 lastuser.init_app(app)


@jace these lines above seems wrong. specially the db.app = <> part. How to handle these?

db.app assignment is only required if there's a database operation outside a request context, to let the db object know which app to operate on. This can happen during init, if we're loading from db, or doing a background job. Workaround: enclose the database operation in with app.app_context(): (we've previously used with app.test_request_context():, a somewhat heavier operation since it also constructs a dummy request).

Ok. And what about the others? Double assigning migrate and calling init_app() multiple times on db, mail and lastuser doesn't seem right.

Mail and Lastuser are fine. Migrate doesn't need to be on Funnelapp.

jace · 2018-08-17T10:08:50Z

funnel/__init__.py

@@ -12,6 +12,7 @@
 from ._version import __version__


+funnelapp = Flask(__name__, instance_relative_config=True)
 app = Flask(__name__, instance_relative_config=True)


Always put app first and funnelapp second, since funnelapp will be removed eventually.

jace · 2018-08-17T10:10:33Z

funnel/__init__.py

 db.init_app(app)
+
+db.app = funnelapp
 db.app = app


Remove both these lines.

jace · 2018-08-17T10:11:08Z

funnel/__init__.py

 db.app = app
+
+migrate = Migrate(funnelapp, db)


Remove this. Migrate only needs to be on the main app.

jace · 2018-08-17T10:11:59Z

funnel/__init__.py

 mail.init_app(app)
+
+lastuser.init_app(funnelapp)
 lastuser.init_app(app)


Mail and Lastuser are fine. Migrate doesn't need to be on Funnelapp.

jace · 2018-08-17T10:13:13Z

funnel/util.py

@@ -4,8 +4,8 @@
 from urlparse import urlparse
 import qrcode
 import qrcode.image.svg
+from flask import current_app


Place this import above qrcode since it's a more foundational library.

jace · 2018-08-17T12:35:39Z

funnel/views/proposal.py

@@ -379,7 +388,8 @@ def proposal_prev(profile, space, proposal):
        return redirect(space.url_for())


-@app.route('/<space>/<proposal>/move', methods=['POST',], subdomain='<profile>')
+@app.route('/<profile>/<space>/<proposal>/move', methods=['POST',])
+@funnelapp.route('/<space>/<proposal>/move', methods=['POST',], subdomain='<profile>')


methods=['POST'] in both lines (for style consistency).

jace · 2018-08-17T12:49:16Z

funnel/views/usergroup.py

+@funnelapp.route('/<space>/users/new', defaults={'group': None}, endpoint='usergroup_new',
+    methods=['GET', 'POST'], subdomain='<profile>')
+@app.route('/<profile>/<space>/users/<group>/edit', methods=['GET', 'POST'])
+@funnelapp.route('/<space>/users/<group>/edit', methods=['GET', 'POST'], subdomain='<profile>')


Order them app, app, funnelapp, funnelapp.

jace · 2018-08-17T12:50:49Z

manage.py

@@ -2,7 +2,7 @@

 from coaster.manage import init_manager

-from funnel import app, models
+from funnel import funnelapp, app, models


funnelapp is not required here.

jace · 2018-08-17T12:51:22Z

rqfunnelinit.py

@@ -0,0 +1,14 @@
+from urlparse import urlparse


This file is not required. The same RQ backend will power both apps.

jace · 2018-08-17T12:51:54Z

runserver.py

@@ -6,5 +6,5 @@
 try:
    port = int(sys.argv[1])
 except (IndexError, ValueError):
-    port = 3000
+    port = 3001


Keep the port here, change it for Funnel.

jace

Partial review, will continue later today.

jace · 2018-09-17T04:22:04Z

funnel/jobs/jobs.py



 def import_tickets(ticket_client_id):
-    with app.test_request_context():
+    with current_app.test_request_context():


Change this back to app. If you need a test_request_context, then current_app will be None, so this construct will fail. Do this everywhere that test_request_context or app_context is used.

jace · 2018-09-17T04:24:00Z

funnel/models/profile.py

+        spaces_all = ProposalSpace.fetch_sorted().filter(
+            ProposalSpace.profile == self, ProposalSpace.parent_space == None
+        ).all()
+        return spaces_all


This should be a relationship. Why is it a property?

Not sure why it was defined as a property, I just organized it. Anyway, now when I try to define it as a relationship, there is a circular import issue. ProposalSpace model requires Profile for a relationship and vice versa.

You can put the name in quotes. SQLAlchemy will do an eval using the model namespace to pick the correct model. That means any valid Python expression will work inside the quotes.

jace

A few more cosmetic changes.

jace · 2018-09-17T06:48:58Z

funnel/views/index.py

 from ..models import Profile, ProposalSpace, Proposal
 from .space import space_data


+def index_jsonify(data):
+    spaces_data = list()


What's wrong with spaces_data = []?

jace · 2018-09-17T06:51:04Z

funnel/views/index.py

+        space_dict = dict(space.current_access())
+        if space_dict:
+            spaces_data.append(space_dict)
+    return jsonify(spaces=spaces_data)


This whole thing could be:

return jsonify(spaces=[d for d in [dict(s.current_access()) for s in data['spaces']] if d])

jace · 2018-09-17T06:52:11Z

funnel/views/index.py

 def index():
    g.profile = None
    g.permissions = []
    spaces = ProposalSpace.fetch_sorted().filter(ProposalSpace.profile != None).all()
-    return render_template('index.html.jinja2', spaces=spaces)
+    return dict(spaces=spaces)


return {'spaces': spaces}

jace · 2018-09-17T06:52:21Z

funnel/views/index.py

+    g.profile = None
+    g.permissions = []
+    spaces = ProposalSpace.fetch_sorted().filter(ProposalSpace.profile != None).all()
+    return dict(spaces=spaces)


jace · 2018-09-17T06:53:25Z

funnel/views/index.py

+@render_with({'text/html': 'funnelindex.html.jinja2', 'application/json': index_jsonify})
+def funnelindex():
+    g.profile = None
+    g.permissions = []


This has to be current_auth.permissions now. See Coaster's load_models for how to set it.

g.permissions is used in a lot of places on this repo. I'm not completely sure about the effects of change to current_auth.permissions. can we do this part of another PR? or maybe the next PR.

current_auth.permissions is guaranteed to be an InspectableSet. g.permissions only promises to support the in operator. It could be a list, tuple or set. This difference is crucial for the transition.

jace · 2018-09-17T06:54:55Z

funnel/views/index.py

+            spaces_data.append(space_dict)
+    return jsonify(
+        profile=dict(data['profile'].current_access()),
+        spaces=spaces_data)


Simplify this as above.

jace · 2018-09-17T07:17:02Z

funnel/views/space.py

@@ -38,7 +38,8 @@ def space_data(space):


 # Test endpoint
-@app.route('/form', methods=['GET', 'POST'], subdomain='<profile>')
+@app.route('/<profile>/form', methods=['GET', 'POST'])
+@funnelapp.route('/form', methods=['GET', 'POST'], subdomain='<profile>')


Remove this endpoint. We added it to test Baseframe's form generator. It's not required anymore.

iambibhas · 2018-09-17T12:55:10Z

funnel/views/index.py

+    g.permissions = []
+    spaces = ProposalSpace.fetch_sorted().filter(ProposalSpace.profile != None).all()
+    # XXX: Can we just return index() ?
+    return {'spaces': [d for d in [dict(s.current_access()) for s in spaces] if d]}


iambibhas · 2018-09-17T12:58:03Z

funnel/models/profile.py

+    parent_spaces_test = db.relationship('ProposalSpace',
+        lazy='dynamic',
+        primaryjoin="and_(ProposalSpace.profile_id == Profile.id, ProposalSpace.parent_space_id == None)"
+        )


@jace need some help with this one, parent_spaces below is returned by the fetch_sorted() classmethod which, along with sorting the results, does a state filter, which I can't do where without importing ProposalSpace, which I can do as I removed the circular import issue from space.py, but want to check if it's possible without importing ProposalSpace

That entire function could be a SQL expression, which means it could be a relationship.

What's complicated in the SQL expression is the custom sorting that fetch_sorted() does. Closest event is first, then second third are future events and then forth and so on are older events etc like that. can be done using sqlalchemy's hybrid_property I think.

Leave this aside for now. I'll look at rewriting it as a relationship later.

iambibhas · 2018-09-17T12:59:53Z

funnel/views/index.py

+@render_with({'text/html': 'funnelindex.html.jinja2'}, json=True)
+def funnelindex():
+    g.profile = None
+    g.permissions = []


@jace g.permissions is widely used in this repo. I'm not too sure about the changes required to change them back to current_auth.permissions. Can we handle this in a separate PR?

iambibhas · 2018-09-17T13:12:07Z

funnel/views/index.py

+    g.permissions = []
+    spaces = ProposalSpace.fetch_sorted().filter(ProposalSpace.profile != None).all()
+    # XXX: Can we just return index() ?
+    return {'spaces': spaces}


@jace can we just return index()?

You'll get the output of render_with, unless you pass it _render=False. https://coaster.readthedocs.io/en/latest/views/decorators.html#coaster.views.decorators.render_with

* replaced app with funnelapp and created new app instance * added route for new app to all views * replaced runtime use of app iwth current_app * renamed runfunnelserver.py to runfunnel.py * separated rqinit file for both applications * added app specific configuration * added comment for app specific config * fixed app order, removed unnecessary views from funnelapp and more fixes * renamed jsonify function name * moved model import inside function to avoid circular import * misc fixes * moving lastuser confix to app specific config files * added LASTUSER_COOKIE_DOMAIN to app specific config * fixed mui css issue with funnelapp * funnelindex page update with new UI * downloading CSV file instead of showing on tab * using content type text/csv * fixed placement of few hr tags * Call embedIframe only when preview video link is present. * misc fixes * reverted to index_jsonify function * removed unused function * using parent_spaces_test relationship to test * removed parent_spaces_test * Sync funnelindex with index. Change heading to bold on proposal page. * removed doubt

* Add static pages. * Second funnel app (#254) * replaced app with funnelapp and created new app instance * added route for new app to all views * replaced runtime use of app iwth current_app * renamed runfunnelserver.py to runfunnel.py * separated rqinit file for both applications * added app specific configuration * added comment for app specific config * fixed app order, removed unnecessary views from funnelapp and more fixes * renamed jsonify function name * moved model import inside function to avoid circular import * misc fixes * moving lastuser confix to app specific config files * added LASTUSER_COOKIE_DOMAIN to app specific config * fixed mui css issue with funnelapp * funnelindex page update with new UI * downloading CSV file instead of showing on tab * using content type text/csv * fixed placement of few hr tags * Call embedIframe only when preview video link is present. * misc fixes * reverted to index_jsonify function * removed unused function * using parent_spaces_test relationship to test * removed parent_spaces_test * Sync funnelindex with index. Change heading to bold on proposal page. * removed doubt * Update footer with hasgeek info. * Update hasgeek index page. * Update navbar in about page. * Update Flask Flat Pages in requirements.txt. * Fix columns of footer. * Minor css changes. * Add divider btw headings. * Add analytics and social media widgets. Add previous events section. * added flatpages config to app settings

* replaced app with funnelapp and created new app instance * added route for new app to all views * replaced runtime use of app iwth current_app * renamed runfunnelserver.py to runfunnel.py * separated rqinit file for both applications * added app specific configuration * added comment for app specific config * fixed app order, removed unnecessary views from funnelapp and more fixes * renamed jsonify function name * moved model import inside function to avoid circular import * misc fixes * moving lastuser confix to app specific config files * added LASTUSER_COOKIE_DOMAIN to app specific config * fixed mui css issue with funnelapp * funnelindex page update with new UI * downloading CSV file instead of showing on tab * using content type text/csv * fixed placement of few hr tags * Call embedIframe only when preview video link is present. * misc fixes * reverted to index_jsonify function * removed unused function * using parent_spaces_test relationship to test * removed parent_spaces_test * Sync funnelindex with index. Change heading to bold on proposal page. * removed doubt

* Add static pages. * Second funnel app (#254) * replaced app with funnelapp and created new app instance * added route for new app to all views * replaced runtime use of app iwth current_app * renamed runfunnelserver.py to runfunnel.py * separated rqinit file for both applications * added app specific configuration * added comment for app specific config * fixed app order, removed unnecessary views from funnelapp and more fixes * renamed jsonify function name * moved model import inside function to avoid circular import * misc fixes * moving lastuser confix to app specific config files * added LASTUSER_COOKIE_DOMAIN to app specific config * fixed mui css issue with funnelapp * funnelindex page update with new UI * downloading CSV file instead of showing on tab * using content type text/csv * fixed placement of few hr tags * Call embedIframe only when preview video link is present. * misc fixes * reverted to index_jsonify function * removed unused function * using parent_spaces_test relationship to test * removed parent_spaces_test * Sync funnelindex with index. Change heading to bold on proposal page. * removed doubt * Update footer with hasgeek info. * Update hasgeek index page. * Update navbar in about page. * Update Flask Flat Pages in requirements.txt. * Fix columns of footer. * Minor css changes. * Add divider btw headings. * Add analytics and social media widgets. Add previous events section. * added flatpages config to app settings

Bibhas added 7 commits July 11, 2018 14:56

replaced app with funnelapp and created new app instance

a991fe3

added route for new app to all views

5a30016

replaced runtime use of app iwth current_app

1f08109

renamed runfunnelserver.py to runfunnel.py

9985310

separated rqinit file for both applications

87d54ef

added app specific configuration

ed30be1

added comment for app specific config

7d6412c

iambibhas changed the title ~~Second funnel app [WIP]~~ Second funnel app Jul 31, 2018

iambibhas requested a review from jace July 31, 2018 08:42

iambibhas commented Jul 31, 2018

View reviewed changes

iambibhas changed the base branch from master to hasgeekapp August 14, 2018 07:36

jace requested changes Aug 17, 2018

View reviewed changes

Bibhas added 7 commits August 20, 2018 01:00

fixed app order, removed unnecessary views from funnelapp and more fixes

8b25851

renamed jsonify function name

5b82b46

Fixed conflict

b7d06ee

moved model import inside function to avoid circular import

2985ace

misc fixes

3a5339f

moving lastuser confix to app specific config files

496d061

added LASTUSER_COOKIE_DOMAIN to app specific config

a19c56b

vidya-ram mentioned this pull request Sep 11, 2018

Update Funnel UI to MUI (material design css framework) #261

Merged

Bibhas added 7 commits September 12, 2018 07:54

Merge branch 'master' of github.com:hasgeek/funnel into url-rewrite

b531ce8

fixed mui css issue with funnelapp

decbe45

funnelindex page update with new UI

277fcb3

downloading CSV file instead of showing on tab

5453045

using content type text/csv

7b7a73f

Merge branch 'master' of github.com:hasgeek/funnel into url-rewrite

c9711ea

fixed placement of few hr tags

ded4710

jace requested changes Sep 17, 2018

View reviewed changes

Call embedIframe only when preview video link is present.

d76005b

misc fixes

f29f040

iambibhas commented Sep 17, 2018

View reviewed changes

reverted to index_jsonify function

aab659c

iambibhas commented Sep 17, 2018

View reviewed changes

Bibhas and others added 6 commits September 17, 2018 18:52

removed unused function

84822cc

using parent_spaces_test relationship to test

26bdaa3

removed parent_spaces_test

053a3fe

Merge branch 'master' into url-rewrite

f37e63e

Sync funnelindex with index. Change heading to bold on proposal page.

f55a51c

removed doubt

6379724

iambibhas merged commit 8ffd7f4 into hasgeekapp Sep 20, 2018

iambibhas deleted the url-rewrite branch September 20, 2018 02:11

jace pushed a commit that referenced this pull request Apr 6, 2020

Update sqlalchemy_utils from 0.33.3 to 0.33.4 (#254)

ff0809c

vidya-ram pushed a commit that referenced this pull request Nov 23, 2021

Update sqlalchemy_utils from 0.33.3 to 0.33.4 (#254)

0b3fe6a

Second funnel app #254

Second funnel app #254

Conversation

iambibhas commented Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambibhas Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambibhas Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambibhas commented Jul 11, 2018 •

edited

Loading

iambibhas Sep 18, 2018 •

edited

Loading

iambibhas Sep 17, 2018 •

edited

Loading