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.

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

[FS-1334] Reset a Remote Subconversation #2964

Merged
merged 7 commits into from
Jan 13, 2023
Merged

[FS-1334] Reset a Remote Subconversation #2964

merged 7 commits into from
Jan 13, 2023

Conversation

mdimjasevic
Copy link
Contributor

@mdimjasevic mdimjasevic commented Jan 6, 2023 •
edited
Loading

As part of https://wearezeta.atlassian.net/browse/FS-1334, this PR adds support for resetting a remote subconversation. The local case was done in PR #2956.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@mdimjasevic mdimjasevic temporarily deployed to cachix January 6, 2023 14:56 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 6, 2023 14:56 — with GitHub Actions Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jan 6, 2023
@mdimjasevic mdimjasevic changed the base branch from develop to mls January 6, 2023 14:59
@mdimjasevic mdimjasevic changed the base branch from mls to fs-1334/reset-subconversation January 6, 2023 14:59
@mdimjasevic mdimjasevic temporarily deployed to cachix January 6, 2023 15:55 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 6, 2023 15:55 — with GitHub Actions Inactive
@smatting
Copy link
Contributor

smatting commented Jan 9, 2023

Closing this in favor of #2956

@smatting smatting closed this Jan 9, 2023
@smatting smatting reopened this Jan 9, 2023
@smatting smatting temporarily deployed to cachix January 9, 2023 09:13 — with GitHub Actions Inactive
@smatting smatting temporarily deployed to cachix January 9, 2023 09:13 — with GitHub Actions Inactive
@smatting
Copy link
Contributor

smatting commented Jan 9, 2023

Err nevermind

Base automatically changed from fs-1334/reset-subconversation to mls January 9, 2023 10:13
@mdimjasevic
Support resetting remote subconversations
a0c531c
@mdimjasevic
Tests: delete a remote subconversation
2a4e43c 
- These tests are parameterised by whether the user is a member of the
subconversation
@mdimjasevic mdimjasevic force-pushed the fs-1334/reset-remote-subconversation branch from 983a88a to 2a4e43c Compare January 9, 2023 10:25
@mdimjasevic mdimjasevic temporarily deployed to cachix January 9, 2023 10:25 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 9, 2023 10:25 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 9, 2023 13:53 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 9, 2023 13:53 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic marked this pull request as ready for review January 9, 2023 13:53
@mdimjasevic mdimjasevic temporarily deployed to cachix January 9, 2023 14:30 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 9, 2023 14:30 — with GitHub Actions Inactive
pcapriotti
pcapriotti approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@pcapriotti pcapriotti 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. However, the test coverage is a bit strange. If I'm not mistaken, we're testing the failure case of a non-member attempting to reset only in the remote case.

Another question: is a client that is not part of a subconversation allowed to reset it?

@mdimjasevic mdimjasevic temporarily deployed to cachix January 13, 2023 10:32 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 13, 2023 10:32 — with GitHub Actions Inactive
@mdimjasevic
Copy link
Contributor Author

mdimjasevic commented Jan 13, 2023 •
edited
Loading

Looks good. However, the test coverage is a bit strange. If I'm not mistaken, we're testing the failure case of a non-member attempting to reset only in the remote case.

A fair point. I just added a test for the local case.

Another question: is a client that is not part of a subconversation allowed to reset it?

My understanding is that a client is allowed to reset a subconversation so long as they're a member of the parent conversation. They don't necessarily have to be in the subconversation.

@mdimjasevic mdimjasevic merged commit 46fcd12 into mls Jan 13, 2023
@mdimjasevic mdimjasevic deleted the fs-1334/reset-remote-subconversation branch January 13, 2023 13:17
@stefanwire stefanwire mentioned this pull request Jan 17, 2023
Merged
9 tasks
This was referenced Oct 19, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcapriotti pcapriotti pcapriotti approved these changes

@smatting smatting Awaiting requested review from smatting

Assignees
No one assigned
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mdimjasevic @smatting @pcapriotti @zebot