Page MenuHomePhabricator

[Cashtab] Move contact lists to own screen
ClosedPublic

Authored by bytesofman on Apr 4 2024, 05:21.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABC8b3820167f3d: [Cashtab] Move contact lists to own screen
Summary

T2207

Move contact list to own screen. Rebuild to use better styles and code.

Ensure same features are met by upgrading integration tests.

Once again, way too much time spent on Contacts. But, has to be done if we are to maintain the feature. Some upside here is that we are getting lots of lessons learned and best practices for managing formData and modals. Compare the implementation in Configure with the new Contacts implementation.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
contact-list-new-screen
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 28375
Build 56292: Build Diffcashtab-tests
Build 56291: arc lint + arc unit

Event Timeline

Tail of the build log:

  vectors.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
 src/slpv1                         |   95.32 |    89.09 |     100 |   95.32 |                                                                                                                                                                                                                                               
  index.js                         |   95.32 |    89.09 |     100 |   95.32 | 264,275,327,331,336                                                                                                                                                                                                                           
 src/slpv1/fixtures                |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  mocks.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  vectors.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
 src/transactions                  |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  index.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
 src/transactions/fixtures         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  mocks.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  vectors.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
 src/utils                         |   88.88 |    86.66 |     100 |   88.88 |                                                                                                                                                                                                                                               
  cashMethods.js                   |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  formatting.js                    |   84.84 |    81.81 |     100 |   84.84 | 18,28,51-53                                                                                                                                                                                                                                   
 src/utils/fixtures                |       0 |        0 |       0 |       0 |                                                                                                                                                                                                                                               
  vectors.js                       |       0 |        0 |       0 |       0 |                                                                                                                                                                                                                                               
 src/validation                    |   95.23 |    97.31 |     100 |   95.55 |                                                                                                                                                                                                                                               
  index.js                         |   95.23 |    97.31 |     100 |   95.55 | 58-63,125-126,268,300,320,367,611-612,745,750                                                                                                                                                                                                 
 src/validation/fixtures           |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  mocks.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  vectors.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
 src/wallet                        |   94.29 |     86.9 |   83.67 |   94.75 |                                                                                                                                                                                                                                               
  context.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  index.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  useWallet.js                     |   91.53 |    80.53 |   74.19 |   92.21 | 60,144,417-423,462,522,562,589,611-619,659,691,722,728-731,805-806,831                                                                                                                                                                        
 src/wallet/fixtures               |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  mocks.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
  vectors.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                               
-----------------------------------|---------|----------|---------|---------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='3347']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='4252']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='1637']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='2206']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='712']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='994']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='3305']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='4179']
##teamcity[blockClosed name='Code Coverage Summary']

Summary of all failing tests
FAIL src/components/Contacts/__tests__/index.test.js
  ● Test suite failed to run

    Your test suite must contain at least one test.

      at onResult (node_modules/@jest/core/build/TestScheduler.js:133:18)
      at node_modules/@jest/core/build/TestScheduler.js:254:19
      at node_modules/emittery/index.js:363:13
          at Array.map (<anonymous>)
      at Emittery.emit (node_modules/emittery/index.js:361:23)


Test Suites: 1 failed, 30 passed, 31 total
Tests:       699 passed, 699 total
Snapshots:   0 total
Time:        39.745 s
Ran all test suites.
Build cashtab-tests failed with exit code 1

better styles, component works, modal improvements

Tail of the build log:

  vectors.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
 src/slpv1                         |   95.32 |    89.09 |     100 |   95.32 |                                                                                                                                                                                                                                     
  index.js                         |   95.32 |    89.09 |     100 |   95.32 | 264,275,327,331,336                                                                                                                                                                                                                 
 src/slpv1/fixtures                |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  mocks.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  vectors.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
 src/transactions                  |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  index.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
 src/transactions/fixtures         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  mocks.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  vectors.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
 src/utils                         |   89.58 |    88.23 |     100 |   89.58 |                                                                                                                                                                                                                                     
  cashMethods.js                   |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  formatting.js                    |   86.11 |    84.61 |     100 |   86.11 | 18,28,51-53                                                                                                                                                                                                                         
 src/utils/fixtures                |       0 |        0 |       0 |       0 |                                                                                                                                                                                                                                     
  vectors.js                       |       0 |        0 |       0 |       0 |                                                                                                                                                                                                                                     
 src/validation                    |   95.17 |    97.09 |     100 |   95.47 |                                                                                                                                                                                                                                     
  index.js                         |   95.17 |    97.09 |     100 |   95.47 | 58-63,125-126,268,300,320,367,550,646-647,781,786                                                                                                                                                                                   
 src/validation/fixtures           |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  mocks.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  vectors.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
 src/wallet                        |   94.36 |    85.79 |   83.67 |   94.82 |                                                                                                                                                                                                                                     
  context.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  index.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  useWallet.js                     |   91.69 |    79.33 |   74.19 |   92.36 | 66,150,423-429,468,538,609,636,658-666,706,738,769,775-778,852-853,878                                                                                                                                                              
 src/wallet/fixtures               |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  mocks.js                         |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
  vectors.js                       |     100 |      100 |     100 |     100 |                                                                                                                                                                                                                                     
