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-1191] List the MLS Self-conversation Automatically #2856

Merged
merged 13 commits into from
Nov 23, 2022
Merged

[FS-1191] List the MLS Self-conversation Automatically #2856

merged 13 commits into from
Nov 23, 2022

Conversation

mdimjasevic
Copy link
Contributor

@mdimjasevic mdimjasevic commented Nov 21, 2022 •
edited
Loading

Automatically list the MLS self-conversation in conversation-listing endpoints:

  • POST /conversations/list-ids

Tracked by https://wearezeta.atlassian.net/browse/FS-1191.

Checklist

  • Make the change only to v3 of the POST /conversation/list-ids endpoint
  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@mdimjasevic mdimjasevic temporarily deployed to cachix November 21, 2022 15:56 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 21, 2022 15:56 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Nov 21, 2022
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 09:41 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 09:41 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 12:25 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 12:25 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 17:20 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 17:20 Inactive
@mdimjasevic mdimjasevic force-pushed the fs-925/no-need-to-call-get-endpoint branch from c295d3f to 81a40f5 Compare November 22, 2022 17:21
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 17:21 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 17:21 Inactive
@mdimjasevic mdimjasevic marked this pull request as draft November 22, 2022 17:22
@mdimjasevic mdimjasevic force-pushed the fs-925/no-need-to-call-get-endpoint branch from 81a40f5 to ff3c901 Compare November 22, 2022 21:26
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 21:26 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 21:26 Inactive
@mdimjasevic mdimjasevic force-pushed the fs-925/no-need-to-call-get-endpoint branch from ff3c901 to 3ae7a41 Compare November 22, 2022 21:30
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 21:30 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 21:30 Inactive
@mdimjasevic mdimjasevic force-pushed the fs-925/no-need-to-call-get-endpoint branch from 3ae7a41 to c5c294c Compare November 22, 2022 21:35
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 21:35 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 22, 2022 21:35 Inactive
@mdimjasevic mdimjasevic marked this pull request as ready for review November 22, 2022 21:35
smatting
smatting reviewed Nov 23, 2022
View reviewed changes
Copy link
Contributor

@smatting smatting left a comment

Choose a reason for hiding this comment

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

The deprecated GET /conversations/ids list-conversation-ids-unqualified endpoint is not updated by this PR.
Rather than also updating it maybe we can remove it for V3 in this PR?

@mdimjasevic mdimjasevic temporarily deployed to cachix November 23, 2022 09:45 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 23, 2022 09:45 Inactive
@mdimjasevic
Copy link
Contributor Author

The deprecated GET /conversations/ids list-conversation-ids-unqualified endpoint is not updated by this PR. Rather than also updating it maybe we can remove it for V3 in this PR?

Those two old endpoints are not even touched by this PR, are they? And I'd say that given the scope of this ticket (only the list-ids endpoint) your suggestion should make for a separate PR.

@mdimjasevic mdimjasevic temporarily deployed to cachix November 23, 2022 09:50 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 23, 2022 09:50 Inactive
@smatting
Copy link
Contributor

smatting commented Nov 23, 2022 •
edited
Loading

Those two old endpoints are not even touched by this PR, are they? And I'd say that given the scope of this ticket (only the list-ids endpoint) your suggestion should make for a separate PR.

I'm suggesting that the scope of the ticket is not big enough. If we don't update both endpoints together they are inconsistent: that sure can't be right.

@mdimjasevic mdimjasevic temporarily deployed to cachix November 23, 2022 13:08 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 23, 2022 13:08 Inactive
@mdimjasevic
Make tests for GET /conversations/ids use v2
b6e4dd7 
- The endpoint is removed starting v3, and tests by default use the
latest version, which is currently v3
@mdimjasevic mdimjasevic temporarily deployed to cachix November 23, 2022 14:29 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix November 23, 2022 14:29 Inactive
@mdimjasevic mdimjasevic merged commit 58604d7 into develop Nov 23, 2022
@mdimjasevic mdimjasevic deleted the fs-925/no-need-to-call-get-endpoint branch November 23, 2022 15:53
This was referenced Nov 25, 2022
Closed
Closed
Closed
Merged
Merged
@zebot zebot mentioned this pull request Jan 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smatting smatting smatting left review comments

@elland elland elland left review comments

@pcapriotti pcapriotti pcapriotti approved these changes

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.
5 participants
@mdimjasevic @smatting @pcapriotti @elland @zebot