Add different trust levels to AppStore interface #15314

LukasReschke · 2015-03-30T14:04:05Z

🚧 🚧 🚧

TODO:

Have apps.owncloud.com return the trust levels – somehow (cc @karlitschek)
Make it look nicer (cc @jancborchardt)
Move the "Experimental applications ahead" bar right before the experimental ones
Show the experimental applications at the end of the site if
Make the "Show experimental applications" button actually working

🚧 🚧 🚧

Heads-up: We don't know when an application is activated (i.e. already installing) whether it is an official one / approved one or complete random stuff. Thus in the "activated" and "not activated" view we only show the official apps where we know it from the shipped flag and everything else has no flag applied.

Current layout:

LukasReschke · 2015-03-30T14:24:32Z

Interesting. Seems like there is a SimpleXML version on our CI server that behaves differently than mine 😞

:22:09 1) OCSClientTest::testGetCategoriesParseError
10:22:09 simplexml_load_string(): Entity: line 1: parser error : Start tag expected, '<' not found
10:22:09 
10:22:09 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/ocsclient.php:114
10:22:09 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/tests/lib/OCSClientTest.php:178
10:22:09 
10:22:09 2) OCSClientTest::testGetApplicationsParseError
10:22:09 simplexml_load_string(): Entity: line 1: parser error : Start tag expected, '<' not found
10:22:09 
10:22:09 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/ocsclient.php:178
10:22:09 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/tests/lib/OCSClientTest.php:374
10:22:09 
10:22:09 3) OCSClientTest::testGetApplicationParseError
10:22:09 simplexml_load_string(): Entity: line 1: parser error : Start tag expected, '<' not found
10:22:09 
10:22:09 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/ocsclient.php:259
10:22:09 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/tests/lib/OCSClientTest.php:636
10:22:09 
10:22:09 4) OCSClientTest::testGetApplicationDownloadParseError
10:22:09 simplexml_load_string(): Entity: line 1: parser error : Start tag expected, '<' not found
10:22:09 
10:22:09 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/ocsclient.php:323
10:22:09 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/tests/lib/OCSClientTest.php:867

… digging …

jancborchardt · 2015-03-30T16:40:14Z

settings/templates/apps.php

Instead of a permanent header, can you insert a <div class="section"> with that info between the last »Approved« and the first »Experimental« app?

Guess what it says in the top post:

Move the "Experimental applications ahead" bar right before the experimental ones

jospoortvliet · 2015-03-30T16:54:03Z

@LukasReschke could you use a different wording to describe the experimental apps? I'd like this:
[] show experimental apps
Experimental apps are not checked for security issues and are new or known to be unstable and under heavy development. Installing these can cause data loss or security breaches.

Use the same in the yellow header, saying:
"display of experimental apps enabled"
You might want to include a 'hide experimental apps' button there, as well as a 'dismiss warning' one.

jancborchardt · 2015-03-30T16:55:52Z

tests/lib/OCSClientTest.php

So now we can remove the »Recommended« label, right?

Already is removed in the web interface. That one is in the response that the server returns. It's an unit test. Probably we need to keep it on the server as well for longer or it won't work at all for older versions.

LukasReschke · 2015-04-02T10:47:10Z

@karlitschek @jancborchardt @jospoortvliet Please take a look - should work now - not the best code but I added at least some unit tests for other untested stuff 🙈

Please test properly, in my case it works good. To test this checkout this branch and – this is important – change the $OC_VERSION in version.php to array(8,0,0,1) so that the AppStore will show you the 8.0 apps. If you then switch to a section such as "Tool" you should see some experimental applications.

The yellow banner might require some CSS tweaks - but I'm not a design guy and for me everything looks good as long as it has a color ;-) – I also added the recommendations by @jospoortvliet in the text field as well as the tipsy overlay on the labels.

Also: Please ensure to clean your browser cache before testing this.

Some small remarks that we can't address at the moment:

The "Activated" view will only show tags for the "Official" apps, the thing is we really don't know at this step which state an application has as we are not connected to the appstore and addressing this properly should be done in a refactoring that I wanted to avoid.
Clicking the "Enable experimental apps" button will reload the page and you have to go to the category you were before manually. That is caused by the reason that we don't recognize the current category by the URL and is also there in master. Should be addressed in the future but is out-of-scope for this PR.

And yes, some code of this is quite hacky but I couldn't come up with something better in this short timeframe since I have to do other stuff as well… But we always can improve in the future anyway :-)

LukasReschke · 2015-04-02T10:48:37Z

Some screenshots:

LukasReschke · 2015-04-02T10:50:52Z

I think we can do other improvements such as showing the author/domain/whatever etc. in other PRs – these changes are actually much easier (about 1 line additional PHP and then the usual CSS /HTML thing) but require that somebody (hint hint @jancborchardt hint hint) comes up with a design for those ;-)

jospoortvliet · 2015-04-02T11:53:44Z

I'd make the popup text this: "This app is not checked for security issues and is new or known to be unstable. Install on your own risk."

Look-wise I have no comments (I'm sure Jan does). Functionality-wise, I agree that this, once working and safe (not breaking things) should go in before we iterate and improve. As a first step I'm happy with this, even if we don't get to improve things before the 8.1 release.

jospoortvliet · 2015-04-02T11:54:09Z

That is a first 👍 provided the popup text is improved but note I didn't (and won't) test.

LukasReschke · 2015-04-02T11:58:04Z

Adjusted. Thanks @jospoortvliet.

karlitschek · 2015-04-02T20:07:16Z

Awesome!!! I would add a "It is not recommended to install this app on a production server" to the warning. But beside that really good 👍

karlitschek · 2015-04-02T20:08:58Z

And I agree that a link to the author page would be nice :-)

Allows administrators to disable or enabled experimental applications as well as show the trust level.

LukasReschke · 2015-04-03T11:36:31Z

Rebased. The usernames are now also clickable. I use the profilepage entry for this. However, this means it links for example to http://opendesktop.org/usermanager/search.php?username=arkascha instead of https://apps.owncloud.com/usermanager/search.php?username=arkascha.

@karlitschek Would it be easier to add a hack to core or change this in the appstore code?

LukasReschke · 2015-04-03T11:40:34Z

@karlitschek Also this means we have to ensure that profilepage will never contain user controllable input, or at least check it begins with http:// or https:// otherwise if somebody inserts a profilepage like javascript:alert(1) this could be potentially bad…

LukasReschke · 2015-04-03T11:41:04Z

Mhm. Maybe I can use a JS or PHP function to ensure this in the ownCloud code base as well.

jancborchardt · 2015-04-07T09:56:19Z

@LukasReschke added some design enhancements to the tags.

I can’t fully test it though since for the pages for market apps I get »no apps for your version«. Why that? (I run ownCloud from git.)

LukasReschke · 2015-04-07T09:56:57Z

I can’t fully test it though since for the pages for market apps I get »no apps for your version«. Why that?

Changed the $OC_Version to 8.0? i.e. $OC_Version=array(8, 0, 0, 1); in version.php ? :-)

MorrisJobke · 2015-04-07T15:25:09Z

Code looks good and it works 👍

PVince81 · 2015-04-07T16:01:08Z

"Install on your own risk" => "Install at your own risk" http://ell.stackexchange.com/questions/13918/at-your-own-risk-or-on-your-own-risk

PVince81 · 2015-04-07T16:03:43Z

@Xenopathic @tomneedham native language checks please ?

I guess this still has time to get translated right ?

PVince81 · 2015-04-07T16:05:54Z

config/config.sample.php

"to experimental" => "to show" ? Or does it remove the checkbox ?

Does the checkbox change that setting ?

Tested this, this controls the checkbox state.

This should probably be 'Whether to show experimental apps...'

Indeed 🙈

PVince81 · 2015-04-07T16:51:32Z

Tested, works.

Can you add JS unit tests for testing the sort algo ?
There is already test code that renders each app block one by one, will need to be changed to call a mocked loadCategory with possibly a mocked response. Let me know if you need help.

LukasReschke · 2015-04-07T16:58:28Z

Let me look into that tomorrow. Glad that you didn't find anything other grave 😄

RobinMcCorkell · 2015-04-07T19:28:20Z

settings/js/apps.js

Too many 'and's in this sentence, you might want to reword it and/or add some commas. Also, 'serious use' => 'production use'

scrutinizer-notifier · 2015-04-08T20:09:05Z

A new inspection was created.

LukasReschke · 2015-04-08T20:09:24Z

Changed the wording and added a JS unit test for sorting as well.

LukasReschke · 2015-04-08T20:14:59Z

Not now @owncloud-bot …

ghost · 2015-04-08T20:41:17Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11333/
🚀 Test PASSed.🚀

MorrisJobke · 2015-04-08T22:04:46Z

My 👍 is still valid

PVince81 · 2015-04-09T08:07:15Z

👍

Add different trust levels to AppStore interface

LukasReschke · 2015-04-09T08:07:55Z

Awesome. Thanks for the review @PVince81 @MorrisJobke. And kudos for the nice design to @jancborchardt :-)

LukasReschke · 2015-04-09T08:13:50Z

@karlitschek Just to clarify some things about apps.owncloud.com, from my understanding:

When the user that created the app is the "owncloud" user it will get marked official?
When I mark an app as approved it will get marked as approved?
When "maximum" required =< the current ownCloud version and "minimum required" >= the current ownCloud version it will get shown in the ownCloud interface ?

Are these assumptions correct?

karlitschek · 2015-04-13T00:38:19Z

No. Being marked as "official" requires that an admin set´s this flag. I think this is the correct behavior. What do you think?
yes
yes

LukasReschke · 2015-04-13T10:58:47Z

No. Being marked as "official" requires that an admin set´s this flag. I think this is the correct behavior. What do you think?

That makes sense. So just to re-verify, if I'm modifying an existing app as admin:

