Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render error tx844 #898

Open
wants to merge 12 commits into
base: staging
Choose a base branch
from
Open

Conversation

sohammk08
Copy link
Contributor

High Level Overview of Change

Made changes in translations.json to exclude connection.server.publicKey from server_ledgers_hint

Context of Change

This change was made because the connection.server.publicKey in server_ledgers_hint in translations.json wasn't able to function properly. Upon closer inspection, it was found that the reason for this occurrence was the publicKey field being empty. Removed the connection.server.publicKey part and restructured sentences accordingly.

Type of Change

  • Translation Updates

TypeScript/Hooks Update

  • Updated files to React Hooks
  • Updated files to TypeScript

Before / After

Before:

"server_ledgers_hint": "This node {{connection.server.publicKey}} only contains ledgers {{connection.ledger.validated}}",

After:

"server_ledgers_hint": "This node only contains ledgers {{connection.ledger.validated}}",

Test Plan

Changed server_ledgers_hint in translations.json in a manner such that only the 'connection.ledger.validated' is used as there was some error with displaying 'connection.server.publicKey'
@@ -200,7 +200,7 @@
"invalid_transaction_hash": "The transaction hash is invalid",
"ledger_not_found": "Ledger not Found",
"check_ledger_id": "Please check your ledger id",
"server_ledgers_hint": "This node ({{connection.server.publicKey, truncate(length: 10)}}) only contains ledgers {{connection.ledger.validated}}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still want the pubkey to be printed if it's available, with some sort of "This Clio node only..." alternate message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, you're saying that the clio-node-part still needs to be present in the code. What if I change the code such that whenever the pubKey is not empty, it gets displayed. But when it is empty, print it as "This clio node only..."

Would that be viable?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, something like that sounds good.

@@ -196,7 +196,7 @@
"invalid_transaction_hash": "El hash de la transacción es inválido",
"ledger_not_found": "Libro Contable no Encontrado",
"check_ledger_id": "Por favor, comprueba el id de tu libro contable",
"server_ledgers_hint": "Este nodo ({{connection.server.publicKey, truncate(length: 10)}}) solo contiene libros contables {{connection.ledger.validated}}",
"server_ledgers_hint": "Este nodo solo contiene libros contables {{connection.ledger.validated}}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, just a note, if you are unsure about the translations in other languages, you can set them to null and others can fill in later. It would use the default en-US if set to null

@sohammk08
Copy link
Contributor Author

@ckniffen @pdp2121 @mvadari
I finally have an update. I managed to get the publicKey part working.

However, I'm still not sure about the part where we display server_ledgers_hint if publicKey does not exist and server_ledgers_hint_with_key if it does.

Here is where I currently am, for reference:

Capture

@pdp2121
Copy link
Collaborator

pdp2121 commented Apr 2, 2024

@ckniffen @pdp2121 @mvadari I finally have an update. I managed to get the publicKey part working.

However, I'm still not sure about the part where we display server_ledgers_hint if publicKey does not exist and server_ledgers_hint_with_key if it does.

Here is where I currently am, for reference:

Capture

@sohammk08 You could use ternary operator to condition on whether the publicKey exists and display different messages based on that.

@sohammk08
Copy link
Contributor Author

sohammk08 commented Apr 6, 2024

@pdp2121 @mvadari @ckniffen I have solved the error and now, it works properly. Just to make sure, I have also tested to see if it works, and it does. I am giving below the method I used to test. I will be making a commit containing the changes soon after writing this comment. Below is how I tested it to make sure if it works or not. Take a look:

Original file:

  const notFound = title.includes('not_found')
  const hintMsg = hints.map((hint) => (
    <div className="hint" key={hint}>
      {t(hint as any, values)}
    </div>
  ))
  const derivedWarning = warning ?? (notFound && t('not_found'))

  return (
    <div className="no-match">
      <Helmet title={t(title as any)} />
      {isError && <div className="uh-oh">{t('uh_oh')}</div>}
      <div className="title">{t(title as any, values)}</div>
      {hintMsg}
      {(derivedWarning || isError) && (
        <div className="warning">
          <InfoIcon title={derivedWarning} />
          &nbsp;
          <span>{derivedWarning}</span>
        </div>
      )}
    </div>
  )
}

export default NoMatch

Changed file:

  const notFound = title.includes('not_found')

  // Determine which hint message to display based on the presence of publicKey
  const hintMsg = values.connection?.server?.publicKey
    ? t('server_ledgers_hint_with_key', values)
    : t('server_ledgers_hint', values)

  const derivedWarning = warning ?? (notFound && t('not_found'))

  return (
    <div className="no-match">
      <Helmet title={t(title as any)} />
      {isError && <div className="uh-oh">{t('uh_oh')}</div>}
      <div className="title">{t(title as any, values)}</div>
      {hintMsg && <div className="hint">{hintMsg}</div>}
      {(derivedWarning || isError) && (
        <div className="warning">
          <InfoIcon title={derivedWarning} />
          &nbsp;
          <span>{derivedWarning}</span>
        </div>
      )}
    </div>
  )
}

export default NoMatch

I also made changes to translations.json as such:

  "server_ledgers_hint": "This node only contains ledgers {{connection.ledger.validated}}",
  "server_ledgers_hint_with_key": "This node ({{connection.server.publicKey, truncate(length: 10)}}) only contains ledgers {{connection.ledger.validated}}",

Also, here is how I tested whether the change works or not;
First, I set the publicKey to null for debugging purposes. Then, I changed the hintMsg line. Here is how it looks in code:

  const notFound = title.includes('not_found')
  const publicKey = null

  // Determine which hint message to display based on the presence of publicKey
  const hintMsg = publicKey
    ? t('server_ledgers_hint_with_key', values)
    : t('server_ledgers_hint', values)

  const derivedWarning = warning ?? (notFound && t('not_found'))

I checked the logic after adding the above debugging code as said before. Please let me know if I should try and implement a test in NoMatch.test.tsx for the same. Also, as @pdp2121 suggested, I set the server_ledgers_hint_with_key for other language translations as null

Have a good day :)

@pdp2121
Copy link
Collaborator

pdp2121 commented Apr 16, 2024

@pdp2121 @mvadari @ckniffen I have solved the error and now, it works properly. Just to make sure, I have also tested to see if it works, and it does. I am giving below the method I used to test. I will be making a commit containing the changes soon after writing this comment. Below is how I tested it to make sure if it works or not. Take a look:

Original file:

  const notFound = title.includes('not_found')
  const hintMsg = hints.map((hint) => (
    <div className="hint" key={hint}>
      {t(hint as any, values)}
    </div>
  ))
  const derivedWarning = warning ?? (notFound && t('not_found'))

  return (
    <div className="no-match">
      <Helmet title={t(title as any)} />
      {isError && <div className="uh-oh">{t('uh_oh')}</div>}
      <div className="title">{t(title as any, values)}</div>
      {hintMsg}
      {(derivedWarning || isError) && (
        <div className="warning">
          <InfoIcon title={derivedWarning} />
          &nbsp;
          <span>{derivedWarning}</span>
        </div>
      )}
    </div>
  )
}

export default NoMatch

Changed file:

  const notFound = title.includes('not_found')

  // Determine which hint message to display based on the presence of publicKey
  const hintMsg = values.connection?.server?.publicKey
    ? t('server_ledgers_hint_with_key', values)
    : t('server_ledgers_hint', values)

  const derivedWarning = warning ?? (notFound && t('not_found'))

  return (
    <div className="no-match">
      <Helmet title={t(title as any)} />
      {isError && <div className="uh-oh">{t('uh_oh')}</div>}
      <div className="title">{t(title as any, values)}</div>
      {hintMsg && <div className="hint">{hintMsg}</div>}
      {(derivedWarning || isError) && (
        <div className="warning">
          <InfoIcon title={derivedWarning} />
          &nbsp;
          <span>{derivedWarning}</span>
        </div>
      )}
    </div>
  )
}

export default NoMatch

I also made changes to translations.json as such:

  "server_ledgers_hint": "This node only contains ledgers {{connection.ledger.validated}}",
  "server_ledgers_hint_with_key": "This node ({{connection.server.publicKey, truncate(length: 10)}}) only contains ledgers {{connection.ledger.validated}}",

Also, here is how I tested whether the change works or not; First, I set the publicKey to null for debugging purposes. Then, I changed the hintMsg line. Here is how it looks in code:

  const notFound = title.includes('not_found')
  const publicKey = null

  // Determine which hint message to display based on the presence of publicKey
  const hintMsg = publicKey
    ? t('server_ledgers_hint_with_key', values)
    : t('server_ledgers_hint', values)

  const derivedWarning = warning ?? (notFound && t('not_found'))

I checked the logic after adding the above debugging code as said before. Please let me know if I should try and implement a test in NoMatch.test.tsx for the same. Also, as @pdp2121 suggested, I set the server_ledgers_hint_with_key for other language translations as null

Have a good day :)

Hi @sohammk08, it would be nice if you could add a test inside NoMatch.test.tsx. Thanks for your contribution!

const derivedWarning = warning ?? (notFound && t('not_found'))

return (
<div className="no-match">
<Helmet title={t(title as any)} />
{isError && <div className="uh-oh">{t('uh_oh')}</div>}
<div className="title">{t(title as any, values)}</div>
{hintMsg}
{hintMsg && <div className="hint">{hintMsg}</div>}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we need to change here. Would hintMsg ever be null? Also, are we not using hints array here at all?

Copy link
Contributor Author

@sohammk08 sohammk08 Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will implement the suggested change in the next commit

@pdp2121
Copy link
Collaborator

pdp2121 commented May 2, 2024

Tests are failing since we are using node 18 instead.

@sohammk08
Copy link
Contributor Author

Tests are failing since we are using node 18 instead.

Yes. I checked the staging branch of the ripple/staging and all tests are passing. I ran npm run test and almost 11 tests are failing. Hopefully, I will be committing the revised code such that it doesn't fail any tests and.
Thanks for your input.

Good Day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants