Fix preview of theming by danxuliu · Pull Request #5981 · nextcloud/server

danxuliu · 2017-08-04T13:05:34Z

Pull request #5429 made cached SCSS files depend on a hash of the base URL, so the /css/core/server.css file does no longer exist; as the file can not be loaded the "Loading preview" message is never removed and the "Saved" message is never shown.

As it now depends on the hash of the base URL the file to be reloaded can no longer be hardcoded, so the DataResponse from the controller now provides the full URL to the server.css file that has to be reloaded (if any).

Fixes #5975

Acceptance tests for the preview after setting and resetting the color in the Theming app are also included in this pull request.

mention-bot · 2017-08-04T13:05:35Z

@danxuliu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @juliushaertl, @schiessle and @LukasReschke to be potential reviewers.

codecov · 2017-08-04T14:35:51Z

Codecov Report

Merging #5981 into master will decrease coverage by 15.85%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##             master    #5981       +/-   ##
=============================================
- Coverage     52.87%   37.02%   -15.86%     
+ Complexity    22770    22768        -2     
=============================================
  Files          1404     1404               
  Lines         88177    88159       -18     
  Branches       1327     1327               
=============================================
- Hits          46627    32639    -13988     
- Misses        41550    55520    +13970

Impacted Files	Coverage Δ	Complexity Δ
apps/theming/lib/Controller/ThemingController.php	`4.83% <0%> (-69.81%)`	`34 <0> (ø)`
core/Command/TwoFactorAuth/Disable.php	`0% <0%> (-100%)`	`4% <0%> (ø)`
lib/private/App/CodeChecker/EmptyCheck.php	`0% <0%> (-100%)`	`6% <0%> (ø)`
apps/files/lib/Activity/Settings/FileChanged.php	`0% <0%> (-100%)`	`8% <0%> (ø)`
lib/private/Hooks/ForwardingEmitter.php	`0% <0%> (-100%)`	`5% <0%> (ø)`
lib/private/Files/SimpleFS/SimpleFolder.php	`0% <0%> (-100%)`	`11% <0%> (ø)`
apps/dav/lib/CalDAV/Activity/Setting/Todo.php	`0% <0%> (-100%)`	`8% <0%> (ø)`
lib/private/Files/ObjectStore/Mapper.php	`0% <0%> (-100%)`	`2% <0%> (ø)`
apps/dav/lib/CalDAV/Plugin.php	`0% <0%> (-100%)`	`2% <0%> (ø)`
core/Command/TwoFactorAuth/Enable.php	`0% <0%> (-100%)`	`4% <0%> (ø)`
... and 484 more

danxuliu · 2017-08-04T14:37:01Z

Oops, I had forgotten to add the acceptance tests for the Theming app to Drone; it is fixed now.

MorrisJobke · 2017-08-04T21:34:40Z

You are awesome!!!! 🚀 😍

Thanks again for helping that much 👍

MorrisJobke · 2017-08-04T21:41:50Z

apps/theming/js/settings-admin.js

+		// Ensure that the URL starts by '/' to prevent "stylesheet.load" from
+		// prepending the URL of the current page.
+		if (serverCssUrl[0] !== '/') {
+			serverCssUrl = '/' + serverCssUrl;


This doesn't work for instances that are in a subfolder. It needs to be the full URL. linkToAbsolute or something like that should exist, which prepends you the webroot.

This doesn't work for instances that are in a subfolder. It needs to be the full URL.

Totally right; getCachedSCSS removes the webroot.

linkToAbsolute or something like that should exist, which prepends you the webroot.

I have used Util::linkTo instead, as Util::linkToAbsolute adds the webroot, but also the protocol and server host.

MorrisJobke

Tested and works 👍

nickvergessen · 2017-08-09T10:15:51Z

Tests fail:

There were 8 failures:

1) OCA\Theming\Tests\Controller\ThemingControllerTest::testUpdateStylesheet

nickvergessen

Tests

MorrisJobke · 2017-08-09T11:48:30Z

And please rebase on master - we updated the drone config 🙈

In some cases the acceptance tests have to explicitly wait for something to happen without using the "find" method from the actor; in those cases the timeout multiplier needs to be taken into account too, so the test cases must be able to retrieve it from the actor. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

This is a preparatory step for a following commit in which the properties present in the response will change depending on whether the request was successful or not. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Pull request #5429 made cached SCSS files depend on a hash of the base URL, so the "/css/core/server.css" file does no longer exist. The "server.css" URL must be known by the Theming app in order to update the stylesheets when previewing the changes to the theme, so the DataResponse from the controller now provides the full URL to the "server.css" file that has to be reloaded (if any). The "server.css" URL provided by the response will be taken into account by the JavaScript front-end in a following commit. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

