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.

Sign up for GitHub

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

Jump to bottom

Import: Support import of huddles. #9547

Closed
wants to merge 1 commit into from
Closed

Import: Support import of huddles. #9547

wants to merge 1 commit into from

Conversation

rheaparekh
Copy link
Collaborator

For importing huddles we have to have unique huddle hashes. Huddle hashes are extracted from the list of users participating in a huddle. So to extract this user ids, we first use huddle id to getting the matching recipient, and then we use subscription to get the user ids from the recipient id.

@timabbott
Copy link
Sponsor Member

@rheaparekh how have you tested this? This is complicated enough that automated tests seem pretty high-value here.

@rheaparekh
Copy link
Collaborator Author

@timabbott I have tested this manually several times, by checking if all of the Huddle features are present in the exported and then imported realm with the original realm. For writing automated tests, I would have to start from scratch and write tests for the entire import_realm module. I am planning to do that (after finishing with the remaining features of import/export), and after I have done that, I can write tests for this feature as well. What are your thoughts?

@timabbott
Copy link
Sponsor Member

Have you seen zerver/tests/test_export.py? It has coverage on a lot of import_realm.

@rheaparekh
Copy link
Collaborator Author

@timabbott zerver/tests/test_export.py doesn't have coverage on import_realm. It only covers tests for export.py. I am planning to increase the coverage for both of these modules.

@rheaparekh
Copy link
Collaborator Author

I'll be updating this with the tests after #9659 has been merged

@neiljp
Copy link
Contributor

neiljp commented Jun 16, 2018

This is still pending on #9659?

@zulipbot zulipbot added size: L and removed size: M labels Jun 19, 2018
@rheaparekh rheaparekh force-pushed the import_huddles branch 2 times, most recently from 55b7878 to ec17d83 Compare June 19, 2018 17:31
@rheaparekh
Copy link
Collaborator Author

I have updated this with tests.

neiljp
neiljp reviewed Jun 25, 2018
View reviewed changes
Copy link
Contributor

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

I'm not overly familiar with this, but here's a review based on some more general aspects.

zerver/tests/test_import_export.py Outdated
user_id_list = [
[UserProfile.objects.get(realm=realm, short_name=name).id
for name in short_names] for realm in realms]
huddle_hash = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call this huddle_hashes, and probably use user_id_lists above, so that the highest level list comprehension loops are clearer.

zerver/tests/test_import_export.py Outdated
for name in short_names] for realm in realms]
huddle_hash = [
get_huddle_hash(id_list)
for id_list in user_id_list]
Copy link
Contributor

Choose a reason for hiding this comment

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

For example, this looks like id_list is not just one of many, but a different data type, if that makes sense?
Whereas for user_id_list in user_id_lists seems clearer, at least to me - what do you think?

zerver/tests/test_import_export.py Outdated
get_huddle_hash(id_list)
for id_list in user_id_list]

self.assertNotEqual(huddle_hash[0], huddle_hash[1])
Copy link
Contributor

Choose a reason for hiding this comment

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

I know these are "just" tests, but after these and any other tests are complete, I would enjoy a refactor to avoid using 0 and 1, as we discussed previously. Not a priority, but it makes it clearer :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll do a refactor in another PR. As you suggested, I think using a helper function would be good.

zerver/tests/test_import_export.py Outdated

huddle_ids = [
Huddle.objects.get(huddle_hash=huddle_hash).id
for huddle_hash in huddle_hash]
Copy link
Contributor

Choose a reason for hiding this comment

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

These are the same variable name, in the loop. The plurals might help.

zerver/lib/import_realm.py Outdated
Extract the IDs of the user_profiles involved in a huddle from the subscription object
This helps to generate a unique huddle hash from the updated user_profile ids
"""
id_map_to_list['huddle_to_user_list'] = dict(
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a dict comprehension:

{value: [] for value in id_maps['recipient_to_huddle_map'].values()}

zerver/lib/import_realm.py Outdated
(value, []) for value in id_maps['recipient_to_huddle_map'].values())

for subscription in data[table]:
if subscription['recipient'] in id_maps['recipient_to_huddle_map'].keys():
Copy link
Contributor

Choose a reason for hiding this comment

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

You can skip .keys() here.

@rheaparekh rheaparekh force-pushed the import_huddles branch 2 times, most recently from d5029e9 to f3a2567 Compare June 25, 2018 14:10
@rheaparekh
Copy link
Collaborator Author

rheaparekh commented Jun 25, 2018 •
edited

I have updated this! This is ready for a final review.

@zulipbot
Copy link
Member

Heads up @rheaparekh, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@timabbott
Copy link
Sponsor Member

@rheaparekh can you do a rebase on this?

@rheaparekh
Copy link
Collaborator Author

@timabbott I have rebased and updated this.

@rheaparekh
import: Support import of huddles.
75696d9 
For importing huddles we have to have unique huddle hashes.
Huddle hashes are extracted from the list of users participating
in a huddle. So to extract these user ids, we first use huddle
id to getting the matching recipient, and then we use subscription
to get the user ids from the recipient id.

Added tests for the same.
@timabbott
Copy link
Sponsor Member

timabbott commented Jul 12, 2018 •
edited

I gave this a careful read, and the logic looks great. I made a small change to some variable names in the tests for clarity (pluralization), and added a TODO comment for migrating this new test to use assert_realm_values (since it doesn't), and merged this as e988491. Thanks @rheaparekh!

@timabbott timabbott closed this Jul 12, 2018
@rheaparekh
Copy link
Collaborator Author

Oh I forget to rebase it after the refactor got merged yesterday. I'll add another PR for changing the tests to assert_realm_values method. @timabbott @neiljp Thank you for the reviews!

@rheaparekh rheaparekh deleted the import_huddles branch July 12, 2018 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neiljp neiljp neiljp left review comments

Assignees
No one assigned
Labels
size: L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rheaparekh @timabbott @neiljp @zulipbot