Skip to content
This repository has been archived by the owner on Sep 5, 2023. It is now read-only.

Fix notifications settings that were not remembered #75

Merged
merged 6 commits into from
Nov 2, 2021

Conversation

petitcl
Copy link
Contributor

@petitcl petitcl commented Oct 16, 2021

Fixes #72

  • Rename SyncToggles component to NotificationsSettings to better reflect its purpose
  • Save notifications settings in the config object. I didn't put it in the local storage because the settings need to be accessed from the renderer and from the main process. Because of this use, it didn't make sense to me to use the local storage.
  • Fix a bug where you would get a lot of old notifications when mixing between auto sync and full sync. I personally had this bug many times because I play with the buttons a lot. To reproduce on the main branch: start auto sync, wait for a tick, then trigger a full sync, and wait for another auto sync tick. That last tick will notify each deal closed since the beginning of the history (it should notify only if there were new deals closed since the full sync). I fixed it by not resetting the time to 0 after a full sync.

@coltoneshaw
Copy link
Owner

This is fantastic, thanks for all the hard work! Love the idea of storing this in the config file, makes a lot of sense honestly.

My only thought here might be to pull the sync settings from the redux store and pass into the sync settings. Since we already have the data stored within redux.

@coltoneshaw
Copy link
Owner

Only other thing here is to write a config migration script to add this to the users config.json file. I'm more than happy to do that if you want since electron-store can be weird to work with.

@petitcl
Copy link
Contributor Author

petitcl commented Oct 16, 2021

For the migration script, it's good for me if you take care of it. I'll take a look at how you do it so that I can get and idea of how it works.

For the sync settings point, I'm not really following, sorry. I thought we were already pulling the sync settings from the redux store. Could you give a bit more details ?

@coltoneshaw
Copy link
Owner

Just added some comments, let me know if it makes sense!

@petitcl
Copy link
Contributor Author

petitcl commented Oct 17, 2021

I cannot see your comments x) Maybe you need to submit them or something

@coltoneshaw
Copy link
Owner

@petitcl - I put them actually on the code, below is a screenshot of them. Basically, i'm wondering if you think it would be worth it to pass the notification settings down as params to the update function instead of reading the config in the update function.

Screen Shot 2021-10-17 at 12 22 28 PM

src/main/3Commas/index.ts Outdated Show resolved Hide resolved
src/app/redux/threeCommas/Actions.ts Show resolved Hide resolved
@coltoneshaw
Copy link
Owner

@petitcl - I did another review on this, sorry my prior comments seemed confusing. I pushed a few updates and actually still had the original issue. It seems when switching profiles we never reset the syncCount. Adjusted this, and added the config migration scripts we talked about.

Let me know if you have any feedback. I'll probably merge this here shortly.

@coltoneshaw coltoneshaw merged commit 65f8ea6 into coltoneshaw:v1.0.1 Nov 2, 2021
@coltoneshaw
Copy link
Owner

@petitcl - I went ahead and merged this since I had to make some changes in the release branch that relied on this. Let me know if you noticed any issues and I'll open another PR to fix them. Thanks for this hard work!!

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

Successfully merging this pull request may close these issues.

Notification are sent despite box being unchecked
2 participants