Migrate apps management to Vue.js by juliusknorr · Pull Request #9565 · nextcloud/server

juliusknorr · 2018-05-23T17:59:51Z

This PR will move the apps management to vue, heavily based on the work done by @skjnldsv in #8824

General improvements to the existing feature set:

Implement sidebar with app details Detail pages for apps #3193Show app details in apps management via right sidebar #2138
Add search across categories Find an app by name within nextcloud #6868
Move ajax endpoints to proper Controller Move endpoints to from apps management apps to AppSettingsController #7931
Make categories enabled/disabled/update filters, to reduce request Settings Apps Redesign Overview #3194
Fix "Your apps" does not show available updates "Your apps" does not show available updates #8703
Avoid updating multiple apps at once Updating multiple apps at once results in some not being updated #7581

skjnldsv · 2018-05-24T05:26:43Z

Weeeeeeee! Let me know if you need help!
I'll probably move the new components to the repo as soon as I can :)

codecov · 2018-05-25T13:52:55Z

Codecov Report

Merging #9565 into master will decrease coverage by 1.37%.
The diff coverage is 14.45%.

@@ Coverage Diff @@## master #9565 +/- ##
============================================
- Coverage 53.34% 51.96% -1.38% - Complexity 24451 25743 +1292 
============================================
Files 1478 1633 +155 Lines 81470 95346 +13876 Branches 0 1307 +1307 ============================================
+ Hits 43459 49551 +6092 - Misses 38011 45795 +7784

Impacted Files	Coverage Δ	Complexity Δ
settings/js/apps.js	`3.44% <ø> (ø)`	`0 <0> (?)`
settings/routes.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/legacy/app.php	`62.36% <0%> (-0.28%)`	`196 <0> (ø)`
lib/private/App/AppManager.php	`95.62% <100%> (+0.02%)`	`61 <0> (ø)`	⬇️
settings/Controller/AppSettingsController.php	`19.62% <15.09%> (-2.6%)`	`79 <75> (-10)`
.../tests/Unit/Collaboration/CommentersSorterTest.php	`25.55% <0%> (-66.45%)`	`6% <0%> (ø)`
apps/sharebymail/tests/SettingsTest.php	`52.17% <0%> (-47.83%)`	`3% <0%> (ø)`
lib/private/Security/RateLimiting/Limiter.php	`55.55% <0%> (-44.45%)`	`5% <0%> (ø)`
...b/private/App/AppStore/Fetcher/CategoryFetcher.php	`69.23% <0%> (-30.77%)`	`1% <0%> (ø)`
...ps/comments/tests/Unit/AppInfo/ApplicationTest.php	`69.56% <0%> (-30.44%)`	`4% <0%> (ø)`
... and 348 more

skjnldsv · 2018-05-25T22:14:06Z

+
+const mutations = {
+
+	APPS_API_FAILURE(state, error) {


Any reason not to use the global API_Failure?

I introduced this because the apps management controller endpoints are no OCS API at the moment, so the regular API_FAILURE will not work. I didn't move them to proper OCS since I wanted to keep backend code changes in the beginning. If nobody protests, i would tackle that in a follow up PR.

Guess who will earn a beer in Berlin if he makes that happen ;) 🍺

SO many beers :D

juliusknorr · 2018-06-02T08:17:42Z

Ready for review. @nextcloud/designers @nextcloud/javascript

rullzer · 2018-06-02T12:06:53Z

If I enable an app. The state is toggled in the sidebar. But not in the main view
I like the list overview (with the details). But a lot of the thumbnails in the list are to small to be meaningfull
For apps without a screenshot (and rating) the sidebar looks a bit off. Large space between the title and the author etc.

rullzer · 2018-06-02T12:03:06Z

+ </div>
+
+ <p class="documentation">
+ <a class="appslink" :href="appstoreUrl" v-if="!app.internal" target="_blank" rel="noreferrer noopener">Im Store anzeigen ↗</a>


Lets write that in English ;)
And make it translatatble

ChristophWurst

Good stuff, but quite a big PR to review 🙈

I commented on a few spots that might need polishing.

ChristophWurst · 2018-06-04T06:10:24Z

 use OCP\App\IAppManager;
 use OCP\App\ManagerEvent;
 use OCP\ICacheFactory;
+use OCP\IConfig;


the import on L35 is unused then, I guess 😉

Right, removed :)

ChristophWurst · 2018-06-04T06:11:14Z

- $this->appConfig->setValue($appId, 'enabled', 'no');
-
- // run uninstall steps
- $appData = $this->getAppInfo($appId);


was this removed intentionally?

ChristophWurst · 2018-06-04T06:12:29Z

+	}
+
+	public function getApp(string $appId) {
+ return $this->apps[$appId];


null checks, e.g. with ?? syntax would be great

ChristophWurst · 2018-06-04T06:17:22Z

- return new JSONResponse($this->getAllCategories());
+	public function listApps(): JSONResponse {
+
+ $this->fetchApps();


IMO this class should be split into multiple at some point. There's so much going on here and the code is quite hard to understand for me.

Toally with you on that. It will get a lot simpler once those methods are moved to proper OCS endpoints, but I'd do that in a follow up PR as mentioned in #9565 (comment)

ChristophWurst · 2018-06-04T06:19:58Z

 .catch((error) => context.commit('API_FAILURE', error));
 	},

+	getGroups(context) { /* { offset, limit, search } */


what do these comments mean?

I think it's a removed ES6 destructuring part of the function that was set aside?

juliusknorr · 2018-06-05T16:03:10Z

If I enable an app. The state is toggled in the sidebar. But not in the main view
@rullzer Cannot reproduce that. Can you provide some steps how and which enable button you have clicked?

I like the list overview (with the details). But a lot of the thumbnails in the list are to small to be meaningfull

I've changed the behavior there, so we always show the app icon in the list view.

All other comments should be fixed.

rullzer

I like it!

Lets get this in. And fix tiny things later if we find them!

rullzer · 2018-06-05T19:19:53Z

CI not happy

Signed-off-by: Julius Härtl <jus@bitgrid.net> Move app management ajax code to AppSettingsController Signed-off-by: Julius Härtl <jus@bitgrid.net>

Signed-off-by: Julius Härtl <jus@bitgrid.net>

juliusknorr · 2018-06-06T12:40:40Z

@rullzer Fixed the CI failures. 😉

MorrisJobke

Tested and works 👍

skjnldsv · 2018-06-06T16:07:33Z

🎉 ❤️

juliusknorr added 2. developing Work in progress feature: apps management labels May 23, 2018

juliusknorr added this to the Nextcloud 14 milestone May 23, 2018

juliusknorr added the enhancement label May 23, 2018

juliusknorr force-pushed the feature/noid/app-settings branch from 7a59cf0 to a40d969Compare May 25, 2018 13:52

skjnldsv reviewed May 25, 2018
View reviewed changes

juliusknorr force-pushed the feature/noid/app-settings branch 2 times, most recently from 7704564 to 4c27804Compare May 30, 2018 15:13

juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 2, 2018

juliusknorr requested review from ChristophWurst, MorrisJobke, danxuliu, rullzer and skjnldsv June 2, 2018 08:16

juliusknorr force-pushed the feature/noid/app-settings branch from e90021c to 1340a58Compare June 2, 2018 08:17

rullzer requested changes Jun 2, 2018
View reviewed changes

ChristophWurst reviewed Jun 4, 2018
View reviewed changes

juliusknorr force-pushed the feature/noid/app-settings branch 2 times, most recently from a5fe179 to 36ce686Compare June 5, 2018 15:48

rullzer approved these changes Jun 5, 2018
View reviewed changes

juliusknorr added 3 commits June 6, 2018 11:40

Move to AppFramework Controller
8594fdc
Signed-off-by: Julius Härtl <jus@bitgrid.net> Move app management ajax code to AppSettingsController Signed-off-by: Julius Härtl <jus@bitgrid.net>

Add file skeleton for app settings
125d1d3
Signed-off-by: Julius Härtl <jus@bitgrid.net>

Remove default categories from route endpoint
d9d5e97
Signed-off-by: Julius Härtl <jus@bitgrid.net>

juliusknorr added 6 commits June 6, 2018 11:40

Remove unneeded class
29f1c9b
Signed-off-by: Julius Härtl <jus@bitgrid.net>

Make view in store text translatable
9d6aa33
Signed-off-by: Julius Härtl <jus@bitgrid.net>

Add search/offset/limit parameters to group fetching
523cd0a
Signed-off-by: Julius Härtl <jus@bitgrid.net>

Add bundled code
0088326
Signed-off-by: Julius Härtl <jus@bitgrid.net>

Remove unused import
facce73
Signed-off-by: Julius Härtl <jus@bitgrid.net>

Remove space if no rating is available
c30af9b
Signed-off-by: Julius Härtl <jus@bitgrid.net>

juliusknorr force-pushed the feature/noid/app-settings branch from edece7c to c97d6d5Compare June 6, 2018 09:44

juliusknorr added 2 commits June 6, 2018 11:49

Keep AppConfig for now
f30c14a
Signed-off-by: Julius Härtl <jus@bitgrid.net>

Fix AppSettingsControllerTest
23ca2a4
Signed-off-by: Julius Härtl <jus@bitgrid.net>

juliusknorr force-pushed the feature/noid/app-settings branch from c97d6d5 to 23ca2a4Compare June 6, 2018 09:49

Remove old app settings test
bf03d43
Signed-off-by: Julius Härtl <jus@bitgrid.net>

MorrisJobke approved these changes Jun 6, 2018
View reviewed changes

MorrisJobke merged commit b49f8e4 into masterJun 6, 2018

MorrisJobke deleted the feature/noid/app-settings branch June 6, 2018 16:05

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 6, 2018

skjnldsv mentioned this pull request Jun 6, 2018
Vue apps issues #9767
Closed
7 tasks

juliusknorr mentioned this pull request Jun 6, 2018
Missing svg filter mixin for apps management #9771
Merged

This was referenced Jun 18, 2018
Appstore: allow search across all categories #9898
Closed
Search-as-you-type in apps management does not work anymore #9959
Closed
Apps-Management doesn't show missing dependencies #4581
Closed

klawdhfzasjhaa mentioned this pull request Sep 20, 2019
Add Release notes Infobutton to Apps page #6684
Closed

Uh oh!

Uh oh!

Conversation

juliusknorr commented May 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented May 24, 2018

Uh oh!

codecovBot commented May 25, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

juliusknorr commented Jun 2, 2018

Uh oh!

rullzer commented Jun 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristophWurst left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented Jun 5, 2018

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

rullzer commented Jun 5, 2018

Uh oh!

juliusknorr commented Jun 6, 2018

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Jun 6, 2018• 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!

5 participants

juliusknorr commented May 23, 2018•
edited
Loading

codecovBot commented May 25, 2018•
edited
Loading

skjnldsv commented Jun 6, 2018•
edited
Loading