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

Add room members admin endpoint #7842

Merged
merged 8 commits into from
Jul 16, 2020

Conversation

awesome-michael
Copy link
Contributor

Signed-off-by: Michael Albert [email protected]

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Looks good overall! Just had a couple of small comments.

tests/rest/admin/test_room.py Outdated Show resolved Hide resolved
docs/admin_api/rooms.md Show resolved Hide resolved
synapse/rest/admin/rooms.py Outdated Show resolved Hide resolved
changelog.d/7842.feature Outdated Show resolved Hide resolved
@clokep clokep added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jul 14, 2020
synapse/rest/admin/rooms.py Outdated Show resolved Hide resolved
@clokep clokep removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jul 16, 2020
@clokep clokep self-requested a review July 16, 2020 19:34
Comment on lines 250 to 251
if not members:
raise NotFoundError("Room not found or empty")
Copy link
Member

Choose a reason for hiding this comment

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

After playing with this a bit more, I think this is going to be confusing, I think we should add use the self.store.get_room(room_id) method first, if that returns None we can raise NotFoundError.

Then, if there are no members I think we should return it as a success: {"members": [], "total": 0}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a very good suggestion. I'll add the check.

Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Looks good to me! We'll merge this once CI passes.

@clokep clokep merged commit fff483e into matrix-org:develop Jul 16, 2020
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit 'de119063f': (31 commits)
  Convert room list handler to async/await. (#7912)
  Element CSS and logo in email templates (#7919)
  Lint the contrib/ directory in CI and linting scripts, add synctl to linting script (#7914)
  Remove unused code from synapse.logging.utils. (#7897)
  Fix a typo in the sample config. (#7890)
  Fix deprecation warning: import ABC from collections.abc (#7892)
  Change sample config's postgres user to synapse_user (#7889)
  Fix deprecation warning due to invalid escape sequences (#7895)
  Remove Ubuntu Eoan that is now EOL (#7888)
  Fix the trace function for async functions. (#7872)
  Add help for creating a user via docker (#7885)
  Switch to Debian:Slim from Alpine for the docker image (#7839)
  Stop using 'device_max_stream_id' (#7882)
  Fix TypeError in synapse.notifier (#7880)
  Add a default limit (of 100) to get/sync operations. (#7858)
  Change "unknown room ver" logging to warning. (#7881)
  Convert device handler to async/await (#7871)
  Convert synapse.app to async/await. (#7868)
  Convert _base, profile, and _receipts handlers to async/await (#7860)
  Add admin endpoint to get members in a room. (#7842)
  ...
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