Feature/version metadata #39126

pushnov-i · 2021-08-20T09:02:24Z

The file version author is now being saved in an appropriate timestamp-prefixed JSON file and displayed in the version list on PROPFIND request. Rename, copy and deletion are take into account.

Enable with config-flag: 'file_storage.save_version_author' => true, in config.php

CLAassistant · 2021-08-20T09:02:29Z

All committers have signed the CLA.

IljaN · 2021-08-20T09:40:36Z

You can fix codestyle by running: make test-php-style-fix
You can run stan and phan (static-analyzers) via: make test-php-phpstan and make test-php-phan

JammingBen

Added some comments 👍

Some indentations seem to be off, make test-php-style-fix should fix this.

apps/dav/lib/Meta/MetaFile.php

apps/files_trashbin/lib/Trashbin.php

apps/files_versions/lib/Storage.php

ownclouders · 2021-08-20T13:32:16Z

💥 Acceptance tests pipeline webUISharingExt1-latest-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31929/162/1

JammingBen · 2021-08-21T07:50:37Z

I'm not sure if we should add getUsername() to the overall file interface. This method is (only?) needed in the MetaFileVersionNode class. Look for example how this is done for getContentDispositionFileName() [1].

You could implement a dedicated interface for the getUsername() method, similar to IProvidesAdditionalHeaders. This has then to be implemented by your MetaFileVersionNode class [2]. Your getUsername() method in MetaFile would look something like:

public function getUsername() {
	if ($this->file instanceof \OCP\Files\IVersionAuthor) {
		return $this->file->getUsername();
	}
	return '';
}

[1] https://github.com/owncloud/core/blob/feature/version-metadata/apps/dav/lib/Meta/MetaFile.php#L116
[2] https://github.com/owncloud/core/blob/feature/version-metadata/lib/private/Files/Meta/MetaFileVersionNode.php#L42

About the failing JS test: As the UI now shows the author in the versions tab, you have to adjust the expected result in the tests: https://github.com/owncloud/core/blob/feature/version-metadata/apps/files_versions/tests/js/versionstabviewSpec.js#L71

Feel free to ask if you need any help with that 🙂

