Application Multiplexer #844
Conversation
ghost
assigned 
| @@ -136,6 +136,10 @@ def get_router_definition(router_type, router=None): | |||
| 'namespace': 'keyword', | |||
| 'display_name': 'Keyword', | |||
| }, | |||
| 'go.routers.app_multiplexer': { | |||
| 'namespace': 'application_multiplexer', | |||
There was a problem hiding this comment.
It's probably best if the namespace matches the module name, i.e. app_multiplexer. We don't do that in a few places but those are all places which we want to get rid of eventually.
There was a problem hiding this comment.
The namespace value needs to change to match the module name.
|
General layout looks good. :) |
…sue-844-application-multiplexer
…sue-844-application-multiplexer
| @@ -24,6 +24,8 @@ | |||
| {% for edit_form in edit_forms %} | |||
| <fieldset> | |||
| {{ edit_form|crispy }} | |||
| <!-- TODO: style this correctly --> | |||
| <strong>{{ edit_form.non_form_errors }}</strong> | |||
There was a problem hiding this comment.
I think this should be above the form and probably protected with a block that checks that non_form_errors exists (i.e. the form is a formset) and a note that this is a crispy forms bug (I've filed django-crispy-forms/django-crispy-forms#298 for it).
|
Review done for now. I would work on |
| response = self.client.get(rtr_helper.get_view_url('edit')) | ||
| self.assertEqual(response.status_code, 200) | ||
|
|
||
| def test_user_input_good(self): |
There was a problem hiding this comment.
We should probably have a corresponding test_user_input_bad() or something.
| # Static configuration | ||
| session_expiry = ConfigInt( | ||
| "Maximum amount of time in seconds to keep session data around", | ||
| default=1800, static=True) |
There was a problem hiding this comment.
Half an hour is a very long default session expiry. Maybe change this to 5 minutes? We update the expiry every time we update the session, so it would expire 5 minutes after the last change instead of 5 minutes after the initial creation.
|
Finished this review pass. There are a whole pile of minor things (some of which probably slipped through previous review passes) and a couple of less minor ones. |
| config.redis_manager, | ||
| key_prefix=self.worker_name, | ||
| max_session_length=config.session_expiry | ||
| ) |
There was a problem hiding this comment.
This needs to have the router key in the key_prefix. Something like ':'.join([self.worker_name, config.router.key]) should do the trick.
There was a problem hiding this comment.
Good catch, I've pushed a commit which fixes that.
|
Pointer to #844 (comment) which is hidden behind an "outdated diff" line. |
|
👍 🎆 \o/ |
…ultiplexer Merge in the application multiplexing router
This is to track a special router which links various Go USSD apps to a single starcode.