Approval
- Approved: will get marked as approved
- Not approved: Experimental
Category
- Recommended: ??
- 3rdparty: ??

Guess I will only concentrate on the "Approval" switch then ,-)

jancborchardt · 2015-04-13T11:11:12Z

@karlitschek @LukasReschke since the new Official/Approved/Experimental is the new system, we can remove the Recommended/3rdparty, right?

karlitschek · 2015-04-13T11:12:47Z

@jancborchardt Yes. I would replace it with the new terms

LukasReschke · 2015-04-13T11:19:51Z

@karlitschek @LukasReschke since the new Official/Approved/Experimental is the new system, we can remove the Recommended/3rdparty, right?

Well the terms are already gone in the current web interface as far I can tell – I referred to the usage on apps.owncloud.com :-)

MorrisJobke · 2015-04-13T11:24:53Z

No. Being marked as "official" requires that an admin set´s this flag. I think this is the correct behavior. What do you think?

That makes sense. So just to re-verify, if I'm modifying an existing app as admin:
Approval
    Approved: will get marked as approved
    Not approved: Experimental

Can we document this somewhere? Because there will be people that ask for these terms and their meaning.

jancborchardt · 2015-04-13T11:33:20Z

Yes – basically there should just be one selection in apps.owncloud.com called »App trust level«. Default to »Experimental« for new apps, and app store reviewers can set it to »Approved«. (And only admins could set apps to »Official«, which would rarely happen though.)

LukasReschke added the 2 - Developing label Mar 30, 2015

LukasReschke mentioned this pull request Mar 30, 2015

Improve apps management with new app trust levels #15274

Closed

7 tasks

DeepDiver1975 added this to the 8.2-next milestone Mar 30, 2015

jancborchardt reviewed Mar 30, 2015
View reviewed changes

LukasReschke force-pushed the app-categories-15274 branch from 63b398a to f36bd70 Compare April 2, 2015 10:40

LukasReschke added 3 - To Review and removed 2 - Developing labels Apr 2, 2015

LukasReschke changed the title ~~WIP: Add different trust levels to AppStore interface~~ Add different trust levels to AppStore interface Apr 2, 2015

Add experimental applications switch

0816cf9

Allows administrators to disable or enabled experimental applications as well as show the trust level.

LukasReschke force-pushed the app-categories-15274 branch from a485bbc to 47f160f Compare April 3, 2015 11:33

Make user names clickable

25531ba

LukasReschke force-pushed the app-categories-15274 branch from 47f160f to 25531ba Compare April 3, 2015 11:36

app mgmt app level design enhancements

6250365

PVince81 reviewed Apr 7, 2015
View reviewed changes

RobinMcCorkell reviewed Apr 7, 2015
View reviewed changes

LukasReschke added 3 commits April 8, 2015 21:24

Fix wording

0cd29e0

Reference $(this) instead

eb1cf58

Add JS unit test for sorting

98698e0

LukasReschke added a commit that referenced this pull request Apr 9, 2015

Merge pull request #15314 from owncloud/app-categories-15274

ba52f6f

Add different trust levels to AppStore interface

LukasReschke merged commit ba52f6f into master Apr 9, 2015

LukasReschke deleted the app-categories-15274 branch April 9, 2015 08:07

lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019

Add different trust levels to AppStore interface #15314

Add different trust levels to AppStore interface #15314

Uh oh!

Conversation

LukasReschke commented Mar 30, 2015

Uh oh!

LukasReschke commented Mar 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jospoortvliet commented Mar 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukasReschke commented Apr 2, 2015

Uh oh!

LukasReschke commented Apr 2, 2015

Uh oh!

LukasReschke commented Apr 2, 2015

Uh oh!

jospoortvliet commented Apr 2, 2015

Uh oh!

jospoortvliet commented Apr 2, 2015

Uh oh!

LukasReschke commented Apr 2, 2015

Uh oh!

karlitschek commented Apr 2, 2015

Uh oh!

karlitschek commented Apr 2, 2015

Uh oh!

LukasReschke commented Apr 3, 2015

Uh oh!

LukasReschke commented Apr 3, 2015

Uh oh!

LukasReschke commented Apr 3, 2015

Uh oh!

jancborchardt commented Apr 7, 2015

Uh oh!

LukasReschke commented Apr 7, 2015

Uh oh!

MorrisJobke commented Apr 7, 2015

Uh oh!

PVince81 commented Apr 7, 2015

Uh oh!

PVince81 commented Apr 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Apr 7, 2015

Uh oh!

LukasReschke commented Apr 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scrutinizer-notifier commented Apr 8, 2015

Uh oh!

LukasReschke commented Apr 8, 2015

Uh oh!

LukasReschke commented Apr 8, 2015

Uh oh!

ghost commented Apr 8, 2015

Uh oh!

MorrisJobke commented Apr 8, 2015

Uh oh!

PVince81 commented Apr 9, 2015

Uh oh!

LukasReschke commented Apr 9, 2015

Uh oh!

LukasReschke commented Apr 9, 2015

Uh oh!