On a side note: I tried it on my local instance, but I only see a { where the author should be:

mrow4a · 2021-08-21T08:57:00Z

@hodyroff @pushnov-i what about making it optional in admin settings ? Just not to release new feature and reduce blast radius in case of bugs? It touches core functionality of sync/share.

I am bit worried what happens when one of that version “copy” commands fails in the middle. How consistent is the state?

What about expiring old versions ? Is this also handled (if relevant)?

What about acceptance tests here?

Unit tests covering major classes ?

Tested with encryption?

lib/public/Files/IProvidesVersionAuthor.php

pushnov-i · 2021-08-23T14:46:47Z

@phil-davis thank you, corrected the changes in the version for the interface.
@JammingBen bug fixed (double json encode and tested locally), also implemented everything via interface and adjusted the tests, the unit tests and js tests pass now new quick action feature prevent the drone from getting a successful build

@mrow4a not sure if this is related to the expiration, only retainVersions and getVersion methods from storage are affected, tests have been adjusted. regarding the optional feature, how it should appear on the frontend?

IljaN · 2021-08-23T15:13:14Z

There are 8 version-related unit-tests failing https://drone.owncloud.com/owncloud/core/31947/12/9
Click "Show all Lines" and scroll down to line 249. You can ignore the skipped tests.

You can run them locally by running make test-php-unit

IljaN · 2021-08-24T08:40:03Z

apps/files_trashbin/tests/StorageTest.php

@@ -352,7 +352,7 @@ public function testDeleteVersionsOfFolder() {

 		// check if versions are in trashbin
 		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions/' . $name . '/');
-		$this->assertCount(1, $results);
+		$this->assertCount(2, $results);


Could you check additionally for an explicit expected result value in the second element of $result here? Only if you don't need to jump trough too much hoops.

IljaN · 2021-08-24T08:40:08Z

apps/files_trashbin/tests/StorageTest.php

@@ -352,7 +352,7 @@ public function testDeleteVersionsOfFolder() {

 		// check if versions are in trashbin
 		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions/' . $name . '/');
-		$this->assertCount(1, $results);
+		$this->assertCount(2, $results);


Could you check additionally for an explicit expected result value in the second element of $result here? Only if you don't need to jump trough too much hoops.

Also in all following assertions.

@IljaN I did a check yesterday with xdebug and just checked once again now - for this specific case the trashbin dir contains two files first one is the file version with the timestamp and the second is the JSON file with metadata:

So if the second file was introduced trough your change we should maybe explicitly check for it. Maybe in a additional test.

Do you imply a completely separate test for the trash bin, or just an additional test if the second file is a JSON, has a valid name, and contains metadata within testDeleteVersionsOfFolder?

just an additional test if the second file is a JSON, has a valid name, and contains metadata within testDeleteVersionsOfFolder?

This ^

But the more the better ;)

apps/dav/lib/Meta/MetaPlugin.php

IljaN · 2021-08-24T08:51:04Z

@pushnov-i Looks good now, good work! 🕺 Just a few small nitpicks left.

mrow4a · 2021-08-24T11:34:28Z

@pushnov-i is there a unit/integration test covering backwards compatibility? How code/ui is behaving if there is no metadata-file ?

IljaN · 2021-08-24T11:37:06Z

@mrow4a wrote:
Tested with encryption?

This is also a point we should take in to account as the feature will be used first in an ownCloud instance with encryption.

phil-davis · 2021-08-24T12:04:21Z

@pushnov-i the CLA assistant still says that you have not "signed" the CLA. Please press the button to do that.

IljaN · 2021-08-24T12:07:12Z

@mrow4a @pushnov-i Just checked with @phil-davis. If we merge this all acceptance-tests will be run with encryption (master-key and user-key) as nightly-job. Results will be posted tomorrow by drone in #builds in chat.

@pushnov-i Please use the squash-button in github when merging to clean-up the commit history.

apps/files_trashbin/tests/StorageTest.php

phil-davis · 2021-08-24T12:30:49Z

I don't see any changelog. Did I miss it?

pushnov-i · 2021-08-24T14:04:16Z

@phil-davis added changelog, fixed space.
@IljaN I did not check the encryption since this requires installing an additional plugin, but if only the content of the files will be encrypted, this feature should not affect the workflow as long as the structure of the files within storage won't be changed. I did not manage to add additional tests yet... @mrow4a, if a JSON file is missing, no author will be shown on the UI side. On the code's side, within the storage, you can see that there are conditional checks everywhere, so if the JSON file is present, it will be considered, otherwise ignored.

phil-davis · 2021-08-24T14:38:18Z

@pushnov-i please click the link to sign the CLA at #39126 (comment)

Does it not work for you? Or?

pushnov-i · 2021-08-24T15:02:26Z

@pushnov-i please click the link to sign the CLA at #39126 (comment)

Does it not work for you? Or?

@phil-davis CLA requires me to change the author of one merge commit since it does not recognize it in github, working on it

sonarqubecloud · 2021-11-17T11:38:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

63.2% Coverage
0.0% Duplication

pmaier1 · 2021-11-17T12:39:38Z

The requirements need to be defined.

The requirement is to see the user who created a version (or: the user who uploaded/saved this state of a file) as part of the version metadata. Basically this enhancement shall allow to "check the last version that Peter created" or "roll back to the state before Peter started working on it".

Here's the original input https://github.com/owncloud/enterprise/issues/4518.

phil-davis · 2021-11-17T15:23:21Z

Good - now I think that I understand the requirement, and it seems a perfectly reasonable item of metadata about a file (and versions of a file).

The technical thing that I don't think has been implemented in this PR so far is that "the system" needs to know who "created" the current "version" of a file (i.e. the live current data in the file, that is not yet in the version history). That meta-data needs to be stored. For example, if Alice shares a folder with Brian, and Brian uploads "file.txt" into the shared folder, then "the system" needs to remember that the file "creator" is Brian.

Then the rest of the logic is simple - When someone changes the file, the code that moves the "now old" data to the version history has to also move the meta-data also to the version history. Then the new file data is written along with new meta-data about who just "created" the now-latest live "version" of the file.

IljaN · 2021-11-17T16:07:08Z

The technical thing that I don't think has been implemented in this PR so far is that "the system" needs to know who "created" the current "version" of a file

AFAIR this requirement was removed in a subsequent call => Creator can be checked via activity app. Will clarify with @pmaier1 now.

If we need to show the creator than this would mean that we will need to show the very first version of a file in the versions view which we don't do yet.

phil-davis · 2021-11-17T16:16:08Z

We don't have to show the "creator" of the "current version" of a file (the "version" that is not shown in the version history, because it is not yet history!) But technically, IMO, we will have to remember it somewhere so that it is available if and when that data becomes an "old version".

At the moment, if

Alice creates a file
Brian edits it
Carol edits it

At step (2) it is Brian who is authenticated. I don't think "the system" has any way to reliably know that the original file was created by Alice.

At step (3) it is Carol who is authenticated. I don't think "the system" has any way to reliably know that the file before the edit was uploaded by Brian.

IljaN · 2021-11-17T16:26:45Z

Maybe we can create a specially-named metadata-file which is not returned by the webdav-api but is internally replaced as soon as a new version is available.

IljaN · 2021-11-17T17:28:04Z

Maybe we can create a specially-named metadata-file which is not returned by the webdav-api but is internally replaced as soon as a new version is available.

After talk with @pmaier1 Will implement something like this

phil-davis · 2021-11-18T04:48:50Z

I raised an issue to make some acceptance tests for this. We can do that in parallel in a branch on top of this.

Note my comment #39504 (comment)

Currently for this feature there is no way for end-users to find out who is the author of the data in the current file. If I was user "Alice" looking at a file, and seeing the author of each old version in the version history, an thinking about restoring some version, then probably I am doing it because the current data in the file is "wrong/corrupt" in some way. So I will immediately want to know "who is the author of the current data in the file?"

As Alice I can already find out other meta-data about the current file - modified date, tags, comments. Should users be able to find out the "latest author"?

IljaN · 2021-11-18T08:49:53Z

Possible edge-case: As of now we don't write versions for empty files (filesize=0): So we also shouldn't write metadata for emtpy files. (?)

phil-davis · 2021-11-18T09:33:05Z

Possible edge-case: As of now we don't write versions for empty files (filesize=0): So we also shouldn't write metadata for emtpy files. (?)

And a combination edge-case. If we add some way for users to find out the "author" of the current file, then if there is version history of non-empty versions of the file, and "someone" has uploaded an empty file as the current file, then I think that user Alice will want to know who uploaded the "nothing" to the file.

AlexAndBear

Approving for now - Still in Beta Phase - Will be ready before RC

mmattel · 2021-11-18T13:40:25Z

@janackermann docs relevant 👋, pls file a docs issue.

Co-authored-by: Ilia Pushnov <[email protected]>

IljaN · 2021-11-22T13:02:42Z

Continued here: #39516

Co-authored-by: Ilia Pushnov <[email protected]>

jnweiger · 2021-12-08T02:51:27Z

tested in 10.9.0 beta1

works fine with and without encryption.
delete restore should be improved -> 39571

phil-davis · 2021-12-08T02:57:47Z

Note: I am quite sure that the version authors are off-by-one in 10.9.0 beta1. That is being fixed in #39516

JammingBen reviewed Aug 20, 2021

View reviewed changes

apps/dav/lib/Meta/MetaFile.php Outdated Show resolved Hide resolved

apps/files_trashbin/lib/Trashbin.php Outdated Show resolved Hide resolved

apps/files_trashbin/lib/Trashbin.php Outdated Show resolved Hide resolved

apps/files_versions/lib/Storage.php Outdated Show resolved Hide resolved

micbar requested a review from DeepDiver1975 August 20, 2021 16:06

phil-davis reviewed Aug 23, 2021

View reviewed changes

lib/public/Files/IProvidesVersionAuthor.php Outdated Show resolved Hide resolved

lib/public/Files/IProvidesVersionAuthor.php Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

IljaN assigned IljaN and pushnov-i and unassigned IljaN Aug 23, 2021

IljaN added the 3 - To Review label Aug 23, 2021

IljaN added this to the development milestone Aug 23, 2021

IljaN reviewed Aug 24, 2021

View reviewed changes

apps/dav/lib/Meta/MetaPlugin.php Outdated Show resolved Hide resolved

phil-davis reviewed Aug 24, 2021

View reviewed changes

apps/files_trashbin/tests/StorageTest.php Outdated Show resolved Hide resolved

pushnov-i force-pushed the feature/version-metadata branch from 4247c57 to a0f7f5a Compare August 24, 2021 15:54

phil-davis mentioned this pull request Nov 18, 2021

[tests-only] Add acceptance tests for version metadata #39504

Closed

AlexAndBear approved these changes Nov 18, 2021

View reviewed changes

AlexAndBear merged commit 49110b5 into master Nov 18, 2021

delete-merged-branch bot deleted the feature/version-metadata branch November 18, 2021 10:59

phil-davis pushed a commit that referenced this pull request Nov 18, 2021

Acceptance tests for #39126

fb36bb0

Co-authored-by: Ilia Pushnov <[email protected]>

IljaN mentioned this pull request Nov 22, 2021

Version Metadata Continued #39516

Merged

11 tasks

phil-davis pushed a commit that referenced this pull request Nov 25, 2021

Acceptance tests for #39126

277f549

Co-authored-by: Ilia Pushnov <[email protected]>

JammingBen added php Pull requests that update Php code and removed 3 - To Review labels Nov 25, 2021

mmattel mentioned this pull request Nov 30, 2021

New config key for files metadata owncloud/docs#4419

Open

12 tasks

phil-davis pushed a commit that referenced this pull request Dec 1, 2021

Acceptance tests for #39126

6b5b3ca

Co-authored-by: Ilia Pushnov <[email protected]>

IljaN pushed a commit that referenced this pull request Dec 5, 2021

Acceptance tests for #39126

3ac5e1b

Co-authored-by: Ilia Pushnov <[email protected]>

phil-davis pushed a commit that referenced this pull request Dec 6, 2021

Acceptance tests for #39126

398ddd7

Co-authored-by: Ilia Pushnov <[email protected]>

jnweiger mentioned this pull request Dec 8, 2021

Versions author info is lost after delete and restore by share receiver #39571

Closed

phil-davis mentioned this pull request Dec 22, 2021

file versions do not report the version author owncloud/ocis#2914

Open

Feature/version metadata #39126

Feature/version metadata #39126

Conversation

pushnov-i commented Aug 20, 2021 • edited by IljaN Loading

CLAassistant commented Aug 20, 2021 • edited Loading

IljaN commented Aug 20, 2021

JammingBen left a comment

Choose a reason for hiding this comment

ownclouders commented Aug 20, 2021

JammingBen commented Aug 21, 2021 • edited Loading

mrow4a commented Aug 21, 2021 • edited Loading

pushnov-i commented Aug 23, 2021 • edited Loading

IljaN commented Aug 23, 2021 • edited Loading

This comment has been minimized.

IljaN Aug 24, 2021

Choose a reason for hiding this comment

IljaN Aug 24, 2021

Choose a reason for hiding this comment

IljaN Aug 24, 2021

Choose a reason for hiding this comment

pushnov-i Aug 24, 2021

Choose a reason for hiding this comment

IljaN Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

pushnov-i Aug 24, 2021

Choose a reason for hiding this comment

IljaN Aug 24, 2021

Choose a reason for hiding this comment

IljaN Aug 24, 2021

Choose a reason for hiding this comment

IljaN commented Aug 24, 2021

mrow4a commented Aug 24, 2021

IljaN commented Aug 24, 2021 • edited Loading

phil-davis commented Aug 24, 2021

IljaN commented Aug 24, 2021 • edited Loading

phil-davis commented Aug 24, 2021

pushnov-i commented Aug 24, 2021

phil-davis commented Aug 24, 2021

pushnov-i commented Aug 24, 2021

sonarqubecloud bot commented Nov 17, 2021

pmaier1 commented Nov 17, 2021 • edited Loading

phil-davis commented Nov 17, 2021

IljaN commented Nov 17, 2021 • edited Loading

phil-davis commented Nov 17, 2021

IljaN commented Nov 17, 2021 • edited Loading

IljaN commented Nov 17, 2021 • edited Loading

phil-davis commented Nov 18, 2021

IljaN commented Nov 18, 2021 • edited Loading

phil-davis commented Nov 18, 2021

AlexAndBear left a comment

Choose a reason for hiding this comment

mmattel commented Nov 18, 2021

IljaN commented Nov 22, 2021

jnweiger commented Dec 8, 2021

tested in 10.9.0 beta1

phil-davis commented Dec 8, 2021

pushnov-i commented Aug 20, 2021 •

edited by IljaN

Loading

CLAassistant commented Aug 20, 2021 •

edited

Loading

JammingBen commented Aug 21, 2021 •

edited

Loading

mrow4a commented Aug 21, 2021 •

edited

Loading

pushnov-i commented Aug 23, 2021 •

edited

Loading

IljaN commented Aug 23, 2021 •

edited

Loading

IljaN Aug 24, 2021 •

edited

Loading

IljaN commented Aug 24, 2021 •

edited

Loading

IljaN commented Aug 24, 2021 •

edited

Loading

pmaier1 commented Nov 17, 2021 •

edited

Loading

IljaN commented Nov 17, 2021 •

edited

Loading

IljaN commented Nov 17, 2021 •

edited

Loading

IljaN commented Nov 17, 2021 •

edited

Loading

IljaN commented Nov 18, 2021 •

edited

Loading