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
Fixes: #2426 Populate with UserPresences #2626
Conversation
Automated message from Dropbox CLA bot @Juanvulcano, it looks like you've already signed the Dropbox CLA. Thanks! |
@@ -177,8 +179,22 @@ def handle(self, **options): | |||
Recipient.objects.filter(type=Recipient.STREAM)] | |||
|
|||
# Extract a list of all users | |||
user_profiles = [user_profile.id for user_profile in UserProfile.objects.all()] # type: List[int] | |||
|
|||
user_profiles = [user_profile for user_profile in UserProfile.objects.all()] # type: List[Any] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use user_profiles = list(UserProfile.objects.all())
. The type should be UserProfile
instead of Any
.
|
||
# Populate users with some bar data | ||
for user in user_profiles: | ||
month = random.choice(range(1, 13)) # type: Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use random.randint(1, 12)
. Type should be int
. Same for other places where you have used choice
.
random_client = get_client("API") | ||
else: | ||
random_client = get_client("website") | ||
UserPresence.objects.get_or_create(user_profile=user, client=random_client, timestamp=random_date, status=random_status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use just status
, date
and client, drop the
random_` part.
random_client = get_client("website") | ||
UserPresence.objects.get_or_create(user_profile=user, client=random_client, timestamp=random_date, status=random_status) | ||
|
||
user_profiles = [user_profile.id for user_profile in UserProfile.objects.all()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to hit the DB again, just use user_profile_ids = [user_profile.id for user_profile in user_profiles]
.
@Juanvulcano, added some feedback, taking care of it should fix your mypy issues as well. I would recommend that you take your mypy changes that are not related to this issue to a separate commit. Also, add |
Hi, I followed the recommended suggestions but Mypy is still failling, what could be wrong? |
9a8d39f
to
94d4baf
Compare
Umar suggests changing UserProfile model to be of UserProfile type instead of List[Any], this gives the following error: Also, test is failing here I think because the data I am delivering is not the same but I don't know how to fix it. Maybe I need to add new list to the test_runner.py test? https://travis-ci.org/zulip/zulip/jobs/182114718 |
b894637
to
f1d2f41
Compare
Hi @Juanvulcano, this is looking really good! I think to fix up this remaining Travis CI failure you'll need to edit It'd also be great if you could reword your commits so that there are only two of them, even after the tests fix described above. Then:
Once this is passing tests I expect it will be good to merge! |
a988318
to
ec94629
Compare
Hi, I've been trying to debug this assertion error. |
What's going on here is that we some tests specifically for the presence system which assume that they created the only UserPresence entries that exist, and they fail when you add in some extra random data. We really only need your pre-populated presence data for manual testing, not for the automated test suite. @Juanvulcano can you try putting your new presence creation stuff in I think the other thing we could consider is hardcoding the data rather than generating it randomly. |
(accidentally hit enter on the previous message; I just edited my comment to not be half-finished) |
Hi @timabbott , just before reading your message I commited some changes that made the automated test suite work with the new data. Do you think it's a good solution or would you prefer me to have the new data inside the if statement? |
Well, my guess is the new data will probably change every time you run |
Looking at the test changes, I think I'd rather add the new data inside the |
@timabbott Data was not randomly generated and was sorted before assessing, why do you think the new data would change? |
7313b77
to
9fec7ba
Compare
date = datetime.datetime.today() # type: datetime.datetime | ||
client = get_client("website") | ||
for i in range(3): | ||
client = get_client("API") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are overriding client
here. client
will never be equal toget_client("website")
due to client = get_client("API")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you can just remove the following block:
for i in range(3):
client = get_client("API")
@Juanvulcano, looking great. So it would be great if you take all the mypy fixes that are not related to #2426 in another commit. You also need to update the commit message. It should have the following format:
You should have two commits at the end. One commit with unrelated mypy changes and the other with the fix. |
For your first commit can you say something like this?:
|
# Populate users with some bar data | ||
for user in user_profiles: | ||
status = 1 # type: int | ||
date = datetime.datetime.today() # type: datetime.datetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use now()
here instead of today()
Fixes: zulip#2426 mypy: Added annotations to populate_db.py
Merged! Thanks @Juanvulcano! |
@Juanvulcano what's the story with this block of code?
I don't understand why one would do this in a loop... |
Hi, debugging models is harder than I thought it would haha.
Note that currently there is an error that fails the Mypy test, I would love to know what annotations are needed to solve it.