Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Transfer user's push rules on room upgrade #4838

Merged
merged 12 commits into from
Mar 12, 2019

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Mar 8, 2019

Transfer's push rules (notification settings) on room upgrade.

Closes #4597.

TODO:

@codecov
Copy link

codecov bot commented Mar 11, 2019

Codecov Report

Merging #4838 into develop will decrease coverage by 7.62%.
The diff coverage is 40%.

@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
- Coverage    75.34%   67.72%   -7.63%     
===========================================
  Files          340      340              
  Lines        34920    34940      +20     
  Branches      5717     5725       +8     
===========================================
- Hits         26312    23664    -2648     
- Misses        6999     9619    +2620     
- Partials      1609     1657      +48

@codecov
Copy link

codecov bot commented Mar 11, 2019

Codecov Report

Merging #4838 into develop will increase coverage by 0.04%.
The diff coverage is 87.5%.

@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
+ Coverage    75.31%   75.35%   +0.04%     
===========================================
  Files          340      340              
  Lines        34924    34940      +16     
  Branches      5718     5724       +6     
===========================================
+ Hits         26302    26329      +27     
+ Misses        7009     6997      -12     
- Partials      1613     1614       +1

@anoadragon453 anoadragon453 marked this pull request as ready for review March 11, 2019 15:36
@anoadragon453 anoadragon453 requested a review from a team March 11, 2019 17:28
synapse/storage/push_rule.py Outdated Show resolved Hide resolved
synapse/storage/push_rule.py Outdated Show resolved Hide resolved
synapse/storage/push_rule.py Outdated Show resolved Hide resolved
synapse/storage/push_rule.py Outdated Show resolved Hide resolved
@anoadragon453 anoadragon453 dismissed richvdh’s stale review March 11, 2019 18:20

Cleaned up and stopped using push_rule ID

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

getting there :)

synapse/storage/push_rule.py Outdated Show resolved Hide resolved
@@ -185,6 +185,63 @@ def bulk_get_push_rules(self, user_ids):

defer.returnValue(results)

@defer.inlineCallbacks
def copy_push_rule_from_room_to_room(
Copy link
Member

Choose a reason for hiding this comment

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

s/copy/move/. Likewise in the docstring

synapse/storage/push_rule.py Outdated Show resolved Hide resolved
# delete them from the old room
for rule in user_push_rules:
for condition in rule["conditions"]:
if "key" in condition and condition["key"] == "room_id":
Copy link
Member

Choose a reason for hiding this comment

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

How about simply:

Suggested change
if "key" in condition and condition["key"] == "room_id":
if condition.get("key") == "room_id":

synapse/storage/push_rule.py Outdated Show resolved Hide resolved
synapse/storage/push_rule.py Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

# Get rules relating to the old room, move them to the new room, then
# delete them from the old room
for rule in user_push_rules:
for condition in rule["conditions"]:
Copy link
Member

Choose a reason for hiding this comment

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

conditions is apparently optional:

Suggested change
for condition in rule["conditions"]:
for condition in rule.get("conditions", []):

# delete them from the old room
for rule in user_push_rules:
for condition in rule["conditions"]:
if condition.get("key") == "room_id":
Copy link
Member

Choose a reason for hiding this comment

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

we probably need to worry about what happens if somebody makes a rule that has two conditions that match: I suspect your code will blow up.

an easy fix might be:

conditions = rule.get("conditions", [])
if any((c.get("key") == "room_id" and c.get("pattern") == old_room_id) for c in conditions):
    # ... move

Copy link
Member Author

Choose a reason for hiding this comment

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

This works and is very pythonic, thanks.

@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #4838 into develop will increase coverage by 0.04%.
The diff coverage is 87.5%.

@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
+ Coverage    75.31%   75.35%   +0.04%     
===========================================
  Files          340      340              
  Lines        34924    34940      +16     
  Branches      5718     5724       +6     
===========================================
+ Hits         26302    26329      +27     
+ Misses        7009     6997      -12     
- Partials      1613     1614       +1

1 similar comment
@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #4838 into develop will increase coverage by 0.04%.
The diff coverage is 87.5%.

@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
+ Coverage    75.31%   75.35%   +0.04%     
===========================================
  Files          340      340              
  Lines        34924    34940      +16     
  Branches      5718     5724       +6     
===========================================
+ Hits         26302    26329      +27     
+ Misses        7009     6997      -12     
- Partials      1613     1614       +1

@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #4838 into develop will increase coverage by 0.04%.
The diff coverage is 87.5%.

@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
+ Coverage    75.31%   75.35%   +0.04%     
===========================================
  Files          340      340              
  Lines        34924    34940      +16     
  Branches      5718     5724       +6     
===========================================
+ Hits         26302    26329      +27     
+ Misses        7009     6997      -12     
- Partials      1613     1614       +1

@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #4838 into develop will increase coverage by 0.03%.
The diff coverage is 93.33%.

@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
+ Coverage    75.31%   75.35%   +0.03%     
===========================================
  Files          340      340              
  Lines        34924    34939      +15     
  Branches      5718     5722       +4     
===========================================
+ Hits         26302    26327      +25     
+ Misses        7009     7000       -9     
+ Partials      1613     1612       -1

1 similar comment
@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #4838 into develop will increase coverage by 0.03%.
The diff coverage is 93.33%.

@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
+ Coverage    75.31%   75.35%   +0.03%     
===========================================
  Files          340      340              
  Lines        34924    34939      +15     
  Branches      5718     5722       +4     
===========================================
+ Hits         26302    26327      +25     
+ Misses        7009     7000       -9     
+ Partials      1613     1612       -1

@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #4838 into develop will increase coverage by 0.03%.
The diff coverage is 93.33%.

@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
+ Coverage    75.31%   75.35%   +0.03%     
===========================================
  Files          340      340              
  Lines        34924    34939      +15     
  Branches      5718     5722       +4     
===========================================
+ Hits         26302    26327      +25     
+ Misses        7009     7000       -9     
+ Partials      1613     1612       -1

2 similar comments
@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #4838 into develop will increase coverage by 0.03%.
The diff coverage is 93.33%.

@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
+ Coverage    75.31%   75.35%   +0.03%     
===========================================
  Files          340      340              
  Lines        34924    34939      +15     
  Branches      5718     5722       +4     
===========================================
+ Hits         26302    26327      +25     
+ Misses        7009     7000       -9     
+ Partials      1613     1612       -1

@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #4838 into develop will increase coverage by 0.03%.
The diff coverage is 93.33%.

@@             Coverage Diff             @@
##           develop    #4838      +/-   ##
===========================================
+ Coverage    75.31%   75.35%   +0.03%     
===========================================
  Files          340      340              
  Lines        34924    34939      +15     
  Branches      5718     5722       +4     
===========================================
+ Hits         26302    26327      +25     
+ Misses        7009     7000       -9     
+ Partials      1613     1612       -1

@anoadragon453 anoadragon453 merged commit d42c81d into develop Mar 12, 2019
@anoadragon453 anoadragon453 deleted the anoa/notification_settings_room_upgrade branch March 12, 2019 14:43
@anoadragon453 anoadragon453 changed the title Transfer local user's push rules on room upgrade Transfer user's push rules on room upgrade Jun 19, 2019
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