-----------------------------------|---------|----------|---------|---------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='3488']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='4383']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='1722']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='2293']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='728']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='1012']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='3445']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='4309']
##teamcity[blockClosed name='Code Coverage Summary']

Summary of all failing tests
FAIL src/components/Contacts/__tests__/index.test.js
  ● Test suite failed to run

    Your test suite must contain at least one test.

      at onResult (node_modules/@jest/core/build/TestScheduler.js:133:18)
      at node_modules/@jest/core/build/TestScheduler.js:254:19
      at node_modules/emittery/index.js:363:13
          at Array.map (<anonymous>)
      at Emittery.emit (node_modules/emittery/index.js:361:23)


Test Suites: 1 failed, 30 passed, 31 total
Tests:       728 passed, 728 total
Snapshots:   0 total
Time:        42.57 s
Ran all test suites.
Build cashtab-tests failed with exit code 1

Remove contacts from config, add tests

Failed tests logs:

====== CashTab Unit Tests: <App /> Adding a contact to to a new contactList by clicking on tx history adds it to localforage and wallet context ======
TestingLibraryElementError: Unable to find an element by: [data-testid="configure-ctn"]

Ignored nodes: comments, script, style
<body>
  <div>
    <div
      class="ant-spin-nested-loading css-dev-only-do-not-override-1rqnfsa"
    >
      <div
        class="ant-spin-container"
      >
        <div
          class="sc-dRaagA iASEmI"
        >
          <div
            class="Toastify"
          />
          <div
            class="sc-iiUIRa eQnmlE"
          >
            <div
              class="sc-iIHSe bkHjvE"
            >
              <div
                class="sc-dREXXX fRJRay"
              >
                <div
                  class="sc-gldTML hzCFsF"
                >
                  <img
                    alt="cashtab"
                    class="sc-feryYK liSsDu"
                    src="test-file-stub"
                  />
                </div>
                <div
                  class="sc-hSdWYo dgPahU"
                  data-testid="wallet-info-ctn"
                >
                  <div
                    class="sc-bfYoXt hZRQbJ"
                  >
                    <select
                      class="sc-gbOuXE iBsOHK"
                      id="wallets"
                      name="wallets"
                    >
                      <option
                        class="sc-dRFtgE bmOkor"
                        value="[Burned] useWallet Mock"
                      >
                        [Burned] useWallet Mock
                      </option>
                    </select>
                    <div
                      class="sc-gipzik iHnUKn"
                    >
                      <svg
                        class="sc-htoDjs ipNpSe"
                        style="margin-top: 8px;"
                      />
                    </div>
                    <div
                      class="sc-eilVRo kNDvpj"
                    >
                      <div
                        class="sc-eerKOB VCLyQ"
                      >
                        <input
                          checked=""
                          class="sc-bnXvFD clpqCe"
                          data-testid="show-hide-balance"
                          id="show-hide-balance"
                          name="show-hide-balance"
                          type="checkbox"
                        />
                        <label
                          class="sc-emmjRN bKoUpK"
                          for="show-hide-balance"
                        >
                          <span
                            class="sc-cpmLhU prmji"
                            data-off=""
                            data-on=""
                          />
                          <span
                            class="sc-dymIpo kCLFY"
                          />
                        </label>
                      </div>
                    </div>
                  </div>
                  <div
                    class="sc-gkFcWv QncPl"
                    data-testid="balance-xec"
                  >
                    10,000.00
                     
                    XEC
                     
                  </div>
                  <div
                    class="sc-hUfwpO dqkrok"
                    data-testid="balance-fiat"
                  >
                    $
                    0.30
                     
                    USD
                  </div>
                  <p
                    class="sc-imABML kpiOij"
                    data-testid="ecash-price"
                  >
                    1 
                    XEC
                     = 
                    0.00003000
                     
                    USD
                  </p>
                </div>
              </div>
              <div
                class="sc-hgRTRy fQdiGc"
              >
                <p
                  class="sc-cvbbAY iyZSgv"
                >
                  <b>
                    Error in chronik connection
                  </b>
                  <br />
                   If not corrected by refresh,
                   
                  <a
                    href="https://t.me/eCashDevelopment"
                    rel="noreferrer"
                    target="_blank"
                  >
                    notify admin
                  </a>
                </p>
                <div
                  class="sc-jKJlTe fkpOik"
                >
                  <span
                    aria-label="loading"
                    class="anticon anticon-loading anticon-spin"
                    data-testid="cash-loader"
                    role="img"
                  >
                    <svg
                      aria-hidden="true"
                      data-icon="loading"
                      fill="currentColor"
                      focusable="false"
                      height="1em"
                      viewBox="0 0 1024 1024"
                      width="1em"
                    ...
    at Object.getElementError (/work/cashtab/node_modules/@testing-library/dom/dist/config.js:37:19)
    at /work/cashtab/node_modules/@testing-library/dom/dist/query-helpers.js:76:38
    at /work/cashtab/node_modules/@testing-library/dom/dist/query-helpers.js:52:17
    at /work/cashtab/node_modules/@testing-library/dom/dist/query-helpers.js:95:19
    at Object.getByTestId (/work/cashtab/src/components/App/__tests__/App.test.js:325:23)
