From 69c45b2f027aca235ceca306c81453c5c510430e Mon Sep 17 00:00:00 2001
From: Benedikt Kulmann <benedikt@kulmann.biz>
Date: Mon, 7 Mar 2022 17:09:25 +0100
Subject: [PATCH] Cleaned up account info page

Removed code duplication in template, limited loading indication to the
part of the template where actual loading happens (only the group
membership info).
---
 .../enhancement-account-page-loading          |   5 +
 packages/web-runtime/src/pages/account.vue    | 112 +++++++++---------
 .../pages/__snapshots__/account.spec.js.snap  |  21 ++--
 .../tests/unit/pages/account.spec.js          | 103 ++++++++--------
 4 files changed, 114 insertions(+), 127 deletions(-)
 create mode 100644 changelog/unreleased/enhancement-account-page-loading

diff --git a/changelog/unreleased/enhancement-account-page-loading b/changelog/unreleased/enhancement-account-page-loading
new file mode 100644
index 00000000000..0c19a12c8b0
--- /dev/null
+++ b/changelog/unreleased/enhancement-account-page-loading
@@ -0,0 +1,5 @@
+Enhancement: Don't block account page while groups are loading
+
+We don't show a loading state for the full account information page anymore while the group membership information is loading. Instead we only show a loading spinner for the group membership information, while the rest of the user information is available immediately.
+
+https://github.com/owncloud/web/pull/6547
diff --git a/packages/web-runtime/src/pages/account.vue b/packages/web-runtime/src/pages/account.vue
index f9ff330b96e..6192264b63b 100644
--- a/packages/web-runtime/src/pages/account.vue
+++ b/packages/web-runtime/src/pages/account.vue
@@ -1,12 +1,8 @@
 <template>
-  <div class="oc-width-1-1 oc-container oc-p">
-    <div v-if="loading" class="oc-flex oc-flex-between oc-flex-middle">
-      <h1 class="oc-page-title">{{ pageTitle }}</h1>
-      <oc-loader />
-    </div>
-    <template v-else>
-      <div class="oc-flex oc-flex-between oc-flex-middle">
-        <h1 id="account-page-title" v-translate class="oc-page-title">Account</h1>
+  <main id="account" class="oc-height-1-1 oc-m">
+    <div class="oc-flex oc-flex-between oc-flex-bottom oc-width-1-1 oc-border-b oc-py">
+      <h1 id="account-page-title" class="oc-page-title oc-m-rm">{{ pageTitle }}</h1>
+      <div>
         <oc-button
           v-if="editUrl"
           variation="primary"
@@ -28,51 +24,51 @@
           <translate>Edit</translate>
         </oc-button>
       </div>
-      <hr />
-      <h2 v-translate class="oc-text-bold oc-text-initial oc-mb">Account Information</h2>
-
-      <dl class="account-page-info oc-flex oc-flex-wrap">
-        <div class="account-page-info-username oc-mb oc-width-1-2@s">
-          <dt v-translate class="oc-text-normal oc-text-muted">Username</dt>
-          <dd>
-            {{ user.username || user.id }}
-          </dd>
-        </div>
-        <div v-if="user.username && user.id" class="account-page-info-userid">
-          <dt v-translate class="oc-text-normal oc-text-muted">User ID</dt>
-          <dd>
-            {{ user.id }}
-          </dd>
-        </div>
-        <div class="account-page-info-displayname oc-mb oc-width-1-2@s">
-          <dt v-translate class="oc-text-normal oc-text-muted">Display name</dt>
-          <dd>
-            {{ user.displayname }}
-          </dd>
-        </div>
-        <div class="account-page-info-email oc-mb oc-width-1-2@s">
-          <dt v-translate class="oc-text-normal oc-text-muted">Email</dt>
-          <dd>
-            <template v-if="user.email">{{ user.email }}</template>
-            <span v-else v-translate>No email has been set up</span>
-          </dd>
-        </div>
-        <div class="account-page-info-groups oc-mb oc-width-1-2@s">
-          <dt
-            v-translate
-            class="oc-text-normal oc-text-muted"
-            @click="$_oc_settingsAccount_getGroup"
+    </div>
+    <h2 v-translate class="oc-text-bold oc-text-initial oc-mb">Account Information</h2>
+    <dl class="account-page-info oc-flex oc-flex-wrap">
+      <div class="account-page-info-username oc-mb oc-width-1-2@s">
+        <dt v-translate class="oc-text-normal oc-text-muted">Username</dt>
+        <dd>
+          {{ user.username || user.id }}
+        </dd>
+      </div>
+      <div v-if="user.username && user.id" class="account-page-info-userid">
+        <dt v-translate class="oc-text-normal oc-text-muted">User ID</dt>
+        <dd>
+          {{ user.id }}
+        </dd>
+      </div>
+      <div class="account-page-info-displayname oc-mb oc-width-1-2@s">
+        <dt v-translate class="oc-text-normal oc-text-muted">Display name</dt>
+        <dd>
+          {{ user.displayname }}
+        </dd>
+      </div>
+      <div class="account-page-info-email oc-mb oc-width-1-2@s">
+        <dt v-translate class="oc-text-normal oc-text-muted">Email</dt>
+        <dd>
+          <template v-if="user.email">{{ user.email }}</template>
+          <span v-else v-translate>No email has been set up</span>
+        </dd>
+      </div>
+      <div class="account-page-info-groups oc-mb oc-width-1-2@s">
+        <dt v-translate class="oc-text-normal oc-text-muted" @click="loadGroups">
+          Group memberships
+        </dt>
+        <dd data-testid="group-names">
+          <oc-spinner
+            v-if="loadingGroups"
+            :aria-label="$gettext('Loading group membership information')"
+          />
+          <span v-else-if="groupNames">{{ groupNames }}</span>
+          <span v-else v-translate data-testid="group-names-empty"
+            >You are not part of any group</span
           >
-            Group memberships
-          </dt>
-          <dd>
-            <span v-if="groupNames">{{ groupNames }}</span>
-            <span v-else v-translate>You are not part of any group</span>
-          </dd>
-        </div>
-      </dl>
-    </template>
-  </div>
+        </dd>
+      </div>
+    </dl>
+  </main>
 </template>
 
 <script>
@@ -81,7 +77,7 @@ export default {
   name: 'Personal',
   data() {
     return {
-      loading: true,
+      loadingGroups: true,
       groups: []
     }
   },
@@ -108,14 +104,12 @@ export default {
     }
   },
   mounted() {
-    this.$_oc_settingsAccount_getGroup()
+    this.loadGroups()
   },
   methods: {
-    $_oc_settingsAccount_getGroup() {
-      this.$client.users.getUserGroups(this.user.id).then((groups) => {
-        this.groups = groups
-        this.loading = false
-      })
+    async loadGroups() {
+      this.groups = await this.$client.users.getUserGroups(this.user.id)
+      this.loadingGroups = false
     }
   }
 }
diff --git a/packages/web-runtime/tests/unit/pages/__snapshots__/account.spec.js.snap b/packages/web-runtime/tests/unit/pages/__snapshots__/account.spec.js.snap
index d0257971500..2a92e503fac 100644
--- a/packages/web-runtime/tests/unit/pages/__snapshots__/account.spec.js.snap
+++ b/packages/web-runtime/tests/unit/pages/__snapshots__/account.spec.js.snap
@@ -1,15 +1,6 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`account when the page is not in loading state anymore account information with group information 1`] = `
-<div class="account-page-info-groups oc-mb oc-width-1-2@s">
-  <dt class="oc-text-normal oc-text-muted">
-    Group memberships
-  </dt>
-  <dd><span>one, two, three</span></dd>
-</div>
-`;
-
-exports[`account when the page is not in loading state anymore account information without group information 1`] = `
+exports[`account page account information displays basic user information 1`] = `
 <dl class="account-page-info oc-flex oc-flex-wrap">
   <div class="account-page-info-username oc-mb oc-width-1-2@s">
     <dt class="oc-text-normal oc-text-muted">Username</dt>
@@ -32,19 +23,23 @@ exports[`account when the page is not in loading state anymore account informati
     <dt class="oc-text-normal oc-text-muted">
       Group memberships
     </dt>
-    <dd><span>You are not part of any group</span></dd>
+    <dd data-testid="group-names">
+      <oc-spinner-stub aria-label="Loading group membership information"></oc-spinner-stub>
+    </dd>
   </div>
 </dl>
 `;
 
-exports[`account when the page is not in loading state anymore edit buttons edit route button should be displayed if running with ocis and has navItems 1`] = `
+exports[`account page account information group membership displays group names 1`] = `<dd data-testid="group-names"><span>one, two, three</span></dd>`;
+
+exports[`account page header section edit buttons edit route button should be displayed if running with ocis and has navItems 1`] = `
 <oc-button-stub variation="primary" type="router-link" to="some-route" data-testid="account-page-edit-route-btn">
   <oc-icon-stub name="edit"></oc-icon-stub>
   <translate-stub tag="span">Edit</translate-stub>
 </oc-button-stub>
 `;
 
-exports[`account when the page is not in loading state anymore edit buttons edit url button should be displayed if not running with ocis 1`] = `
+exports[`account page header section edit buttons edit url button should be displayed if not running with ocis 1`] = `
 <oc-button-stub variation="primary" type="a" href="http://server/address/index.php/settings/personal" data-testid="account-page-edit-url-btn">
   <oc-icon-stub name="edit"></oc-icon-stub>
   <translate-stub tag="span">Edit</translate-stub>
diff --git a/packages/web-runtime/tests/unit/pages/account.spec.js b/packages/web-runtime/tests/unit/pages/account.spec.js
index f615c4d85f7..6f93854f6a8 100644
--- a/packages/web-runtime/tests/unit/pages/account.spec.js
+++ b/packages/web-runtime/tests/unit/pages/account.spec.js
@@ -24,43 +24,21 @@ const $route = {
 
 const selectors = {
   pageTitle: '.oc-page-title',
-  loaderStub: 'oc-loader-stub',
-  accountPageTitle: '#account-page-title',
+  loaderStub: 'oc-spinner-stub',
   editUrlButton: '[data-testid="account-page-edit-url-btn"]',
   editRouteButton: '[data-testid="account-page-edit-route-btn"]',
   accountPageInfo: '.account-page-info',
-  groupInformation: '.account-page-info-groups'
+  groupNames: '[data-testid="group-names"]',
+  groupNamesEmpty: '[data-testid="group-names-empty"]'
 }
 
-describe('account', () => {
-  describe('when the page is still loading', () => {
-    let wrapper
-    beforeEach(() => {
-      wrapper = getWrapper()
-      wrapper.setData({ loading: true })
-    })
-    it('should render the loading state', () => {
+describe('account page', () => {
+  describe('header section', () => {
+    it('renders page title', () => {
+      const wrapper = getWrapper()
       const pageTitle = wrapper.find(selectors.pageTitle)
-      const ocLoader = wrapper.find(selectors.loaderStub)
+      expect(pageTitle.exists()).toBeTruthy()
       expect(pageTitle.text()).toBe($route.meta.title)
-      expect(ocLoader.exists()).toBeTruthy()
-    })
-    it('should not render the account page content', () => {
-      const accountPageTitle = wrapper.find(selectors.accountPageTitle)
-      const accountPageInfo = wrapper.find(selectors.accountPageInfo)
-      expect(accountPageTitle.exists()).toBeFalsy()
-      expect(accountPageInfo.exists()).toBeFalsy()
-    })
-  })
-  describe('when the page is not in loading state anymore', () => {
-    it('should not render the loading state', async () => {
-      const store = getStore()
-      const wrapper = getWrapper(store)
-      await wrapper.setData({ loading: false })
-      const pageTitle = wrapper.find(selectors.pageTitle)
-      const ocLoader = wrapper.find(selectors.loaderStub)
-      expect(pageTitle.text()).toBe('Account')
-      expect(ocLoader.exists()).toBeFalsy()
     })
 
     describe('edit buttons', () => {
@@ -71,7 +49,7 @@ describe('account', () => {
             isOcis: false
           })
           const wrapper = getWrapper(store)
-          await wrapper.setData({ loading: false })
+          await wrapper.setData({ loadingGroups: false })
           const editUrlButton = wrapper.find(selectors.editUrlButton)
           const editRouteButton = wrapper.find(selectors.editRouteButton)
           expect(editUrlButton).toMatchSnapshot()
@@ -83,9 +61,9 @@ describe('account', () => {
             isOcis: true
           })
           const wrapper = getWrapper(store)
-          await wrapper.setData({ loading: false })
-          const editButton = wrapper.find(selectors.editUrlButton)
-          expect(editButton.exists()).toBeFalsy()
+          await wrapper.setData({ loadingGroups: false })
+          const editUrlButton = wrapper.find(selectors.editUrlButton)
+          expect(editUrlButton.exists()).toBeFalsy()
         })
       })
       describe('edit route button', () => {
@@ -95,42 +73,57 @@ describe('account', () => {
             getNavItemsByExtension: jest.fn(() => [{ route: 'some-route' }])
           })
           const wrapper = getWrapper(store)
-          await wrapper.setData({ loading: false })
+          await wrapper.setData({ loadingGroups: false })
           const editRouteButton = wrapper.find(selectors.editRouteButton)
           expect(editRouteButton).toMatchSnapshot()
         })
       })
     })
+  })
 
-    describe('account information', () => {
-      it('without group information', async () => {
-        const store = getStore({
-          user: {
-            username: 'some-username',
-            displayname: 'some-displayname',
-            email: 'some-email'
-          }
-        })
+  describe('account information', () => {
+    it('displays basic user information', async () => {
+      const store = getStore({
+        user: {
+          username: 'some-username',
+          displayname: 'some-displayname',
+          email: 'some-email'
+        }
+      })
+      const wrapper = getWrapper(store)
+
+      const accountPageInfo = wrapper.find(selectors.accountPageInfo)
+      expect(accountPageInfo).toMatchSnapshot()
+    })
+
+    describe('group membership', () => {
+      it('shows loading indicator when in loading state', () => {
+        const wrapper = getWrapper()
+        const groupInfoLoading = wrapper.find(selectors.loaderStub)
+        expect(groupInfoLoading.exists()).toBeTruthy()
+      })
+      it('displays message if not member of any groups', async () => {
+        const store = getStore()
         const wrapper = getWrapper(store)
-        await wrapper.setData({ loading: false })
+        await wrapper.setData({ loadingGroups: false })
 
-        const accountPageInfo = wrapper.find(selectors.accountPageInfo)
-        expect(accountPageInfo).toMatchSnapshot()
+        const groupNamesEmpty = wrapper.find(selectors.groupNamesEmpty)
+        expect(groupNamesEmpty.exists()).toBeTruthy()
       })
-      it('with group information', async () => {
+      it('displays group names', async () => {
         const store = getStore()
         const wrapper = getWrapper(store)
-        await wrapper.setData({ loading: false })
+        await wrapper.setData({ loadingGroups: false })
         await wrapper.setData({ groups: ['one', 'two', 'three'] })
 
-        const groupInformation = wrapper.find(selectors.groupInformation)
-        expect(groupInformation).toMatchSnapshot()
+        const groupNames = wrapper.find(selectors.groupNames)
+        expect(groupNames).toMatchSnapshot()
       })
     })
   })
 })
 
-function getWrapper(store = null) {
+function getWrapper(store = getStore()) {
   const component = {
     ...account,
     mounted: jest.fn()
@@ -141,12 +134,12 @@ function getWrapper(store = null) {
       $route
     },
     stubs: {
-      'oc-loader': true,
+      'oc-spinner': true,
       'oc-button': true,
       'oc-icon': true
-    }
+    },
+    store
   }
-  if (store) opts.store = store
   return shallowMount(component, opts)
 }