[pull] master from gradido:master

frontend/src/components/UserSettings/UserThankYouCard.spec.js

@@ -146,11 +146,69 @@ describe('UserThankYouCard', () => {
           BButton,
           BFormInput,
           // The dialog's own machinery is not what is under test here, and the real one
-          // teleports its content out of the wrapper. This stub keeps the two things the
-          // tests do care about: it shows its content only while open, and it closes.
+          // teleports its content out of the wrapper. This stub keeps what the tests do
+          // care about: it shows its content only while open, and it carries the FOOTER
+          // buttons, because that is where the save button lives.
+          //
+          // ⚠️ The emitted payload has a `preventDefault`, and that is not decoration: the
+          // page writes `@ok.prevent`, which Vue compiles into a call on this object. A
+          // bare payload would throw here — and a stub that cannot take `.prevent` cannot
+          // test a dialog whose whole point is that it does not close itself.
           BModal: {
-            props: ['modelValue'],
-            template: '<div v-if="modelValue" class="modal-stub"><slot /></div>',
+            props: ['modelValue', 'okTitle', 'cancelTitle', 'okDisabled', 'cancelDisabled', 'busy'],
+            computed: {
+              // Exactly how the library computes them (`disableCancel = cancelDisabled ||
+              // busy`, `disableOk = okDisabled || busy`). Modelled rather than simplified:
+              // the page passes only `busy`, so a stub that read `okDisabled` alone would
+              // report both buttons live and quietly stop testing the guard.
+              disableOk() {
+                return Boolean(this.okDisabled || this.busy)
+              },
+              disableCancel() {
+                return Boolean(this.cancelDisabled || this.busy)
+              },
+            },
+            emits: ['ok', 'cancel', 'hide', 'update:modelValue'],
+            methods: {
+              // Cancel is the whole chain the real one runs: it announces itself, announces
+              // that the dialog is going, and then goes. A stub that only emitted `cancel`
+              // would leave the dialog standing and quietly turn every test about closing
+              // into a test about nothing.
+              onCancel() {
+                this.$emit('cancel', { preventDefault() {} })
+                this.$emit('hide', { preventDefault() {} })
+                this.$emit('update:modelValue', false)
+              },
+              /**
+               * ⛔ OK closes UNLESS the listener prevents it, which is what the real one
+               * does — and modelling that is the whole point of this stub.
+               *
+               * A stub that simply never closed on OK looked right (the page does prevent
+               * it) and measured nothing: removing `.prevent` from the page left all
+               * thirty-six tests green. The behaviour under test is a CONDITION, so the
+               * stub has to carry the condition, not the outcome the page happens to pick.
+               */
+              onOk() {
+                const event = {
+                  defaultPrevented: false,
+                  preventDefault() {
+                    this.defaultPrevented = true
+                  },
+                }
+                this.$emit('ok', event)
+                if (!event.defaultPrevented) {
+                  this.$emit('hide', { preventDefault() {} })
+                  this.$emit('update:modelValue', false)
+                }
+              },
+            },
+            template:
+              '<div v-if="modelValue" class="modal-stub"><slot />' +
+              '<button data-test="thank-you-card-dialog-cancel" :disabled="disableCancel"' +
+              ' @click="onCancel">{{ cancelTitle }}</button>' +
+              '<button data-test="thank-you-card-dialog-ok" :disabled="disableOk"' +
+              ' @click="onOk">{{ okTitle }}</button>' +
+              '</div>',
           },
           // AppModal teleports to body, so its content would leave the wrapper. The stub
           // keeps the two things the tests care about: it shows while open, and its ok
@@ -309,6 +367,65 @@ describe('UserThankYouCard', () => {
       expect(wrapper.find('.modal-stub').exists()).toBe(false)
     })
 
+    /**
+     * ⛔ The half the dialog's own footer would get wrong on its own. A BModal closes when
+     * its OK is pressed; here it must not, because the PIN may come back refused and the
+     * message about it would land on a screen that no longer shows the field it is about --
+     * with the six digits gone, so there is nothing to correct either.
+     *
+     * That is what `@ok.prevent` buys, and it is one dropped modifier away from being lost
+     * silently: the happy path above stays green either way.
+     */
+    it('keeps the dialog standing when the server refuses the pin', async () => {
+      mockSaveSettings.mockRejectedValue(new Error('pin too easy'))
+      await mountWith()
+      await buttonWith('thank-you-card.settings.change-pin').trigger('click')
+      await field('new-pin').setValue('407312')
+      await dialogButtonWith('form.save').trigger('click')
+      await flushPromises()
+
+      expect(wrapper.find('.modal-stub').exists()).toBe(true)
+      expect(field('new-pin').element.value).toBe('407312')
+    })
+
+    /**
+     * ⛔ While the PIN is on its way, NEITHER button may be pressed — which is why the page
+     * passes `busy` rather than `ok-disabled`: the library derives both from it. With only
+     * OK guarded, Cancel stayed live during the save, and pressing it shut the dialog on a
+     * request that was still running.
+     *
+     * (Deliberately not sealed any further: the x, Escape and the backdrop still work. A
+     * request that hangs must not leave anybody locked in a box with two dead buttons.)
+     */
+    it('takes both buttons out of reach while the pin is on its way', async () => {
+      let finish
+      mockSaveSettings.mockReturnValue(new Promise((resolve) => (finish = resolve)))
+      await mountWith()
+      await buttonWith('thank-you-card.settings.change-pin').trigger('click')
+      await field('new-pin').setValue('407312')
+      await dialogButtonWith('form.save').trigger('click')
+      await nextTick()
+
+      expect(field('dialog-ok').attributes('disabled')).toBeDefined()
+      expect(field('dialog-cancel').attributes('disabled')).toBeDefined()
+
+      finish({})
+      await flushPromises()
+    })
+
+    // Backing out has to leave nothing behind: until this dialog had a Cancel at all, the
+    // only way out was the little x, and a half-typed PIN sat there until the next visit.
+    it('forgets a half-typed pin when the dialog is closed again', async () => {
+      await mountWith()
+      await buttonWith('thank-you-card.settings.change-pin').trigger('click')
+      await field('new-pin').setValue('4073')
+      await field('dialog-cancel').trigger('click')
+      await flushPromises()
+      await buttonWith('thank-you-card.settings.change-pin').trigger('click')
+
+      expect(field('new-pin').element.value).toBe('')
+    })
+
     // ⛔ Without the fallbacks an empty field would send 0, and a limit of zero is a card
     // that cannot pay anything - switched on, and useless, with nothing saying why.
     it('falls back to a usable pair of limits when the fields are empty', async () => {

frontend/src/components/UserSettings/UserThankYouCard.vue

@@ -144,7 +144,41 @@
       </p>
     </AppModal>
 
-    <BModal v-model="showSetup" :title="$t('thank-you-card.settings.pin-title')" hide-footer>
+    <!--
+      ⛔ `no-footer`, NOT `hide-footer`. bootstrap-vue-next renamed the prop; the old name is
+      accepted silently as a plain attribute and does nothing, so the dialog kept its default
+      footer and the panel showed TWO sets of buttons -- a save button of its own plus an
+      untranslated OK/Cancel pair underneath. Nothing warns about this.
+
+      The footer is the answer rather than something to hide: a dialog's actions belong in
+      it, and it brings the Cancel this dialog never had -- until now the only way out was
+      the little x in the corner.
+
+      ⚠️ `@ok.prevent`, because the dialog must NOT close itself: `savePin` closes it only
+      after the server has taken the PIN. A rejected PIN has to leave the dialog standing,
+      or the message lands on a screen that no longer shows the field it is about.
+
+      ⚠️ `busy`, not `ok-disabled`: the library computes both buttons from it
+      (`disableCancel = cancelDisabled || busy`, `disableOk = okDisabled || busy`), so a save
+      in flight takes Cancel out of reach too. Read in the shipped bundle, not assumed.
+
+      ⛔ And deliberately NOT `no-close-on-backdrop` / `no-close-on-esc` / `no-header-close`,
+      although a reviewer asked for them. Dismissing mid-save costs nothing: the mutation is
+      already sent, `run` still refetches and reports, and `@hide` clears the field -- the
+      worst case is a success message arriving after the box is gone. Sealing all three would
+      buy that back at the price of a request that hangs leaving somebody locked in a dialog
+      with both buttons dead and no way out at all, because `run` has no timeout. The x stays.
+    -->
+    <BModal
+      v-model="showSetup"
+      :title="$t('thank-you-card.settings.pin-title')"
+      :ok-title="$t('form.save')"
+      ok-variant="gradido"
+      :cancel-title="$t('form.cancel')"
+      :busy="busy"
+      @ok.prevent="savePin"
+      @hide="newPin = ''"
+    >
       <!--
         The rules carry an id so the field can point at them: a screen reader then reads
         what the PIN may be WITH the field, rather than leaving it behind as a paragraph
@@ -192,9 +226,6 @@
           </BButton>
         </template>
       </BInputGroup>
-      <BButton class="mt-3" variant="gradido" :disabled="busy" @click="savePin">
-        {{ $t('form.save') }}
-      </BButton>
     </BModal>
   </div>
 </template>

frontend/src/layouts/DashboardLayout.spec.js

@@ -125,10 +125,71 @@ describe('DashboardLayout', () => {
   })
 
   afterEach(() => {
+    // ⚠️ Every test mounts a layout and none took it down again, so they piled up and each
+    // one kept listening. Harmless while nothing in here reacted to anything global — and
+    // wrong the moment something did: a route change reached FOUR live layouts and the
+    // refetch spy counted four calls for one navigation.
+    wrapper?.unmount()
     vi.clearAllMocks()
     vi.clearAllTimers()
   })
 
+  /**
+   * The balance in the header is fetched once, when this layout mounts — and the layout
+   * outlives every route change. Until this watch existed, only a page that said so kept it
+   * current (`Send` after a transfer, `Transactions` on paging), so a payment made anywhere
+   * else left the old number standing everywhere the member went next. A thank you card
+   * payment is exactly that: its own page, at somebody else's till, saying nothing here.
+   */
+  describe('the balance in the header', () => {
+    it('asks again when the member opens the overview', async () => {
+      await router.push('/transactions')
+      mockRefetchFn.mockClear()
+
+      await router.push('/overview')
+      await nextTick()
+
+      expect(mockRefetchFn).toHaveBeenCalledTimes(1)
+    })
+
+    it('asks again when the member opens the transactions', async () => {
+      await router.push('/overview')
+      mockRefetchFn.mockClear()
+
+      await router.push('/transactions')
+      await nextTick()
+
+      expect(mockRefetchFn).toHaveBeenCalledTimes(1)
+    })
+
+    /**
+     * ⚠️ With NO arguments. `refetch(variables)` replaces them, so passing the paging ones
+     * along would send somebody sitting on page three back to page one every time they
+     * glanced at their balance. Empty means "the same question again".
+     */
+    it('asks the same question again, rather than resetting the paging', async () => {
+      await router.push('/settings')
+      mockRefetchFn.mockClear()
+
+      await router.push('/transactions')
+      await nextTick()
+
+      expect(mockRefetchFn).toHaveBeenCalledWith()
+    })
+
+    // The counterpart, and the one that keeps this from becoming "refetch on every click":
+    // it is the two pages that show a balance, not the whole wallet.
+    it('leaves other pages alone', async () => {
+      await router.push('/overview')
+      mockRefetchFn.mockClear()
+
+      await router.push('/settings')
+      await nextTick()
+
+      expect(mockRefetchFn).not.toHaveBeenCalled()
+    })
+  })
+
   it('renders DIV .main-page', () => {
     expect(wrapper.find('div.main-page').exists()).toBe(true)
   })
@@ -213,6 +274,27 @@ describe('DashboardLayout', () => {
         onErrorHandler({ message: 'Ouch!' })
         expect(toastErrorSpy).toHaveBeenCalledWith('Ouch!')
       })
+
+      /**
+       * ⛔ `pending` is handed down to the page inside the router-view, so a refetch that
+       * fails must not leave it standing — that page would wait for something that is never
+       * coming. It mattered less while only a deliberate action set it; now that opening the
+       * overview or the transactions sets it, one failed request would strand whatever the
+       * member opened next. (coderabbit, #3763)
+       */
+      it('stops the page waiting when the refetch fails', async () => {
+        await router.push('/overview')
+        await nextTick()
+        // Read off the stub's rendered attributes: `RouterView: true` makes a stub that
+        // declares no props, so what the layout hands down arrives as attrs, not props.
+        const pendingNow = () => wrapper.find('router-view-stub').attributes('pending')
+        expect(pendingNow()).toBe('true')
+
+        onErrorHandler({ message: 'Ouch!' })
+        await nextTick()
+
+        expect(pendingNow()).toBe('false')
+      })
     })
 
     it('has a component Navbar', () => {

frontend/src/layouts/DashboardLayout.vue

@@ -198,7 +198,7 @@
 </template>
 
 <script setup>
-import { computed, onMounted, ref } from 'vue'
+import { computed, onMounted, ref, watch } from 'vue'
 import { useStore } from 'vuex'
 import { useRoute, useRouter } from 'vue-router'
 import { useQuery, useMutation } from '@vue/apollo-composable'
@@ -281,6 +281,36 @@ const updateTransactions = ({ currentPage, pageSize }) => {
   useRefetchTransactionsQuery({ currentPage, pageSize })
 }
 
+/**
+ * The two pages a member opens to check where they stand.
+ *
+ * ⛔ The balance in the header is fetched ONCE, when this layout mounts -- and the layout
+ * outlives every route change, so nothing brings it up to date on its own. Until now the
+ * only thing that refreshed it was a page saying so: `Send` emits `update-transactions`
+ * after a transfer, `Transactions` on paging. A payment made anywhere ELSE left the old
+ * number standing on every screen the member visited afterwards -- and a thank you card
+ * payment happens on its own page, at somebody else's till, and says nothing to this layout.
+ *
+ * ⚠️ Not a cache policy and not a page reload. The query already asks the server
+ * (`network-only`); it simply never ran a second time. A reload would have hidden that by
+ * throwing the whole application away, which is why it looked like an answer.
+ *
+ * ⚠️ Refetched with NO arguments: Apollo then reuses the variables the query already has,
+ * so somebody sitting on page three of their transactions is not sent back to page one.
+ */
+const PAGES_SHOWING_A_BALANCE = ['/overview', '/transactions']
+
+watch(
+  () => route.path,
+  (path) => {
+    if (!PAGES_SHOWING_A_BALANCE.includes(path)) {
+      return
+    }
+    pending.value = true
+    useRefetchTransactionsQuery()
+  },
+)
+
 onResult((value) => {
   if (value && value.data) {
     if (value.data.transactionList) {
@@ -302,6 +332,12 @@ onResult((value) => {
 
 onError((error) => {
   transactionCount.value = -1
+  // ⚠️ Cleared here too, not only on the way that succeeds. `pending` is handed to the page
+  // inside the router-view, so a refetch that fails leaves that page waiting for something
+  // that is never coming. It mattered less while only a deliberate action set it; since the
+  // watch above sets it on every navigation to those two pages, one failed request would
+  // strand whatever the member opened next.
+  pending.value = false
   toastError(error.message)
 })