====== CashTab Unit Tests: <App /> Adding a contact to an existing contactList by clicking on tx history adds it to localforage and wallet context ======
TestingLibraryElementError: Unable to find an element by: [data-testid="configure-ctn"]

Ignored nodes: comments, script, style
<body>
  <div>
    <div
      class="ant-spin-nested-loading css-dev-only-do-not-override-1rqnfsa"
    >
      <div
        class="ant-spin-container"
      >
        <div
          class="sc-dRaagA iASEmI"
        >
          <div
            class="Toastify"
          />
          <div
            class="sc-iiUIRa eQnmlE"
          >
            <div
              class="sc-iIHSe bkHjvE"
            >
              <div
                class="sc-dREXXX fRJRay"
              >
                <div
                  class="sc-gldTML hzCFsF"
                >
                  <img
                    alt="cashtab"
                    class="sc-feryYK liSsDu"
                    src="test-file-stub"
                  />
                </div>
                <div
                  class="sc-hSdWYo dgPahU"
                  data-testid="wallet-info-ctn"
                >
                  <div
                    class="sc-bfYoXt hZRQbJ"
                  >
                    <select
                      class="sc-gbOuXE iBsOHK"
                      id="wallets"
                      name="wallets"
                    >
                      <option
                        class="sc-dRFtgE bmOkor"
                        value="[Burned] useWallet Mock"
                      >
                        [Burned] useWallet Mock
                      </option>
                    </select>
                    <div
                      class="sc-gipzik iHnUKn"
                    >
                      <svg
                        class="sc-htoDjs ipNpSe"
                        style="margin-top: 8px;"
                      />
                    </div>
                    <div
                      class="sc-eilVRo kNDvpj"
                    >
                      <div
                        class="sc-eerKOB VCLyQ"
                      >
                        <input
                          checked=""
                          class="sc-bnXvFD clpqCe"
                          data-testid="show-hide-balance"
                          id="show-hide-balance"
                          name="show-hide-balance"
                          type="checkbox"
                        />
                        <label
                          class="sc-emmjRN bKoUpK"
                          for="show-hide-balance"
                        >
                          <span
                            class="sc-cpmLhU prmji"
                            data-off=""
                            data-on=""
                          />
                          <span
                            class="sc-dymIpo kCLFY"
                          />
                        </label>
                      </div>
                    </div>
                  </div>
                  <div
                    class="sc-gkFcWv QncPl"
                    data-testid="balance-xec"
                  >
                    10,000.00
                     
                    XEC
                     
                  </div>
                  <div
                    class="sc-hUfwpO dqkrok"
                    data-testid="balance-fiat"
                  >
                    $
                    0.30
                     
                    USD
                  </div>
                  <p
                    class="sc-imABML kpiOij"
                    data-testid="ecash-price"
                  >
                    1 
                    XEC
                     = 
                    0.00003000
                     
                    USD
                  </p>
                </div>
              </div>
              <div
                class="sc-hgRTRy fQdiGc"
              >
                <p
                  class="sc-cvbbAY iyZSgv"
                >
                  <b>
                    Error in chronik connection
                  </b>
                  <br />
                   If not corrected by refresh,
                   
                  <a
                    href="https://t.me/eCashDevelopment"
                    rel="noreferrer"
                    target="_blank"
                  >
                    notify admin
                  </a>
                </p>
                <div
                  class="sc-jKJlTe fkpOik"
                >
                  <span
                    aria-label="loading"
                    class="anticon anticon-loading anticon-spin"
                    data-testid="cash-loader"
                    role="img"
                  >
                    <svg
                      aria-hidden="true"
                      data-icon="loading"
                      fill="currentColor"
                      focusable="false"
                      height="1em"
                      viewBox="0 0 1024 1024"
                      width="1em"
                    ...
    at Object.getElementError (/work/cashtab/node_modules/@testing-library/dom/dist/config.js:37:19)
    at /work/cashtab/node_modules/@testing-library/dom/dist/query-helpers.js:76:38
    at /work/cashtab/node_modules/@testing-library/dom/dist/query-helpers.js:52:17
    at /work/cashtab/node_modules/@testing-library/dom/dist/query-helpers.js:95:19
    at Object.getByTestId (/work/cashtab/src/components/App/__tests__/App.test.js:393:23)

