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
[WIP] Bugfix: add reset SimpleFIN credential button to escape SimpleFIN broken/unable to sync state #2739
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
e0fad66
to
0d33a45
Compare
Thanks for adding this, I know lots of people have requested it! It feels quite out of place where you put it though. To me, it would make more sense in the Advanced Settings screen with the other reset/repair buttons. |
I actually slept on it and had this same thought, let me move it and I'll update the PR. |
0f523c1
to
89ca7bc
Compare
whoops, linter fail |
4c8a641
to
998b4af
Compare
@psybers ready for another look whenever :) |
<strong>Reset SimpleFIN Credential</strong> In some cases (like | ||
importing from an existing database to a new instance), you may need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of a nit, but the other settings incorporate the bolded text into the first sentence. Here it is acting more like a heading. Can you reword the text? Something like:
<strong>Reset SimpleFIN credential</strong> if you are having trouble accessing SimpleFIN. ...
I was not able to actually test the code, as I did not want to deal with resetting my token and do not have time to set up a second/test server. |
…in which SimpleFIN is unable to sync
03b53e3
to
b8e02d9
Compare
@psybers actually hold off on this one, I'm going to run some tests of my own and maybe try to write automated tests as well. might not be able to be done done for another day or two |
@psybers the more I sit with this, the more I'm thinking the right way to implement this is to actually have this button hit the server and have the server clear the rows instead of just trying to overwrite by forcing setup again. What do you think? |
@mattpetters I'd suggest throwing a "[WIP]" tag on the PR title while you decide the best course. |
Since you are rethinking it, maybe look at how the GoCardless sync handles the reset. I believe it has similar functionality: https://discord.com/channels/937901803608096828/940290142579605514/1232681744780759041 |
Has to be earlier than that IMO. If you currently mis-configure the keys - you won't be able to access the "link accounts" modal. Meaning you wouldn't be able to change the keys. |
Would it make sense to move the GoCardless one, so then both would be on the initial 'create account' dialog? Can put two menus there, and maybe only show them if credentials are already configured? |
I think my initial impl was something similar to that with the reset button below the regular button. Initially I just did that for convenience (everything I wanted to call was right there) but maybe it could have just been refactored to a) be less obtrusive similar to the 3 dot menu and b) only show conditionally as you're suggesting. PS I should have time to work on this a bit more at some point this weekend so if you guys come upon a design I'll get on it then. And of course feel free to take the reins if you want ;) |
Maybe something like this? @MatissJanis what do you think? |
I think that looks good (just need to vertically align the "..." menu with the button)! |
@mattpetters Feel free to use this as a starting point: |
category: Bugfix
authors: [mattpetters]
Add reset SimpleFIN credential button to resolve a state in which SimpleFIN is unable to sync