This is a preparatory step for a following change in which reloadStylesheets will have to be able to receive absolute URLs. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Pull request #5429 made cached SCSS files depend on a hash of the base URL, so the "/css/core/server.css" file does no longer exist; as the file can not be loaded the "Loading preview" message is never removed and the "Saved" message is never shown. As it now depends on the hash of the base URL the file to be reloaded can no longer be hardcoded, so the full URL to the "server.css" file that has to be reloaded (if any) is now got from the DataResponse provided by the controller. Fixes #5975 Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

danxuliu · 2017-08-10T11:14:21Z

@nickvergessen

Tests fail

🤦 Yes, I forgot to check the tests. And not only that, I also used a deprecated method (OCP\Util::linkTo). Both are fixed now.

@MorrisJobke

And please rebase on master - we updated the drone config 🙈

Done.

Edit: By the way, I have noticed that the commit order in the GitHub interface is messed up (as it is ordered chronologically instead of parent->child); I can change their timestamp and push again so they are showed in the right order for easier review if you want.

MorrisJobke · 2017-08-10T11:59:18Z

Edit: By the way, I have noticed that the commit order in the GitHub interface is messed up (as it is ordered chronologically instead of parent->child); I can change their timestamp and push again so they are showed in the right order for easier review if you want.

No - just leave it as it is :) That's fine :)

Fixed

MorrisJobke

Tested again and works 👍 Code looks good

MorrisJobke · 2017-08-10T12:25:52Z

Huge thanks again @danxuliu for this awesome help here 👍

Could I ask you to open the backport to stable12? That would be nice :)

danxuliu · 2017-08-10T12:32:07Z

Huge thanks again @danxuliu for this awesome help here 👍

My pleasure :-)

Could I ask you to open the backport to stable12? That would be nice :)

Here you have (yes, I had it already prepared :-P ): #6067

danxuliu added 3. to review Waiting for reviews bug regression labels Aug 4, 2017

danxuliu force-pushed the fix-preview-of-theming branch from 1a29af6 to af494e1 Compare August 4, 2017 14:35

MorrisJobke requested changes Aug 4, 2017

View reviewed changes

danxuliu force-pushed the fix-preview-of-theming branch from af494e1 to 9613575 Compare August 8, 2017 08:57

MorrisJobke added the backport-request label Aug 8, 2017

MorrisJobke approved these changes Aug 8, 2017

View reviewed changes

MorrisJobke requested review from LukasReschke, juliusknorr, nickvergessen and rullzer August 8, 2017 09:03

MorrisJobke added this to the Nextcloud 13 milestone Aug 8, 2017

MorrisJobke mentioned this pull request Aug 8, 2017

Preview of theming does not work anymore #5975

Closed

nickvergessen approved these changes Aug 9, 2017

View reviewed changes

nickvergessen previously requested changes Aug 9, 2017

View reviewed changes

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 9, 2017

danxuliu added 6 commits August 10, 2017 12:37

Add acceptance tests for setting the color in the Theming app

0c8627a

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Unify DataResponse format with its uses in the rest of the file

d7e45d4

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Split updateStylesheet tests for success and error

986dffd

This is a preparatory step for a following commit in which the properties present in the response will change depending on whether the request was successful or not. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Refactor reloadStylesheets to receive absolute URLs

5999b21

This is a preparatory step for a following change in which reloadStylesheets will have to be able to receive absolute URLs. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

danxuliu force-pushed the fix-preview-of-theming branch from 9613575 to b16ce1b Compare August 10, 2017 11:07

danxuliu added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 10, 2017

MorrisJobke approved these changes Aug 10, 2017

View reviewed changes

rullzer approved these changes Aug 10, 2017

View reviewed changes

rullzer merged commit ca121b7 into master Aug 10, 2017

rullzer deleted the fix-preview-of-theming branch August 10, 2017 12:23

danxuliu mentioned this pull request Aug 10, 2017

[stable12] Fix preview of theming #6067

Merged

MorrisJobke removed the backport-request label Aug 10, 2017

Uh oh!

Comments

Conversation

danxuliu commented Aug 4, 2017

Uh oh!

mention-bot commented Aug 4, 2017

Uh oh!

codecov bot commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

danxuliu commented Aug 4, 2017

Uh oh!

MorrisJobke commented Aug 4, 2017

Uh oh!

MorrisJobke Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

danxuliu Aug 8, 2017

Choose a reason for hiding this comment

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

nickvergessen commented Aug 9, 2017

Uh oh!

nickvergessen left a comment

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Aug 9, 2017

Uh oh!

danxuliu commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MorrisJobke commented Aug 10, 2017

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Aug 10, 2017

Uh oh!

danxuliu commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Aug 4, 2017 •

edited

Loading

danxuliu commented Aug 10, 2017 •

edited

Loading

danxuliu commented Aug 10, 2017 •

edited

Loading