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

Migrate direct message and tag state on room upgrade #4412

Merged
merged 23 commits into from Jan 28, 2019

Conversation

Projects
None yet
3 participants
@anoadragon453
Copy link
Member

anoadragon453 commented Jan 17, 2019

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off

Sytest PR: matrix-org/sytest#547

@anoadragon453 anoadragon453 force-pushed the anoa/dm_room_upgrade branch from 1707fe3 to db9e7df Jan 17, 2019

@anoadragon453

This comment has been minimized.

Copy link
Member Author

anoadragon453 commented Jan 17, 2019

According to the spec, is_direct should also be set in the createRoom event as well. That's still TODO.

Done.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 17, 2019

Codecov Report

Merging #4412 into develop will increase coverage by 0.02%.
The diff coverage is 92.85%.

@@             Coverage Diff             @@
##           develop    #4412      +/-   ##
===========================================
+ Coverage    74.73%   74.75%   +0.02%     
===========================================
  Files          336      336              
  Lines        34122    34138      +16     
  Branches      5547     5552       +5     
===========================================
+ Hits         25500    25521      +21     
+ Misses        7048     7041       -7     
- Partials      1574     1576       +2

@anoadragon453 anoadragon453 changed the title Migrate direct message state on room upgrade [WIP] Migrate direct message and tag state on room upgrade Jan 18, 2019

@anoadragon453 anoadragon453 force-pushed the anoa/dm_room_upgrade branch from d40d531 to 46718b1 Jan 22, 2019

@anoadragon453 anoadragon453 force-pushed the anoa/dm_room_upgrade branch from 2c5f453 to d9bcecf Jan 22, 2019

@anoadragon453 anoadragon453 changed the title [WIP] Migrate direct message and tag state on room upgrade Migrate direct message and tag state on room upgrade Jan 22, 2019

@anoadragon453 anoadragon453 force-pushed the anoa/dm_room_upgrade branch from d9bcecf to 8086a5c Jan 22, 2019

@anoadragon453

This comment has been minimized.

Copy link
Member Author

anoadragon453 commented Jan 22, 2019

So this seems to be failing due to a timeout on sending a make_leave request over federation.

Ctrl-F for 2019-01-22 13:27:05,040 in https://10830-22844864-gh.circle-artifacts.com/0/logs/server-0/homeserver.log to see the error. Possibly explains why the CI would fail while locally the tests run fine?

This PR does add logic to when someone joins a room. Not sure if that has to do with make_leave however.

@anoadragon453 anoadragon453 force-pushed the anoa/dm_room_upgrade branch from ab2f127 to 766a172 Jan 22, 2019

@anoadragon453 anoadragon453 requested a review from matrix-org/synapse-core Jan 22, 2019

@richvdh
Copy link
Member

richvdh left a comment

generally looks good, but a few suggestions for cleanups here.

@@ -218,6 +220,52 @@ def _local_membership_update(
newly_joined = prev_member_event.membership != Membership.JOIN
if newly_joined:
yield self._user_joined_room(target, room_id)

# Copy over direct message status and room tags if this is a join

This comment has been minimized.

@richvdh

richvdh Jan 24, 2019

Member

please can this stuff move out to a separate function? as a rule of thumb, try to keep functions within a screenful.

return
create_event = yield self.store.get_event(create_id)

if "predecessor" in create_event["content"]:

This comment has been minimized.

@richvdh

richvdh Jan 24, 2019

Member

presumably we can use the method from the search PR for this

This comment has been minimized.

@anoadragon453

anoadragon453 Jan 25, 2019

Author Member

Ah, we can but it's inside SearchHandler:

def get_old_rooms_from_upgraded_room(self, room_id):
"""Retrieves room IDs of old rooms in the history of an upgraded room.
We do so by checking the m.room.create event of the room for a
`predecessor` key. If it exists, we add the room ID to our return
list and then check that room for a m.room.create event and so on
until we can no longer find any more previous rooms.
The full list of all found rooms in then returned.
Args:
room_id (str): id of the room to search through.
Returns:
Deferred[iterable[unicode]]: predecessor room ids
"""

Shall I move it to the store instead?

This comment has been minimized.

@richvdh

richvdh Jan 28, 2019

Member

I was thinking specifically of get_room_predecessor, which is in the Store I think?

This comment has been minimized.

@anoadragon453

anoadragon453 Jan 28, 2019

Author Member

Oh, so it is. Will add, thanks.

user_account_data = yield self.store.get_account_data_for_user(
user_id,
)
room_tags = yield self.store.get_tags_for_room(

This comment has been minimized.

@richvdh

richvdh Jan 24, 2019

Member

feels weird when this is up here and not used until later on

break

# Copy room tags if applicable
if room_tags:

This comment has been minimized.

@richvdh

richvdh Jan 24, 2019

Member

think this condition is redundant? get_tags_for_room always returns a dict, even if it's empty.

old_room_id = create_event["content"]["predecessor"]["room_id"]

# Retrieve room account data for predecessor room
user_account_data = yield self.store.get_account_data_for_user(

This comment has been minimized.

@richvdh

richvdh Jan 24, 2019

Member

given you just want the first item in the tuple you can write:

user_account_data, _ = yield ...

which then saves the slightly cryptic [0] indexes later

This comment has been minimized.

@anoadragon453

anoadragon453 Jan 25, 2019

Author Member

Ohh, nice one. I love destructuring.

)

# Copy direct message state if applicable
if user_account_data and "m.direct" in user_account_data[0]:

This comment has been minimized.

@richvdh

richvdh Jan 24, 2019

Member

suggest you do direct_rooms = user_account_data.get("m.direct", {}) and you can drop these conditions.

HOWEVER it may be worth sanity-checking that nobody has decided to make m.direct a list or something else other than a dict, so if isinstance(direct_rooms, dict).

# Copy room tags if applicable
if room_tags:
# Copy each room tag to the new room
for tag in room_tags.keys():

This comment has been minimized.

@richvdh

richvdh Jan 24, 2019

Member

for tag, tag_content in room_tags.items():

This comment has been minimized.

@anoadragon453

anoadragon453 Jan 25, 2019

Author Member

Oof, I think I got lost in thinking how the tag data was structured to realize how clunky that code was.

Thanks for pointing it out.

@anoadragon453 anoadragon453 requested review from matrix-org/synapse-core and removed request for matrix-org/synapse-core Jan 25, 2019

@richvdh
Copy link
Member

richvdh left a comment

lgtm otherwise

user_id (str)
Returns:
Deferred|None

This comment has been minimized.

@richvdh

richvdh Jan 28, 2019

Member

ITYM Deferred[None] ?

This comment has been minimized.

@anoadragon453

anoadragon453 Jan 28, 2019

Author Member

Sure, it was just copied from another method. We seem to have several different ways of writing the same thing spread across different methods?

This comment has been minimized.

@richvdh

richvdh Jan 28, 2019

Member

Deferred|None means: "either a Deferred (of unspecified value type), or None", which is different to what your method does

This comment has been minimized.

@anoadragon453

anoadragon453 Jan 28, 2019

Author Member

Ahh, I see, thank you.

@anoadragon453 anoadragon453 merged commit 88f4df8 into develop Jan 28, 2019

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@anoadragon453 anoadragon453 deleted the anoa/dm_room_upgrade branch Jan 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment