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

[stable10] Fixup preview issues with trashbin/versions #34533

Merged
merged 8 commits into from
Feb 18, 2019

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented Feb 18, 2019

Backport of #34283

Description

  • uid is always known - pass it within the version_delete hook info
  • pass original_path that contains a path to the versioned file
    and deleted_revision - versionId for the deleted version
  • pass an optional versionId to deleteAllPreviews when we need to remove prievews for the specific version only
  • bonus 1: fix preview generation for files in deleted directories
  • bonus 2: cover some parts of files_versions with tests

Related Issue

Motivation and Context

  • Version previews are not deleted properly
  • Previews don't show for files in deleted directories
  • Versions and trash expiration is flooding the log file with exceptions related to the previews

How Has This Been Tested?

Case 1

  1. Create a text file "test.txt"
  2. Edit the file three times in the text editor and save each time, will create three versions
  3. Open the versions panel to have it generate previews
  4. Run cron.php and observe the logs

Expected result

version previews for cleaned up versions are deleted
No errors

Actual result

version previews for cleaned up versions are NOT deleted

{"reqId":"viyF6aTvAF41QBIxIu9B","level":3,"time":"2018-10-15T19:37:42+00:00","remoteAddr":"","user":"--","app":"no app in context","method":"--","url":"--","message":"Exception: {\"Exception\":\"OCP\\\\Files\\\\NotFoundException\",\"Message\":\"\\\/test.txt.v1539632201 not found while trying to get owner\",\"Code\":0,\"Trace\":\"#0 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Files\\\/Filesystem.php(961): OC\\\\Files\\\\View->getOwner('\\\/test.txt.v1539...')\\n#1 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Preview.php(1241): OC\\\\Files\\\\Filesystem::getOwner('\\\/test.txt.v1539...')\\n#2 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/legacy\\\/hook.php(105): OC\\\\Preview::prepare_delete(Array)\\n#3 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/files_versions\\\/lib\\\/Storage.php(765): OC_Hook::emit('\\\\\\\\OCP\\\\\\\\Versions', 'preDelete', Array)\\n#4 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/files_versions\\\/lib\\\/Command\\\/Expire.php(60): OCA\\\\Files_Versions\\\\Storage::expire('\\\/test.txt', 'admin')\\n#5 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Command\\\/CommandJob.php(34): OCA\\\\Files_Versions\\\\Command\\\\Expire->handle()\\n#6 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/BackgroundJob\\\/Job.php(57): OC\\\\Command\\\\CommandJob->run('O:33:\\\"OCA\\\\\\\\Files...')\\n#7 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/BackgroundJob\\\/QueuedJob.php(42): OC\\\\BackgroundJob\\\\Job->execute(Object(OC\\\\BackgroundJob\\\\JobList), Object(OC\\\\Log))\\n#8 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/cron.php(120): OC\\\\BackgroundJob\\\\QueuedJob->execute(Object(OC\\\\BackgroundJob\\\\JobList), Object(OC\\\\Log))\\n#9 {main}\",\"File\":\"\\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Files\\\/View.php\",\"Line\":1717}"}
{"reqId":"viyF6aTvAF41QBIxIu9B","level":3,"time":"2018-10-15T19:37:42+00:00","remoteAddr":"","user":"--","app":"no app in context","method":"--","url":"--","message":"Exception: {\"Exception\":\"OCP\\\\Files\\\\NotFoundException\",\"Message\":\"\\\/test.txt.v1539632201 not found while trying to get owner\",\"Code\":0,\"Trace\":\"#0 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Files\\\/Filesystem.php(961): OC\\\\Files\\\\View->getOwner('\\\/test.txt.v1539...')\\n#1 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Preview.php(1311): OC\\\\Files\\\\Filesystem::getOwner('\\\/test.txt.v1539...')\\n#2 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Preview.php(1299): OC\\\\Preview::post_delete(Array, 'files\\\/')\\n#3 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/legacy\\\/hook.php(105): OC\\\\Preview::post_delete_versions(Array)\\n#4 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/files_versions\\\/lib\\\/Storage.php(767): OC_Hook::emit('\\\\\\\\OCP\\\\\\\\Versions', 'delete', Array)\\n#5 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/files_versions\\\/lib\\\/Command\\\/Expire.php(60): OCA\\\\Files_Versions\\\\Storage::expire('\\\/test.txt', 'admin')\\n#6 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Command\\\/CommandJob.php(34): OCA\\\\Files_Versions\\\\Command\\\\Expire->handle()\\n#7 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/BackgroundJob\\\/Job.php(57): OC\\\\Command\\\\CommandJob->run('O:33:\\\"OCA\\\\\\\\Files...')\\n#8 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/BackgroundJob\\\/QueuedJob.php(42): OC\\\\BackgroundJob\\\\Job->execute(Object(OC\\\\BackgroundJob\\\\JobList), Object(OC\\\\Log))\\n#9 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/cron.php(120): OC\\\\BackgroundJob\\\\QueuedJob->execute(Object(OC\\\\BackgroundJob\\\\JobList), Object(OC\\\\Log))\\n#10 {main}\",\"File\":\"\\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Files\\\/View.php\",\"Line\":1717}"}

Case 2

  1. create a text files, edit it several times
  2. open versions to create a previews for versions
  3. edit file one more time

Expected result

Only file previews are deleted

Actual result

File previews and all versions previews are deleted

Case 3

delete a directory with a file and browse to the file in the trashbin

Expected result

File in deleted directory has a prievew

Actual result

No file preview

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@VicDeo VicDeo added this to the development milestone Feb 18, 2019
@VicDeo VicDeo self-assigned this Feb 18, 2019
@codecov
Copy link

codecov bot commented Feb 18, 2019

Codecov Report

Merging #34533 into stable10 will increase coverage by 0.04%.
The diff coverage is 63.37%.

Impacted file tree graph

@@              Coverage Diff               @@
##             stable10   #34533      +/-   ##
==============================================
+ Coverage       63.89%   63.94%   +0.04%     
- Complexity      19133    19146      +13     
==============================================
  Files            1268     1269       +1     
  Lines           75519    75582      +63     
  Branches         1293     1293              
==============================================
+ Hits            48256    48328      +72     
+ Misses          26882    26873       -9     
  Partials          381      381
Flag Coverage Δ Complexity Δ
#javascript 53.29% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.08% <63.37%> (+0.04%) 19146 <44> (+13) ⬆️
Impacted Files Coverage Δ Complexity Δ
apps/files_trashbin/ajax/preview.php 0% <0%> (ø) 0 <0> (ø) ⬇️
apps/files_versions/lib/AppInfo/Application.php 100% <100%> (ø) 2 <1> (ø) ⬇️
apps/files_versions/lib/Storage.php 72.39% <24.39%> (+1%) 95 <10> (-9) ⬇️
lib/private/Preview.php 80.12% <58.49%> (+1.38%) 178 <17> (+8) ⬆️
apps/files_trashbin/lib/Trashbin.php 77.7% <85.71%> (+0.29%) 146 <2> (ø) ⬇️
apps/files_versions/lib/FileHelper.php 92.72% <92.72%> (ø) 14 <14> (?)
apps/encryption/templates/settings-admin.php 94.11% <0%> (+5.88%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ef46c5...15faa39. Read the comment docs.

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit a898456 into stable10 Feb 18, 2019
@delete-merged-branch delete-merged-branch bot deleted the stable10-fix-32346 branch February 18, 2019 18:03
@PVince81 PVince81 modified the milestones: development, QA Apr 12, 2019
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.

2 participants