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

feat: include account ID in permissions #2002

Merged
merged 17 commits into from
Mar 6, 2023
Merged

feat: include account ID in permissions #2002

merged 17 commits into from
Mar 6, 2023

Conversation

im-adithya
Copy link
Member

@im-adithya im-adithya commented Jan 21, 2023

Describe the changes you have made in this PR

Adds accountId to the DbPermission fields to link permissions to accounts. (Doesn't include a migration; we handle the permissions without an accountId separately everywhere).

Link this PR to an issue [optional]

Fixes #1953

Type of change

(Remove other not matching type)

  • feat: New feature (non-breaking change which adds functionality)

Screenshots of the changes [optional]

Screenshot 2023-01-21 at 6 41 38 PM

How has this been tested?

Tested manually at various websites.

Checklist

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

@github-actions
Copy link

github-actions bot commented Jan 21, 2023

🚀 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.


This build is brought to you by: channel.ninja (who recently dropped 1000 sats):


Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep earning sats!

Copy link
Contributor

@reneaaron reneaaron left a comment

Choose a reason for hiding this comment

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

Successfully tested 👌 the migration also worked as expected! 👍

@reneaaron
Copy link
Contributor

reneaaron commented Feb 1, 2023

Just did a final round of testing and noticed one thing that I think we should fix. Steps to reproduce:

I have 2 accounts (account A, account B)

  • Account A: Go to a site and give permissions to read public key
  • Go to the websites, select the website from step 1 and click on edit. You will see the permissions correctly:
    image
  • Close the permission popup (but stay on this site), switch to account B
  • Edit the permissions with account B

The edit dialogue now still shows the permissions from account A. (reloading fixes the problem)

@im-adithya Can we somehow fix that?

That being said, I think we really need to move the permissions somewhere else in another iteration, they don't fit into this "edit allowance" window very well. Also the allowance is now extension wide, while the permissions are not. (which I think is quite hard to understand)

Copy link
Contributor

@reneaaron reneaaron left a comment

Choose a reason for hiding this comment

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

Tested again today, left some feedback and merged master.

Let's see if we can somehow fix the bug when switching accounts and get this merged! 🚀

@lujakob
Copy link
Contributor

lujakob commented Feb 2, 2023

Tested again today, left some feedback and merged master.

Let's see if we can somehow fix the bug when switching accounts and get this merged! 🚀

I tested a bit. The bug is in AllowanceMenu line 77 !permissions && fetchPermissions(); - the !permissions condition needs to be removed and permissions from the deps. But better double check the re-rendering performance.

https://github.com/getAlby/lightning-browser-extension/pull/2002/files#diff-1b329da99ce24a934532ca1ee401d135a5d5917619b62db9711da859ce47f987R77

Copy link
Contributor

@reneaaron reneaaron left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @im-adithya! 🚀

tACK

@bumi bumi merged commit cc64a57 into master Mar 6, 2023
@bumi bumi deleted the task-account-perms branch March 6, 2023 22:12
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.

Store account ID to permissions
5 participants