-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
synapse/handlers/user_directory.py
Outdated
None if the field in the events either both match `public_value` o | ||
neither do, i.e. there has been no change. | ||
True if it didnt match `public_value` but now does | ||
Falsse if it did match `public_value` but now doesn't |
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.
Falsse
synapse/handlers/user_directory.py
Outdated
`shared` to `world_readable` (`public_value`). | ||
|
||
Returns: | ||
None if the field in the events either both match `public_value` o |
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.
o
synapse/handlers/user_directory.py
Outdated
defer.returnValue(None) | ||
|
||
prev_hist_vis = None | ||
hist_vis = None |
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.
hist_vis
synapse/handlers/user_directory.py
Outdated
if change: | ||
yield self._handle_new_user(room_id, user_id, profile) | ||
else: | ||
yield self._handle_remove_user(room_id, user_id) |
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.
It looks like lines 204-221 are duplicates of 172-197. Maybe lift them into a function?
avatar_url TEXT | ||
); | ||
|
||
CREATE INDEX user_directory_room_idx ON user_directory(room_id); |
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.
Maybe add a UNIQUE index.
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.
Probably not on room_id, no.
But I've added one to user_id.
WHERE stream_id > ? | ||
GROUP BY stream_id | ||
ORDER BY stream_id ASC | ||
LIMIT 100 |
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.
Why 100?
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.
Done
c6942b7
to
036362e
Compare
synapse/handlers/user_directory.py
Outdated
|
||
Returns: | ||
None if the field in the events either both match `public_value` | ||
neither do, i.e. there has been no change. |
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.
Should this be "or if neither do" ?
- Check if room is public when a user joins before adding to user dir - Fix typo of field name "content.join_rules" -> "content.join_rule"
elif value != public_value and prev_value == public_value: | ||
defer.returnValue(False) | ||
else: | ||
defer.returnValue(None) |
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 could write this as:
old_value_is_public = prev_value == public_value
new_value_is_public = value == public_value
if old_value_is_public == new_value_is_public:
defer.returnValue(None)
else:
defer.returnValue(new_value_is_public)
I'm not sure if it is clearer or not.
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 don't find it any more readable tbh
synapse/handlers/user_directory.py
Outdated
prev_value = prev_event.content.get(key_name, None) | ||
|
||
if event: | ||
value = event.content.get(key_name, None) |
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.
The None
is redundant.
0d13a1f
to
d5477c7
Compare
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.
Other than the possibly spurious test failures LGTM?
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.
Still LGTM?
No description provided.