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

Force enable apps #14578

Merged
merged 4 commits into from
Mar 20, 2019
Merged

Force enable apps #14578

merged 4 commits into from
Mar 20, 2019

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Mar 6, 2019

Allow force enabling apps. So if an app is not yet compatible with 15 the admin can force enable it

Frontend behavior:

  1. The list that is fetched from /settings/apps/list now for each entry contains a isCompatible entry. If isCompatible is false. I suggest we clearly highlight this. Grey out? Red. Whatever. But it needs to be visible. Maybe also a badge oir something?

  2. If isCompatible is false and canInstall is false. There should be a button. This should say something like Force. This will then show a popup I guess where there is some text. Like 'This app is not marked as compatible with your Nextcloud version. If you continue you will still be able to install the app. Note that the app might not work as expected.' Then if the admin continues a post (with the appId) should be made to /setttings/apps/force.

  3. Then reload the page (This is not the most elegant but will ahve to do for now as you need to reobtain the list of apps).

  4. Now you should be able to just enable your app.

Todo:

  • Fix frontend code of the app management page to allow force enable
  • Add controller to add force enabled apps
  • Look into appstore for 16 returning all apps (Need to return all apps on >=NC16 appstore#600)
  • Style the force enable button in a way that it's obvious the user is performing a critical action.
  • Bug on upgrade fixed. It now assumes the max version is not ignored until explicitly stated. This means apps will still get disable on upgrade. even if they are not force enabled here. Which is a fair limitation for a first step.
  • Move to default CSS and not use embedded CSS (inside Vue Components)
  • Fix failing unit tests
  • Fix failing acceptance tests

Screenshots (button in the right sidebar is the hover state):

Bildschirmfoto 2019-03-19 um 07 43 08

Bildschirmfoto 2019-03-19 um 07 43 31

@rullzer

This comment has been minimized.

@ChristophWurst ChristophWurst self-assigned this Mar 7, 2019
@ChristophWurst ChristophWurst added this to IN PROGRESS (max 3 PRs) in Christoph's Tasks via automation Mar 7, 2019
@ChristophWurst

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@ChristophWurst ChristophWurst removed this from IN PROGRESS (max 3 PRs) in Christoph's Tasks Mar 7, 2019
@ChristophWurst ChristophWurst removed their assignment Mar 7, 2019
@rullzer

This comment has been minimized.

@rullzer

This comment has been minimized.

@rullzer
Copy link
Member Author

rullzer commented Mar 7, 2019

  • Issue

The 'download and force enable' is a bit weird. Because it doesn't download. It only force enables.,

But I'm not a big fan of force enable.. @jancborchardt @karlitschek any suggestions?

@rullzer

This comment has been minimized.

@karlitschek

This comment has been minimized.

@jancborchardt

This comment has been minimized.

@rullzer
Copy link
Member Author

rullzer commented Mar 18, 2019

Ok this is shaping up. Failing tests need to be fixed. But not today.

@@ -39,7 +39,7 @@
use OCP\Util;

abstract class Fetcher {
const INVALIDATE_AFTER_SECONDS = 300;
const INVALIDATE_AFTER_SECONDS = 1;
Copy link
Member

Choose a reason for hiding this comment

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

🧐

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that needs to die

@MorrisJobke
Copy link
Member

I fixed the UI to be a bit more elegant (also in dark mode):

Bildschirmfoto 2019-03-19 um 07 43 08

Bildschirmfoto 2019-03-19 um 07 43 31

@jancborchardt
Copy link
Member

Looks very nice! Only 3 things:

  • If there is an update available, we shouldn't show the force enable button? Cause the update might have compatibility with the new version. So actually there should only ever 2 buttons: 1) remove and 2) either enable, update or force enable.
  • The position of the text in the sidebar could be closer to the buttons, e.g. moved a bit up so it's below the buttons and above the links.
  • In dark theme, the button text of the red button should also be white (lile the primary button).

@rullzer
Copy link
Member Author

rullzer commented Mar 19, 2019

If there is an update available, we shouldn't show the force enable button? Cause the update might have compatibility with the new version. So actually there should only ever 2 buttons: 1) remove and 2) either enable, update or force enable.

True. But out of scope for now. I want this in asap. But will create a ticket.

The position of the text in the sidebar could be closer to the buttons, e.g. moved a bit up so it's below the buttons and above the links.

Again. Will create a ticket

In dark theme, the button text of the red button should also be white (lile the primary button).

Fair enough.

@rullzer rullzer marked this pull request as ready for review March 19, 2019 20:31
@MorrisJobke MorrisJobke mentioned this pull request Mar 20, 2019
9 tasks
@MorrisJobke MorrisJobke changed the title Enh/force enable apps Force enable apps Mar 20, 2019
@nextcloud nextcloud deleted a comment from faily-bot bot Mar 20, 2019
@nickvergessen
Copy link
Member

To test my change, set your channel to production in version.php and change

$data = $this->updater->check();

to

		$data = [
			'version' => '16.0.2.1',
			'versionstring' => '16.0.0',
			'autoupdater' => '1',
			'eol' => '0',
			'web' => 'https://nextcloud.com',
			'url' => 'https://nextcloud.com',
			'changes' => '',
		];//$this->updater->check();

if you have talk enabled, it will now still tell you that there is no version for 16 in the update-available check.

rullzer and others added 4 commits March 20, 2019 15:16
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@rullzer
Copy link
Member Author

rullzer commented Mar 20, 2019

CI Green: https://drone.nextcloud.com/nextcloud/server/17171. I just rebased to remove the fixups.

Lets do this!

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🐘 because I still dont think we should do this

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

Would not work in a load balanced scenario due to the change to the config.php, but app management doesn't work as expected there anyways.

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 20, 2019
@faily-bot
Copy link

faily-bot bot commented Mar 20, 2019

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 17177: failure

TESTS=acceptance, TESTS-ACCEPTANCE=apps

  • tests/acceptance/features/apps.feature:43
Show full log
  Scenario: Enable an app bundle                                          # /drone/src/github.com/nextcloud/server/tests/acceptance/features/apps.feature:43
    Given I act as Jane                                                   # ActorContext::iActAs()
    And I am logged in as the admin                                       # LoginPageContext::iAmLoggedInAsTheAdmin()
    And I open the Apps management                                        # SettingsMenuContext::iOpenTheAppsManagement()
    And I open the "App bundles" section                                  # AppNavigationContext::iOpenTheSection()
    When I enable all apps from the "Enterprise bundle"                   # AppsManagementContext::iEnableAllAppsFromThe()
    Then I see that the "Auditing / Logging" app has been enabled         # AppsManagementContext::iSeeThatTheAppHasBeenEnabled()
      Disable button in the app list for Auditing / Logging could not be found after 100 seconds (NoSuchElementException)
    And I see that the "LDAP user and group backend" app has been enabled # AppsManagementContext::iSeeThatTheAppHasBeenEnabled()

@rullzer
Copy link
Member Author

rullzer commented Mar 20, 2019

@MorrisJobke fair point. But yes they have issues with that already indeed.

@MorrisJobke
Copy link
Member

But the acceptance tests failed 🙈

@rullzer
Copy link
Member Author

rullzer commented Mar 20, 2019

@MorrisJobke it did at random... but as you can see https://drone.nextcloud.com/nextcloud/server/17171 passed (which is the same code)

rullzer added a commit that referenced this pull request Mar 20, 2019
The bundle acceptance tests fails after #14578 sometimes. This is
because of a race condition. not all apps have compatible 16 versions
yet. So trying to enable them results in those apps doing 💥.

Because of #14578 we do show them now. So we try to enable them. However
depending on which requests finishes first the disable button for the
audit app either shows up or now.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Copy link
Member Author

rullzer commented Mar 20, 2019

@MorrisJobke yes race condition. Temp disable in #14774 we can revert it once all apps have a 16 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: apps management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants