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

Correctly check the reception of a remote feedback #27430

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Jun 8, 2021

On the creation of a federated share, the receiving server wrongly check the notification response which lead to sending a notification twice.

emitting server receiving server
send share --> _
_ --> receive share
_ --- user accept share
_ <-- send accepted_share notification
receive notification <--- -
_ --- check the result of the notification, it's an array and not true
_ <-- assume the request failed and send the notification to the legacy endpoint
receive duplicate notification <--- _

Here is an example of duplicate notification:

Screenshot from 2021-06-08 19-01-25

@MichaIng
Copy link
Member

MichaIng commented Jun 8, 2021

It looks like one test need to be changed:

  1. OCA\Files_Sharing\Tests\External\ManagerTest::testAddShare
    Expectation failed for method name is "newClient" when invoked at sequence index 0.
    The expected invocation at index 0 was never reached.

testAddShare uses method newClient after acceptShare which calls the changed sendFeedbackToRemote. Not if or how this makes the test fail, but this is the only chain that I could find 😄.

@artonge
Copy link
Contributor Author

artonge commented Jun 8, 2021

testAddShare uses method newClient after acceptShare which calls the changed sendFeedbackToRemote. Not if or how this makes the test fail, but this is the only chain that I could find smile.

Thanks, I was on it, but I am not very familiar with phpunit, so this helps :)

@MichaIng
Copy link
Member

MichaIng commented Jun 8, 2021

Ah, a bid later declineShare is called, which also calls sendFeedbackToRemote of course. With this commit, this functions returns true in more cases, so the remote get's a positive feedback when the test expects a negative one? With such a long function and multiple same method calls, a little more verbosity of the test would be helpful, like a line number 😄.

OOT: For this test #27414 applies as well.

@artonge artonge force-pushed the fix/feedback_response_check branch from f97bcc9 to 2bad5b8 Compare June 9, 2021 10:39
@artonge artonge force-pushed the fix/feedback_response_check branch 2 times, most recently from 652428a to 2f2127c Compare June 9, 2021 12:43
@artonge artonge force-pushed the fix/feedback_response_check branch from 2f2127c to 508aa09 Compare June 9, 2021 13:27
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the fix/feedback_response_check branch from 508aa09 to ebf35fb Compare June 9, 2021 13:37
@blizzz blizzz merged commit bb2b946 into master Jun 9, 2021
@blizzz blizzz deleted the fix/feedback_response_check branch June 9, 2021 18:46
@MorrisJobke MorrisJobke mentioned this pull request Jun 10, 2021
59 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants