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

Add a user directory #2252

Merged
merged 34 commits into from
Jun 1, 2017
Merged

Add a user directory #2252

merged 34 commits into from
Jun 1, 2017

Conversation

erikjohnston
Copy link
Member

No description provided.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Falsse

`shared` to `world_readable` (`public_value`).

Returns:
None if the field in the events either both match `public_value` o
Copy link
Contributor

Choose a reason for hiding this comment

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

o

defer.returnValue(None)

prev_hist_vis = None
hist_vis = None
Copy link
Contributor

Choose a reason for hiding this comment

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

hist_vis

if change:
yield self._handle_new_user(room_id, user_id, profile)
else:
yield self._handle_remove_user(room_id, user_id)
Copy link
Contributor

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);
Copy link
Contributor

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.

Copy link
Member Author

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 100?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


Returns:
None if the field in the events either both match `public_value`
neither do, i.e. there has been no change.
Copy link
Contributor

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)
Copy link
Contributor

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.

Copy link
Member Author

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

prev_value = prev_event.content.get(key_name, None)

if event:
value = event.content.get(key_name, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

The None is redundant.

Copy link
Contributor

@NegativeMjark NegativeMjark left a 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?

Copy link
Contributor

@NegativeMjark NegativeMjark left a comment

Choose a reason for hiding this comment

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

Still LGTM?

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