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

guest: Add a model field for the guest user and add guest user to populate_db. #9173

Closed

Conversation

shubhamdhama
Copy link
Member

@shubhamdhama shubhamdhama commented Apr 20, 2018

I've added a new guest user by name Polonius.
newguestuser
[edit] I've no idea why tests are failing now, any help is welcomed.

This adds new field `is_guest` to UserProfile model and
is meant for the new type of user i.e. "Guest Users".

(Part of zulip#8385).
The purpose of this user is to act as a guest.
(This is a preliminary step in adding the guest type of user
and is a part of zulip#8385.)
@shubhamdhama shubhamdhama force-pushed the guest-preliminary-add-model-user branch from f958515 to e48d5a6 Compare April 20, 2018 21:43
@timabbott
Copy link
Sponsor Member

timabbott commented Apr 20, 2018

@shubhamdhama I think the test failures are related to your changes; probably a test that needs updating. tools/test-api; if you read zerver/lib/api_test_helpers.py, you'll see it makes an assertion about the state of the test fixtures that presumably your change made no longer true.

@timabbott
Copy link
Sponsor Member

I merged the first commit, since it still passes tests.

@timabbott
Copy link
Sponsor Member

@eeshangarg do you understand why test-api is failing with the second commit here?

@timabbott
Copy link
Sponsor Member

Oh, I think I know what's happening. Adding polonius to the test database perturbs the random seed used in generating the assignment of users to streams in this block of populate_db:

            bulk_create_streams(zulip_realm, stream_dict)                                                                                          
            recipient_streams = [Stream.objects.get(name=name, realm=zulip_realm).id                                                               
                                 for name in stream_list]  # type: List[int]                                                                       
            # Create subscriptions to streams.  The following                                                                                      
            # algorithm will give each of the users a different but                                                                                
            # deterministic subset of the streams (given a fixed list                                                                              
            # of users).                                                                                                                           
            subscriptions_to_add = []  # type: List[Subscription]                                                                                  
            event_time = timezone_now()                                                                                                            

@timabbott
Copy link
Sponsor Member

The fix is to add this hunk, which results in Polonius not being auto-subscribed to any streams:

             event_time = timezone_now()
             all_subscription_logs = []  # type: (List[RealmAuditLog])
             profiles = UserProfile.objects.select_related().filter(
-                is_bot=False).order_by("email")  # type: Sequence[UserProfile]
+                is_bot=False, is_guest=False).order_by("email")  # type: Sequence[UserProfile]
             for i, profile in enumerate(profiles):
                 # Subscribe to some streams.
                 for type_id in recipient_streams[:int(len(recipient_streams) *

I think that's what we want (guests should not have a random set of subscriptions like other users), but we should also add code to subscribe him specifically to a stream (probably Denmark, thematically).

@timabbott
Copy link
Sponsor Member

Merged that second commit as 26d2ffa

@timabbott timabbott closed this Apr 20, 2018
@shubhamdhama
Copy link
Member Author

Ahh, I wasn't aware of this, now I got what was happening here, thanks for helping (I don't think I could figure out this easily)

@timabbott
Copy link
Sponsor Member

Yeah, this was a pretty subtle thing; the reason I took the lead on debugging this was because it seemed from the error situation like something that was very likely to be subtle and requiring some obscure knowledge :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants