lightning-browser-extension icon indicating copy to clipboard operation
lightning-browser-extension copied to clipboard

fix: remove Remember & set budget from send #1510

Open vaibhavgarg237 opened this issue 2 years ago • 9 comments

Describe the changes you have made in this PR

Remove "Remember and set a budget" option from send screen when opting for lightning invoice

Link this PR to an issue

Fixes #1510

Type of change (Remove other not matching type)

  • fix: Bug fix (non-breaking change which fixes an issue)

Screenshots of the changes (If any)

  • Previous behaviour image

  • After removal of "Remember and set a budget" image image

How has this been tested?

Manual Testing

  • I used ln invoice from Phoenix and tapped Send on popup and website, which shows "Remember and set a budget" but with the changes it has been removed
  • Lightning invoice used: lnbc1m1p3ngewjpp578s6uh2gvmsuldc7khppc57spggzjuy9aj0cg4633put5x20p6cqdqqxqyjw5q9q7sqqqqqqqqqqqqqqqqqqqqqqqqq9qsqsp5e5a6h39y85fnvlenly6dmfx97y32tru4hkn84x0wcgjf8egyfk4qrzjqwryaup9lh50kkranzgcdnn2fgvx390wgj5jd07rwr3vxeje0glclll0mxh6jgh2xuqqqqlgqqqqqeqqjqfs7gjqntggjnpgxp9rjh78gx2cczjuha65wdz9w2462chm8a7d78s2gwpdndsra4lf7940dh03dw67xcr8dd605lm68j6pusjj7n9tspm0mtde

Checklist

  • [x] My code follows the style guidelines of this project and performed a self-review of my own code
  • [x] New and existing tests pass locally with my changes
  • [x] I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

vaibhavgarg237 avatar Sep 28 '22 16:09 vaibhavgarg237

Does it still maintain the Remember & set budget for the other cases, right?

bitcoinuser avatar Sep 28 '22 16:09 bitcoinuser

Does it still maintain the Remember & set budget for the other cases, right?

I noticed it removed the set budget for other cases like subscribing to some project on geyser.fund. I am rechecking the codebase and cases where it should and should not show the set budget option.

Edit: Figured out above problem, working on tests...

vaibhavgarg237 avatar Sep 28 '22 17:09 vaibhavgarg237

The issue should have more specific, sorry.
The important part here is, that we do not just want to remove this.

For example in the prompt case we will always need it:

  1. Go to https://nakaphoto.vercel.app/accounts/[email protected]
  2. Click on a picture
  3. Prompt shows up, click "connect"
  4. Here we need this functionality:
    image

Try if this works:

  • This block needs to be hidden if the extension-popup is being used but shown if the prompt is being used: https://github.com/getAlby/lightning-browser-extension/blob/master/src/app/screens/ConfirmPayment/index.tsx/#L135
  • Try moving the !navState.origin out and put this with an if around the block. Because only using the prompt we have an origin
    https://github.com/getAlby/lightning-browser-extension/blob/master/src/app/screens/ConfirmPayment/index.tsx/#L104

Does this help? Let me know if you have any questions.

escapedcat avatar Sep 29 '22 03:09 escapedcat

For example in the prompt case we will always need it:

Yeah, I got that one. I was still figuring out for it's tests ... That's why I did some rough changes in a separate branch to hide the option when someone is using send, and show when in case as described above https://github.com/vaibhavgarg237/lightning-browser-extension/commit/e685ce8af043d37840b811bb15b9c60e07410380

vaibhavgarg237 avatar Sep 29 '22 05:09 vaibhavgarg237

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.

Don't forget: keep stacking sats!

github-actions[bot] avatar Sep 30 '22 02:09 github-actions[bot]

@vaibhavgarg237 tests are green here 🚀
Might be good to add a test for your change. I can help with that.

escapedcat avatar Sep 30 '22 03:09 escapedcat

@vaibhavgarg237 tests are green here 🚀 Might be good to add a test for your change. I can help with that.

I think tests are passing because it's only checking for demo page as it is creating a OriginData in starting itself. I think we need to add another test that only runs when in condition when the origin is not present and the current one should only run when in presence of origin.

vaibhavgarg237 avatar Oct 01 '22 13:10 vaibhavgarg237

I was facing some issues related to running original tests locally. I have dm'ed you on slack regarding the same, kindly lemme know whenever you are available. Thanks

vaibhavgarg237 avatar Oct 01 '22 13:10 vaibhavgarg237

ack

escapedcat avatar Oct 07 '22 04:10 escapedcat