Page MenuHomePhabricator

[Cashtab] Improve alias error handling
ClosedPublic

Authored by emack on Feb 18 2024, 22:23.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC229089d71eba: [Cashtab] Improve alias error handling
Summary

As per feedback in D15466, removing the redundant setIsValidAliasInput(false) calls upon server errors.

The remaining instances of setIsValidAliasInput(false) in handleAliasWalletChange, clearInputForms and handleAliasNameInput are untouched as they are genuine alias validation/availability logic.

Test Plan
  • npm test
  • Register Alias button is enabled upon input of a valid and available alias, and registers all ok through the confirmation modal.
  • Register Alias button is disabled upon input of a valid but unavailable alias

Diff Detail

Repository
rABC Bitcoin ABC
Branch
aliasValidationFix
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 27244
Build 54051: Build Diffcashtab-tests
Build 54050: arc lint + arc unit

Event Timeline

emack published this revision for review.Feb 18 2024, 22:28
emack added a reviewer: bytesofman.

FYI - using arc diff --nolint for now until I can resolve the arc lint exceptions. My python was already v3.12 and it didn't seem to fix it. Have also tested with versions < 3.12.

dunno if it's related to this diff but

  • enter alias of multiple characters
  • backspace to nothing
  • get many many notifications of server error

image.png (183×437 px, 17 KB)

image.png (778×437 px, 69 KB)

update: I get this in master as well, so seems to be an unrelated issue

we should show an 'error retrieiving pricing' error if there is one ... but a pricing error does not imply an error in retrieving existing aliases

image.png (434×503 px, 15 KB)

in practice this is probably always true now since they come from the same server. but it isn't the way we should do ux.

if prices are unavailable, user should see one clear msg that says "Failed to fetch alias price information from server. Alias registration disabled. Refresh page to try again." Something like this.

ok to consolidate -- for this diff to be ok, we need the price failure error to appear under the input field. This error trumps all the other possible input validations, since we won't register anything if there is a price failure. The price failure error should not prevent rendering of registered and pending aliases. Those should only not be rendered if there is a server error in fetching them.

This revision now requires changes to proceed.Feb 18 2024, 23:12

Replaced setAliasServerError() calls which was the trigger to not render registered and pending alias lists in dropdowns. Now using setAliasValidationError() to hold the error displayed below the input field.

The price failure error should not prevent rendering of registered and pending aliases. Those should only not be rendered if there is a server error in fetching them.

This has now been adjusted to only prevent rendering when aliasValidationError is false and aliasServerError exists, i.e. if and only if server error.

Note: when you're re-testing during review, since the price error is simulated by malforming the alias-server url, it will result in no registered/pending aliases being rendered.

Note: when you're re-testing during review, since the price error is simulated by malforming the alias-server url, it will result in no registered/pending aliases being rendered.

This is the kind of thing that should not be too difficult to add an integration test for. Since we have none at all for the Alias component, please add this in this diff using the standardized wrappers.

Use jest when to mock an error from the price endpoint and a good return from the registered aliases endpoint(s). Configure.test.js is a good model to follow for integration tests using the standardized wrapper on a specific route + opening collapses, clicking buttons.

This revision now requires changes to proceed.Feb 19 2024, 21:29
emack edited the test plan for this revision. (Show Details)

Added integration tests that validates behavior between aliasValidationError and aliasServerError states.

