Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

Async subscribe #107

Merged
merged 2 commits into from
Oct 2, 2014
Merged

Async subscribe #107

merged 2 commits into from
Oct 2, 2014

Conversation

Osmose
Copy link

@Osmose Osmose commented Sep 26, 2014

Updates the subscribe endpoint to avoid creating a user account within the view if sync is set to N, instead creating the user during the delayed task.

In order to make the code I was changing a little cleaner, I also happened to move all the utility functions out of views.py and into utils.py. I fixed up the tests to pass, and re-wrote the tests for the subscribe view as well as the update_user_task utility function because sorting through the existing tests (which sort've tested the view, task, and utility all at once) was proving too difficult. Apologies if that makes this hard to follow; I tried to separate the correct things into the two commits to help review.

@pmclanahan r?

@@ -673,7 +669,7 @@ def confirm_user(token, user_data):
"""
# Get user data if we don't already have it
if user_data is None:
from .views import get_user_data # Avoid circular import
from .utils import get_user_data # Avoid circular import

Choose a reason for hiding this comment

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

Is the circular import still a concern after moving this function?

Copy link
Author

Choose a reason for hiding this comment

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

It was before I moved the one import from tasks to the inside of a function in utils. Now it's not.

@Osmose
Copy link
Author

Osmose commented Oct 1, 2014

Updated and rebased.

@pmclanahan
Copy link

This looks good to me. Huge +1 for not only fixing the bug but doing some much needed reorganization of the code. 🍰 👏

@pmclanahan
Copy link

pmclanahan pushed a commit that referenced this pull request Oct 2, 2014
Fix bug 1047002: Async subscribe
@pmclanahan pmclanahan merged commit 4ed0680 into mozilla:master Oct 2, 2014
@Osmose Osmose deleted the async-subscribe branch October 2, 2014 14:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants