Redirect navigation flow for 0 value invocies. Fixes #991 #1327
Conversation
emiljoha
commented
Jan 12, 2020
•
emiljoha
commented
- Functionality in desktop. (Tested on Linux)
- Functionality in mobile (Tested on android)
- Fix: Background on new view does not properly match views before and after.
- Make sure text content and placement is consitent with rest of the app.
- Fix: Input field on Desktop becomes 0500 when trying to write 500.
- Error handling. Make sure all error scenarios are considered and handled properly. (Note: My experience in UI coding is limited but I am currently convinced this new feature will handle errors gracefully)
emiljoha
force-pushed
the
support-zero-amount-invocies
branch
from
a653219 to
09d5baa
Compare
emiljoha
force-pushed
the
support-zero-amount-invocies
branch
3 times, most recently
from
4b65fc9 to
07978ca
Compare
src/action/payment.js
Outdated
| this._nav.goPayLightningConfirm(); | ||
| if ( | ||
| this._store.payment.amount === '0' || | ||
| this._store.payment.amount === 0 |
There was a problem hiding this comment.
is there a need for checking for both 0 and '0'? I feel like it should always just be a value of one type. If that's not the case then it should be cast to one type first before checking.
There was a problem hiding this comment.
Not really, I should ™️ be '0' its just me being defensive. Will fix.
jsconfig.json
Outdated
| @@ -0,0 +1,10 @@ | |||
| { | |||
| "compilerOptions": { | |||
There was a problem hiding this comment.
I'm not sure this file really needs to be in this PR. It's useful but has nothing to do with this particular feature. I think this should be added in a follow-up PR.
There was a problem hiding this comment.
Not sure why this is there or what it does... Probably got created when I was trying to set up the project/IDE. Will remove from this PR. Not really sure what it does but I can add it in a follow up PR if It would be useful.
There was a problem hiding this comment.
@emiljoha it's used by VSCode to configure the built-in JS language service. See here for details: https://code.visualstudio.com/docs/languages/jsconfig
There was a problem hiding this comment.
Ah! That makes a lot of sense. Thanks!
src/action/payment.js
Outdated
| * @return {Promise<undefined>} | ||
| */ | ||
| async reEstimateLightningFee() { | ||
| try { |
There was a problem hiding this comment.
I wonder if the error from this try/catch block should be propagated further from this method so that the UI can display some useful info to the use and try to recover rather than just having a log message.
There was a problem hiding this comment.
Thinking about it, that exception will only be thrown if we do not have a valid lightning invoice in the address field. That is dependent on other functions being called before this one. That kind of subtle and implicit dependencies are dangerous, IMHO. I have tried to make the expectations of the method more clear both in code and documentation.
PS: I am used to writing functional Typescript code in my day job, so all this state and no typing is making me a bit uncomfortable.
src/action/payment.js
Outdated
| @@ -321,7 +363,8 @@ class PaymentAction { | |||
| } | |||
|
|
|||
| /** | |||
| * Send the amount specified in the invoice as a lightning transaction and | |||
| * Send the amount specified in the payment.amount. If zero use the amount | |||
There was a problem hiding this comment.
Wonky description here: If zero use the amount specified in the invoice as a lightning transaction and display the wait screen while the payment confirms. doesn't really make sense.
There was a problem hiding this comment.
Will try to make it more clear.
Detecting a zero amount invoice in checkType we redirect to a seperate view. Currently only title is changed in the view.
Amount will be an empty string when user have not specified in the view before pressing "pay".
emiljoha
force-pushed
the
support-zero-amount-invocies
branch
from
49dabdb to
51413f2
Compare
|
@bolatovumar Thank you for you comments! Have tried to address the issues. |
emiljoha
changed the title
emiljoha
changed the title
|
@Roasbeef (Pinging you as you where the originator of the issue and the only one that have merged commits into to this repository for the last couple of months.) The change is IMHO ready and has been reviewed by @bolatovumar. Please advice if there are any further steps needed to be taken in order to move forward in solving this issue. |
|
I have reviewed the PR and it looks good to me. I have not tested it, however. |