bytesofman added inline comments.
cashtab/src/components/Alias/__tests__/Alias.test.js
87–101 ↗(On Diff #45528)

should be able to do just one test here, e.g.

await waitFor(() => {
            expect(
                screen.getByTestId('registered-aliases-list'),
            ).toHaveTextContent('chicken555.xecchicken666.xecchicken444.xec');
        });
197 ↗(On Diff #45528)

why does no price trigger aliasValidationError? sounds like a price error

This revision now requires changes to proceed.Feb 23 2024, 14:39

why does no price trigger aliasValidationError?

This refers to the state var that render errors underneath the alias input field. The earlier feedback was for price errors to be displayed in the same place, as well as to consolidate these vars that disable the register button, hence I decided to use this same var instead of another priceError var. Maybe I can rename it to 'aliasParsingError' to be more generic?

emack marked an inline comment as done.

Combined similar .toHaveTextContent() tests

why does no price trigger aliasValidationError?

This refers to the state var that render errors underneath the alias input field. The earlier feedback was for price errors to be displayed in the same place, as well as to consolidate these vars that disable the register button, hence I decided to use this same var instead of another priceError var. Maybe I can rename it to 'aliasParsingError' to be more generic?

It's okay to have two vars if they mean two different things. A validation error is one thing, a bad price response from the server is another thing. If we want both of these cases to show an error in the same place, we handle the logic at that place. For example, the rendering logic of an error in the validation error field can be priceError || validationError.

Minimizing state fields is good but there is also a reason to have more than zero, more than one, more than two, etc.

We don't want to just make a price error set validationError to true. Maybe this works fine at first, but would be very difficult to debug later if a dev keeps seeing a validation error when there is no validation error and has no idea that 300 diffs back someone made price errors always trigger the validation flag.

anyway i don't like how it's currently managed, but this diff still improves things and adds tests. no blocker

This revision is now accepted and ready to land.Feb 25 2024, 00:08

Failed tests logs:

====== CashTab Unit Tests: <Home /> Renders Sideshift button if user loads with a new wallet ======
Error: Unable to find role="button" and name `/Exchange to XEC via SideShift/`

Ignored nodes: comments, script, style
<body>
  <div>
    <div
      class="ant-spin-nested-loading css-dev-only-do-not-override-1rqnfsa"
    >
      <div>
        <div
          aria-busy="true"
          aria-live="polite"
          class="ant-spin ant-spin-spinning css-dev-only-do-not-override-1rqnfsa"
        >
          <span
            aria-label="loading"
            class="anticon anticon-loading anticon-spin cashLoadingIcon ant-spin-dot"
            role="img"
          >
            <svg
              aria-hidden="true"
              data-icon="loading"
              fill="currentColor"
              focusable="false"
              height="1em"
              viewBox="0 0 1024 1024"
              width="1em"
            >
              <path
                d="M988 548c-19.9 0-36-16.1-36-36 0-59.4-11.6-117-34.6-171.3a440.45 440.45 0 00-94.3-139.9 437.71 437.71 0 00-139.9-94.3C629 83.6 571.4 72 512 72c-19.9 0-36-16.1-36-36s16.1-36 36-36c69.1 0 136.2 13.5 199.3 40.3C772.3 66 827 103 874 150c47 47 83.9 101.8 109.7 162.7 26.7 63.1 40.2 130.2 40.2 199.3.1 19.9-16 36-35.9 36z"
              />
            </svg>
          </span>
        </div>
      </div>
      <div
        class="ant-spin-container ant-spin-blur"
      >
        <div
          class="sc-LKuAh jSXNAo"
        >
          <div
            class="sc-kZmsYB eHkWzo"
          >
            <div
              class="sc-RcBXQ gMCskf"
            >
              <div
                class="sc-hzNEM iiFjIO"
              >
                <div
                  class="sc-iSDuPN ccpgHi"
                >
                  <img
                    alt="cashtab"
                    class="sc-fZwumE bcurWd"
                    src="test-file-stub"
                  />
                </div>
                <div
                  class="sc-elJkPf geNavj"
                  data-testid="wallet-info-ctn"
                >
                  <div
                    class="sc-dEoRIm iVOOxs"
                  >
                    <div
                      class="sc-jtggT dNidya"
                    >
                      Transaction Fixtures
                    </div>
                    <a
                      href="/configure"
                    >
                      <svg
                        class="sc-jTzLTM bTdWCF"
                      />
                    </a>
                    <div>
                       
                      <button
                        aria-checked="true"
                        class="ant-switch ant-switch-small css-dev-only-do-not-override-1rqnfsa ant-switch-checked"
                        role="switch"
                        type="button"
                      >
                        <div
                          class="ant-switch-handle"
                        />
                        <span
                          class="ant-switch-inner"
                        >
                          <span
                            class="ant-switch-inner-checked"
                          >
                            <svg
                              class="sc-cSHVUG bgwEHu"
                            />
                          </span>
                          <span
                            class="ant-switch-inner-unchecked"
                          >
                            <svg
                              class="sc-kAzzGY fUTkYj"
                            />
                          </span>
                        </span>
                      </button>
                    </div>
                  </div>
                  <div
                    class="sc-ebFjAB kxaSkj"
                    data-testid="balance-xec"
                  >
                    0.00
                     
                    XEC
                     
                  </div>
                  <div
                    class="sc-jKVCRD bKzIGa"
                    data-testid="balance-fiat"
                  >
                    $
                    0.00
                     
                    USD
                  </div>
                  <p
                    class="sc-kaNhvL kDOvrh"
                    data-testid="ecash-price"
                  >
                    1 
                    XEC
                     = 
                    0.00003000
                     
                    USD
                  </p>
                </div>
              </div>
              <div
                class="sc-jwKygS hNrbtP"
                data-testid="loading-ctn"
              />
            </div>
            <div
              class="sc-iBEsjs jmPiFQ"
            >
              <button
                class="sc-gmeYpB dpikyJ"
              >
                <svg />
              </button>
              <button
                class="sc-gmeYpB eQwTbZ"
                data-testid="nav-btn-send"
              >
                <svg
                  class="sc-kEYyzF dWIuoY"
                  style="margin-top: -9px;"
                />
              </button>
              <button
                class="sc-gmeYpB eQwTbZ"
                data-testid="nav-btn-etokens"
              >
                <span
                  aria-label="appstore-add"
                  class="anticon anticon-appstore-add"
                  role="img"
                  style="font-size: 24px;"
                >
                  <svg
                    aria-hidden="true"
                    data-icon="appstore-add"
                    fill="currentColor"
                    focusable="false"
                    height="1em"
                    viewBox="64 64 896 896"
                    width="1em"
                  >
                    <defs />
                    <path
                      d="M464 144H160c-8.8 0-16 7.2-16 16v304c0 8.8 7.2 16 16 16h304c8.8 0 16-7.2 16-16V160c0-8.8-7.2-16-16-16zm-52 268H212V212h200v200zm452-268H560c-8.8 0-16 7.2-16 16v304c0 8.8 7.2 16 16 16h304c8.8 0 16-7.2 16-16V160c0-8.8-7.2-16-16-16zm-52 268H612V212h200v200zm52 132H560c-8.8 0-16 7.2-16 16v304c0 8.8 7.2 16 16 16h304c8.8 0 16-7.2 16-16V560c0-8.8-7.2-16-16-16zm-52 268H612V612h200v200zM424 712H296V584c0-4.4-3.6-8-8-8h-48c-4.4 0-8 3.6-8 8v128H104c-4.4 0-8 3.6-8 8v48c0 4.4 3.6 8 8 8h128v128c0 4.4 3.6 8 8 8h48c4.4 0 8-3.6 8-8V776h128c4.4 0 8-3.6 8-8v-48c0-4.4-3.6-8-8-8z"
                    />
                  </svg>
                </span>
              </button>
              <button
                class="sc-gmeYpB eQwTbZ"
                data-testid="nav-btn-receive"
              >
                <svg />
              </button>
              <div
                class="sc-chbbiW ktwTIv"
                data-testid="hamburger"
              >
                <span
                  class="sc-kxynE bbUulF"
                />
                <div
                  class="sc-cooIXK gkGTON"
                  data-testid="hamburger-menu"
                >
                  <button
                    class="sc-fcdeBU RXlWo"
                    data-testid="nav-btn-airdrop"
                  >
                     
                    <p>
                      Airdrop
                    </p>
                    <svg
                      height="33px"
                      width="30px"
                    />
                  </button>
                  <button
                    class="sc-fcdeBU RXlWo"
                    data-testid="nav-btn-swap"
                  >
                     
                    <p>
                      Swap
                    </p>
                    <span
                      aria-label="swap"
                      class="anticon anticon-swap"
                      role="img"
                      style="font-size: 24px;"
                    >
                      <svg
                        aria-hidden="true"
                        data-icon="swap"
                        fill="currentColor"
                        focusable="false"
                        height="1em"
                        viewBox="64 64 896 896"
                        width="1em"
                      >
                        <path
                          d="M847.9 592H152c-4.4 0-8 3.6-8 8v60c0 4.4 3.6 8 8 8h605.2L612.9 851c-4.1 5.2-.4 13 6.3 13h72.5c4.9 0 9.5-2.2 12.6-6.1l168.8-214.1c16.5-21 1.6-51.8-25.2-51.8zM872 356H266.8l144.3-183c4.1-5.2.4-13-6.3-13h-72.5c-4.9 0-9.5 2.2-12.6 6.1L150.9 380.2c-16.5 21-1.6 51.8 25.1 51.8h696c4.4 0 8-3.6 8-8v-60c0-4.4-3.6-8-8-8z"
                        />
                      </svg>
                    </span>
                  </button>
                  <button
                    class="sc-fcdeBU RXlWo"
                    data-testid="nav-btn-signverifymsg"
                  >
                    <p>
                      Sign & Verify
                    </p>
                    <svg
                      class="sc-chPdSV kiHcnD"
                    />
                  </button>
                  <button
                    class="sc-fcdeBU RXlWo"
                    data-testid="nav-btn-configure"
                  >
                    <p>
                      Settings
                    </p>
                    <svg
                      height="33px"
                      width="30px"
                    />
                  </button>
                </div>
              </div>
            </div>
          </div>
        </div>
      </div>
    </div>
  </div>
</body>
    at waitForWrapper (/work/cashtab/node_modules/@testing-library/dom/dist/wait-for.js:163:27)
    at /work/cashtab/node_modules/@testing-library/dom/dist/query-helpers.js:86:33
    at Object.findByRole (/work/cashtab/src/components/Home/__tests__/Home.test.js:128:26)

Each failure log is accessible here:
CashTab Unit Tests: <Home /> Renders Sideshift button if user loads with a new wallet

This revision was automatically updated to reflect the committed changes.