Each failure log is accessible here:
CashTab Unit Tests: <App /> Adding a contact to to a new contactList by clicking on tx history adds it to localforage and wallet context
CashTab Unit Tests: <App /> Adding a contact to an existing contactList by clicking on tx history adds it to localforage and wallet context

update integration tests for new expected behavior

bytesofman published this revision for review.Apr 5 2024, 14:45
bytesofman added inline comments.
cashtab/src/components/Common/CustomIcons.js
91 ↗(On Diff #46878)

I think this was put here originally to solve a one-off alignment problem. now, it's preventing copy icon from properly vertical aligning with other divs

checking through the app, looks like this is an improvement everywhere, not just contact list.

cashtab/src/components/Home/Tx.js
1054 ↗(On Diff #46879)

quite the rube goldberg approach here before. we routed to configure and auto-named it after the address.

now that everything is better managed in state, we can just do it from here, and support user input for the name.

bytesofman added inline comments.
cashtab/src/components/Common/Modal.js
19 ↗(On Diff #46879)

see D15909 which isolates the modal improvements identified in creating this diff

D15909 should land before this diff, but imo this is also ready to review. The issues fixed in D15909 are also present in the current Configure-based contact list.

emack requested changes to this revision.EditedApr 6 2024, 07:25
emack added a subscriber: emack.

It allows duplicate contact names to be added (with different addresses). If they have a long contact list this might become hard to manage. It's the case in prod as well but if we want to check on unique contact names it might be fine to do it in this diff.

Another issue is if you don't input anything for name and address and click add, it successfully adds it as a blank entry in the contact list.

image.png (56×429 px, 3 KB)

Also consider the following workflow:

  1. Click on an inbound tx in txhistory
  2. Click on add to contacts
  3. Then delete this contact
  4. It's no longer possible to add it again via tx history shortcut

It should reappear right?

This revision now requires changes to proceed.Apr 6 2024, 07:25

It allows duplicate contact names to be added (with different addresses). If they have a long contact list this might become hard to manage. It's the case in prod as well but if we want to check on unique contact names it might be fine to do it in this diff.

patched

Another issue is if you don't input anything for name and address and click add, it successfully adds it as a blank entry in the contact list.

image.png (56×429 px, 3 KB)

patched

Also consider the following workflow:

  1. Click on an inbound tx in txhistory
  2. Click on add to contacts
  3. Then delete this contact
  4. It's no longer possible to add it again via tx history shortcut

It should reappear right?

I'm not able to repeat this -- if I delete a contact and go back to tx history, I now have the option to add it. Possibly there is the same address twice in your contact list from testing or some legacy bug? At any rate, should not be related to this diff, we do not change the logic of whether or not to show this option.

better validation functions for contact name and address

cashtab/src/validation/index.js
54

Note

I updated this function and its tests to better suit validation purpose, which is the only place it is used.

However, in testing, realized we cannot really support this unless we save the actual address in the contact list and not the alias.

If we save aliases in the contact list, then every cashtab feature that checks if an address exists in the contact list would also have to resolve every single alias.

So -- this is dead code now. I added a simpler function that just checks for a valid address and if it exists or not. But, will remove this in its own diff so it's easier to find if we want to support aliases in contacts again later.

I do not add a migration for contact list since only devs are expected to have aliases in their contact list at this point, and the failure mode is "tx history might not show that user is actually in contacts."

101

Now we only accept valid ecash: prefixed addresses in contacts

this simplifies using the info for app features.

Possibly there is the same address twice in your contact list from testing

That was it

This revision is now accepted and ready to land.Apr 6 2024, 22:02