Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potentially switch httptester backend to new db backend #328

Merged

Conversation

dpoirier
Copy link
Member

I've created a simple DB backend. Is it worth moving httptester over to it for 0.13?

https://github.com/rapidsms/rapidsms/tree/feature/bulk-messaging-api/rapidsms/backends/db

@dpoirier
Copy link
Member

Seems reasonable to me.

- change httptester to use DB backend
- remove httptester's old backend
- update httptester doc
- add notes to 0.13.0 release notes
@copelco
Copy link
Member Author

copelco commented Mar 21, 2013

I added a poorly named name field to BackendMessage that I was thinking could be used to key/group messages accordingly. Maybe this should be renamed. Anyways, this could be used to filter down to just httptester-related messages in storage.py

@dpoirier
Copy link
Member

Does the Message Tester really need the flexibility of specifying different backend names in the URL? I was thinking it'd be simpler to configure and use if it just made its own instance of the DB backend when needed, and we took the backend name out of the URLs and didn't require adding a backend to the config for message tester.

@ghost ghost assigned dpoirier Mar 22, 2013
@copelco
Copy link
Member Author

copelco commented Mar 22, 2013

Yeah, I agree. It doesn't necessarily need different backend names. The router would still need to know about the backend via INSTALLED_BACKENDS to route outgoing messages and responses, but maybe we could just say it needs to be keyed with httptester. So I could see us dropping the need to support an httptester dynamic names.

What do you mean by "made it's own instance of the DB backend"?

@dpoirier
Copy link
Member

I meant instead of requiring the user to add a magic backend to INSTALLED_BACKENDS just for httptester, that httptester could call add_backend itself if it sees that its backend hasn't already been added.

I guess the backend would still need to be in INSTALLED_APPS so Django would know to create its database tables, so it might not simplify the configuration all that much...

- Use the backend name as the `name` in all the BackendMessages associated
  with the message tester, so we can view just those and easily clear them.
- Don't pass backend names in URLs and methods - just use a fixed backend
  name for HTTP tester, 'message_tester'
@dpoirier
Copy link
Member

@copelco I made changes to use the .name field to identify which messages came from message tester, and not passing around backend names anymore.

dpoirier added a commit that referenced this pull request Mar 25, 2013
... on the assumption that #328 is going in.
@dpoirier
Copy link
Member

Ready to review again @copelco , I'm not going to pursue automatically creating the backend like I was thinking.

@copelco
Copy link
Member Author

copelco commented Mar 27, 2013

I think we can remove name, message id and external id columns from the table on the front-end interface.

…pi' into feature/328-httptester-use-db-backend

Conflicts:
	docs/releases/0.13.0.rst
	rapidsms/backends/db/__init__.py
This is because of a Django app name collision with the database router.
@@ -31,17 +39,21 @@ To define and use Message Tester for your RapidSMS project, you will need to:
...
)

3. Add the Message Tester backend to :setting:`INSTALLED_BACKENDS`::
3. Add the DB backend to :setting:`INSTALLED_BACKENDS` with the name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be consistent across the docs, probably best to use the full DatabaseBackend name here.

@copelco
Copy link
Member Author

copelco commented Mar 27, 2013

After the doc fix, I think we're good 🌵

dpoirier added a commit that referenced this pull request Mar 27, 2013
…ackend

Potentially switch httptester backend to new db backend
@dpoirier dpoirier merged commit 3c57220 into feature/bulk-messaging-api Mar 27, 2013
@dpoirier dpoirier deleted the feature/328-httptester-use-db-backend branch March 27, 2013 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants