Skip to content
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

Create & use filter with enabled LL when testing room summaries #553

Closed
wants to merge 2 commits into from

Conversation

S7evinK
Copy link
Contributor

@S7evinK S7evinK commented Nov 7, 2022

Synapse seems to need a filter with enabled lazy_load_members to add room summaries. This now creates that filter and uses it in /sync requests.

@S7evinK S7evinK requested review from a team as code owners November 7, 2022 06:10
@S7evinK S7evinK removed the request for review from a team November 7, 2022 06:19
@reivilibre
Copy link
Contributor

Synapse seems to need a filter with enabled lazy_load_members to add room summaries.

maybe a more pressing question: but should it?

It feels like we should ensure the spec is clear about what the requirements are here — it smells wrong if Dendrite and Synapse don't do the same thing

@S7evinK
Copy link
Contributor Author

S7evinK commented Nov 9, 2022

As @ShadowJonathan mentioned in matrix-org/matrix-spec#1325, the spec very vague about this field.

Dendrite adds room summaries if there are membership changes, not only if a LL filter is supplied.

Synapse has this, so is only adding the field if it's an not "out of band" AND LL is enable and there are membership changes. (Q: What is "out of band" in this case?)

@ShadowJonathan
Copy link
Contributor

For the record, I would really not add anything in complement that cannot be backed up by wording in the spec, and I hope @kegsay can agree with that.

@reivilibre
Copy link
Contributor

yeah, if the spec is too vague to be useful then, in my opinion, it'd be nice to feed some rigidity back into the spec.
With that said, I appreciate the spec process can be extremely slow. Maybe worth raising in #matrix-spec anyway? Possibly it could be settled with a 'clarification' by documenting existing expectations. At least having some sort of PR open (or failing that, an issue link that can be added to the test) for the spec would be a good start to support this change to the test.

@squahtx
Copy link
Contributor

squahtx commented Nov 23, 2022

Going to remove this from the Synapse review queue until we have more clarity on the spec side. At the very least, some sort of spec PR or issue to link against would be good, as suggested by @reivilibre.

@squahtx squahtx removed the request for review from a team November 23, 2022 18:40
@kegsay
Copy link
Member

kegsay commented Sep 6, 2024

Closing as stale.

@kegsay kegsay closed this Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked stale This issue or PR is old and may be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants