Skip to content

Comments

FS-51 Report unavailable clients for Proteus messages#3097

Merged
mdimjasevic merged 16 commits intowireapp:developfrom
lepsa:fs-51-proteus-message-sending-errors
Mar 9, 2023
Merged

FS-51 Report unavailable clients for Proteus messages#3097
mdimjasevic merged 16 commits intowireapp:developfrom
lepsa:fs-51-proteus-message-sending-errors

Conversation

@lepsa
Copy link
Contributor

@lepsa lepsa commented Feb 21, 2023

Changing the return types to match the ticket.
Adding tests and fixing some logic errors.

Sending a Proteus message will no longer emit an error when the conversation owning server cannot contact federated servers to retrieve client lists. These unavailable users will be listed in the failed to send field of the response sent back to the client. Federation members that can be contacted will have messages fanned out to them.

Checklist

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

Changing the return types to match the ticket.
Adding tests and fixing some logic errors.
@elland elland added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Feb 21, 2023
Rewriting the test, basing it off an existing test that is almost what
is needed, and removing the prior test.
@mdimjasevic mdimjasevic self-requested a review February 27, 2023 10:12
@lepsa lepsa force-pushed the fs-51-proteus-message-sending-errors branch from c46520f to c1d3b77 Compare February 28, 2023 04:02
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Overall, this looks good, but there are some parts that are not that easy for me to grasp. It'd be great if we could go over the PR in a call!

]
)
]
expectedRedundant = expectedFailedToSend
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is deeClient a redundant recipient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is due to other tests expecting redundant clients under similar circumstances. If I add code to filter out users from the redundant field those tests will fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but I still don't understand why. It would also be beneficial for your understanding to get to the bottom of it.

@mdimjasevic
Copy link
Contributor

@lepsa , please take a look at the federation chat and the reasoning why this is a breaking API change. I'd be happy to pair with you tomorrow morning (Wednesday) should you need help with splitting an endpoint in wire-api into two, one for versions up to and including v2, and a new one for all versions from v3 and up. I also tried to incorporate your structure suggestion in Confluence: https://wearezeta.atlassian.net/wiki/spaces/CORE/pages/738557958/Offline+backend+handling+-+analysis+of+prekey+fetching+Proteus#Suggested-change

@lepsa lepsa force-pushed the fs-51-proteus-message-sending-errors branch from 5ce5921 to f21d92d Compare March 3, 2023 05:28
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Apart from moving unit tests into a unit test module, this looks good!

@mdimjasevic
Copy link
Contributor

Please move the unit tests and then merge. No need to wait for another approval from me.

@mdimjasevic mdimjasevic merged commit 06e61ea into wireapp:develop Mar 9, 2023
lepsa added a commit to lepsa/wire-server that referenced this pull request Nov 28, 2023
* FS-51 Report unavailable clients for Proteus messages

Changing the return types to match the ticket.
Adding tests and fixing some logic errors.

* testing changes. Reworking how failing federators are tested.

Rewriting the test, basing it off an existing test that is almost what
is needed, and removing the prior test.

* FS-51: Adding changes from PR review and more tests

* Updating tests

* FS-51: Moving unit tests to a better module

* FS-51: Formatting and linters

* FS-51: Updating nix with generate-local-nix-packages.sh

* FS-51: Fixing an error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants