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

Move federated_share_added into a typed event #21814

Merged

Conversation

MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Jul 13, 2020

This only needs documentation. Should I just add it to https://docs.nextcloud.com/server/19/developer_manual/app/events.html#available-events ?

Also should those events be in the apps or in OCP?

@MorrisJobke MorrisJobke added this to the Nextcloud 20 milestone Jul 13, 2020
@MorrisJobke MorrisJobke force-pushed the techdebt/noid/federated_share_added-into-typed-event branch from 8ccff5e to 089e72b Compare July 14, 2020 02:58
@ChristophWurst
Copy link
Member

Also should those events be in the apps or in OCP?

How clean are we with DAV related code? If other abstractions are in OCP I would vote for that. If there is more stuff that DAV apps use from the dav app directly then we could also just follow that pattern and leave in in dav.

@rullzer
Copy link
Member

rullzer commented Jul 14, 2020

Well we also have events emitted by the files app and by the sharing app. The issue is you get this weird mix if we move them to OCP.

I'd say with the new events it is fine to have them in the apps themselves. That is where they belong.
Also it should all be fine since you use the ::class to get the event so even if you don'thave the app (not possible in this case) it should still work.

@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Jul 15, 2020
@MorrisJobke MorrisJobke force-pushed the techdebt/noid/federated_share_added-into-typed-event branch from 3632ae4 to 3587923 Compare July 15, 2020 11:18
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Didn't test but looks sane!

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke force-pushed the techdebt/noid/federated_share_added-into-typed-event branch from f0cf6ad to 0763a17 Compare July 23, 2020 06:33
@MorrisJobke
Copy link
Member Author

Time to squash the fixups. Ready to get in :)

@nextcloud nextcloud deleted a comment from faily-bot bot Jul 23, 2020
@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 23, 2020
@faily-bot
Copy link

faily-bot bot commented Jul 23, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 31002: failure

postgres11-php7.2

Show full log
There were 8 warnings:

1) Test\DB\DBSchemaTest::testSchema
Using assertContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringContainsString() or assertStringContainsStringIgnoringCase() instead.

2) OCA\DAV\Tests\unit\CalDAV\CalDavBackendTest::testCalendarQuery with data set "all" (array(0, 1, 2, 3), array(), array())
The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.

3) OCA\DAV\Tests\unit\CalDAV\CalDavBackendTest::testCalendarQuery with data set "only-todos" (array(), array('VTODO'), array())
The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.

4) OCA\DAV\Tests\unit\CalDAV\CalDavBackendTest::testCalendarQuery with data set "only-events" (array(0, 1, 2, 3), array(), array(array('VEVENT', false, array(), array(null, null), array())))
The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.

5) OCA\DAV\Tests\unit\CalDAV\CalDavBackendTest::testCalendarQuery with data set "start" (array(1, 2, 3), array(), array(array('VEVENT', false, array(), array(DateTime Object (...), null), array())))
The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.

6) OCA\DAV\Tests\unit\CalDAV\CalDavBackendTest::testCalendarQuery with data set "end" (array(0), array(), array(array('VEVENT', false, array(), array(null, DateTime Object (...)), array())))
The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.

7) OCA\DAV\Tests\unit\CalDAV\CalDavBackendTest::testCalendarQuery with data set "future" (array(3), array(), array(array('VEVENT', false, array(), array(DateTime Object (...), null), array())))
The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.

8) OCA\FederatedFileSharing\Tests\FederatedShareProviderTest::testCreate
assertArraySubset() is deprecated and will be removed in PHPUnit 9.

--

There was 1 failure:

1) OCA\Files_Versions\Tests\VersioningTest::testRestoreMovedShare
File content has not changed
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'version 2'
+'version 1'

/drone/src/apps/files_versions/tests/VersioningTest.php:729

--

There was 1 risky test:

1) OCA\TwoFactorBackupCodes\Tests\Db\BackupCodeMapperTest::testInsertArgonEncryptedCodes
This test did not perform any assertions

/drone/src/apps/twofactor_backupcodes/tests/Db/BackupCodeMapperTest.php:115

integration-auth

  • build/integration/features/auth.feature:70
Show full log
  Scenario: using OCS anonymously                                                    # /drone/src/build/integration/features/auth.feature:70
[Thu Jul 23 06:36:54 2020] 127.0.0.1:42048 [404]: /ocs/v2.php/cloud/users/user0
[Thu Jul 23 06:36:54 2020] 127.0.0.1:42066 [200]: /ocs/v1.php/cloud/users
[Thu Jul 23 06:36:55 2020] 127.0.0.1:42096 [200]: /ocs/v1.php/cloud/users/user0
[Thu Jul 23 06:36:55 2020] 127.0.0.1:42114 [200]: /ocs/v2.php/cloud/users/user0
[Thu Jul 23 06:36:55 2020] 127.0.0.1:42134 [200]: /login
[Thu Jul 23 06:36:55 2020] 127.0.0.1:42138 [303]: /login
[Thu Jul 23 06:36:55 2020] 127.0.0.1:42154 [200]: /index.php/apps/files/
[Thu Jul 23 06:36:56 2020] 127.0.0.1:42158 [200]: /index.php/settings/personal/authtokens
[Thu Jul 23 06:36:56 2020] 127.0.0.1:42170 [200]: /index.php/settings/personal/authtokens/56
[Thu Jul 23 06:36:56 2020] 127.0.0.1:42172 [303]: /login
[Thu Jul 23 06:36:56 2020] 127.0.0.1:42180 [200]: /index.php/apps/files/
[Thu Jul 23 06:36:56 2020] 127.0.0.1:42182 [303]: /login
[Thu Jul 23 06:36:56 2020] 127.0.0.1:42198 [200]: /index.php/apps/files/
[Thu Jul 23 06:36:56 2020] 127.0.0.1:42204 [200]: /index.php/settings/personal/authtokens
[Thu Jul 23 06:36:56 2020] TypeError: Argument 11 passed to OCA\Files_Sharing\External\Manager::__construct() must be of the type string, null given, called in /drone/src/apps/files_sharing/lib/AppInfo/Application.php on line 98 at /drone/src/apps/files_sharing/lib/External/Manager.php#91
[Thu Jul 23 06:36:56 2020] 127.0.0.1:42206 [200]: /ocs/v1.php/apps/files_sharing/api/v1/remote_shares
    When requesting "/ocs/v1.php/apps/files_sharing/api/v1/remote_shares" with "GET" # FeatureContext::requestingWith()
    Then the OCS status code should be "997"                                         # FeatureContext::theOCSStatusCodeShouldBe()
      Notice: Trying to get property 'meta' of non-object in /drone/src/build/integration/features/bootstrap/BasicStructure.php line 153
[Thu Jul 23 06:36:56 2020] 127.0.0.1:42214 [200]: /ocs/v1.php/cloud/users/user0
[Thu Jul 23 06:36:57 2020] 127.0.0.1:42232 [404]: /ocs/v2.php/cloud/users/user0
[Thu Jul 23 06:36:57 2020] Login failed: 'user0' (Remote IP: '127.0.0.1')
[Thu Jul 23 06:36:57 2020] 127.0.0.1:42250 [401]: /remote.php/webdav/myFileToComment.txt
[Thu Jul 23 06:36:57 2020] 127.0.0.1:42252 [207]: /remote.php/dav/systemtags/
[Thu Jul 23 06:36:57 2020] Login failed: 'user0' (Remote IP: '127.0.0.1')
[Thu Jul 23 06:36:57 2020] 127.0.0.1:42262 [401]: /remote.php/webdav/myFileToTag.txt
[Thu Jul 23 06:36:57 2020] 127.0.0.1:42264 [404]: /remote.php/dav/addressbooks/users/admin/MyAddressbook
[Thu Jul 23 06:36:57 2020] 127.0.0.1:42286 [404]: /remote.php/dav/calendars/admin/MyCalendar

@MorrisJobke
Copy link
Member Author

Failures are the ones that fail regularly but not always. Thus merging and documenting this.

@MorrisJobke MorrisJobke merged commit ce314d9 into master Jul 23, 2020
@MorrisJobke MorrisJobke deleted the techdebt/noid/federated_share_added-into-typed-event branch July 23, 2020 19:42
@MorrisJobke
Copy link
Member Author

  • build/integration/features/auth.feature:70

This was actually a bug introduced here. Let me debug it.

@MorrisJobke MorrisJobke removed the pending documentation This pull request needs an associated documentation update label Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants