Skip to content

fix accumulated sync summaries - andy review #3367

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 16, 2023
Merged

fix accumulated sync summaries - andy review #3367

merged 3 commits into from
May 16, 2023

Conversation

@ara4n
Copy link
Member

@ara4n ara4n commented May 16, 2023 •
edited by github-actions bot
Loading

incorporate andy's review from #3366 (review)

This change is marked as an internal change (Task), so will not be included in the changelog.

ara4n added 3 commits May 16, 2023 11:34
@ara4n
correctly accumulate sync summaries.
18722d0 
if a sync summary for (say) invited_member_count goes from 1 to 0, it should be
accumluated as 0, rather than 1.

Should fix element-hq/element-web#23345
@ara4n
incorporate andy review
0c5eb27
@ara4n
merge
1cd8ea6
@ara4n ara4n requested a review from a team as a code owner May 16, 2023 11:08
@ara4n ara4n requested review from andybalaam and t3chguy May 16, 2023 11:08
@ara4n ara4n added the T-Other Questions, user support, anything else label May 16, 2023
@ara4n
Copy link
Member Author

ara4n commented May 16, 2023

going to force-merge given this doesn't need review given it's just incorporating existing review

@ara4n ara4n merged commit 6dc4a62 into develop May 16, 2023
@ara4n ara4n deleted the matthew/fix-accumulated-sync-summaries branch May 16, 2023 11:34
@t3chguy
Copy link
Member

t3chguy commented May 16, 2023 •
edited
Loading

@ara4n please avoid force merging in future as it breaks anything in the merge queue due to the base branch changing out from under it, with potential for things to fall in the cracks for community PRs as we might not notice it falling out of the merge queue do to it. Or in this case causing the RC to be delayed due to breaking CI on the develop branch.

@t3chguy t3chguy mentioned this pull request May 16, 2023
Merged
@richvdh
Copy link
Member

richvdh commented May 16, 2023

@ara4n ... and if you must force-merge, please at least wait for CI to complete.

going to force-merge given this doesn't need review given it's just incorporating existing review

where did this review take place?

@ara4n
Copy link
Member Author

ara4n commented May 16, 2023

@ara4n please avoid force merging in future

ack

where did this review take place?

#3366 - i'm not making it up......

andybalaam
andybalaam reviewed May 16, 2023
View reviewed changes
Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Approved.

@richvdh
Copy link
Member

richvdh commented May 16, 2023

#3366 - i'm not making it up......

I'm sure you're not, but it's helpful to keep a record to help future investigations.

Thanks for clarifying.

@t3chguy t3chguy added T-Task Tasks for the team like planning and removed T-Other Questions, user support, anything else labels Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andybalaam andybalaam andybalaam left review comments

@t3chguy t3chguy Awaiting requested review from t3chguy t3chguy is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees

No one assigned

Labels

T-Task Tasks for the team like planning

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ara4n @t3chguy @richvdh @andybalaam