-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix preview generation in files api controller and adding unit tests #31184
Fix preview generation in files api controller and adding unit tests #31184
Conversation
Codecov Report
@@ Coverage Diff @@
## master #31184 +/- ##
============================================
+ Coverage 62.55% 62.56% +<.01%
Complexity 18279 18279
============================================
Files 1147 1147
Lines 68474 68474
Branches 1234 1234
============================================
+ Hits 42833 42839 +6
+ Misses 25280 25274 -6
Partials 361 361
Continue to review full report at Codecov.
|
@@ -139,7 +139,7 @@ public function createPreview($file, $maxX = 100, $maxY = 75, $scaleUp = false) | |||
if ($user === null) { | |||
throw new NotLoggedInException(); | |||
} | |||
$file = $this->rootFolder->getUserFolder($user->getUID())->get($file); | |||
$file = $this->rootFolder->getUserFolder($user->getUID())->getParent()->get($file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as the change is different than the one on stable10, is there an explanation for why this is the right approach ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the api change on master requires a node to be passed into the preview class.
to get the node object for the given path the lazyroot is used as well as getUserFolder.
because the give file path already contains the folder 'files' the get parent call is necessary.
due to missing tests this all way hidden and never popped up - test got added and now if works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code makes sense, not tested 👍
Reproduced the thumbnail problem on current master. |
Arghhh! I pressed "close and comment" instead of just "comment". Now when I reopen it seems to be running CI again. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
front port of #31183