middleware
middleware copied to clipboard
middlewarehq
Added Settings Page with DefaultSyncDays settings
Linked Issue(s)
#500
Acceptance Criteria fulfillment
- [x] Reducers for Getting and Updating DefaultSyncDaysSetting
- [x] NextJs common setting API for Getting and Updating Org settings.
- [x] NextJs API for Resetting Bookmark
- [x] Added Required types
- [x] Settings Page and SyncDaysSetting component
@samad-yar-khan @jayantbh Is this approach ok? The settings page still need changes to easily add further settings. We can discuss about it.
@Kamlesh72 Can you also add a screenshot for the UI changes that goes with this PR. Thanks :)
https://github.com/user-attachments/assets/05a15de4-cf9f-4ccd-bc36-714e8e851f07
I think the shifting input field feels unpleasant to the eye. Everything else is fine though.
A simple change would be to just add a save button to the bottom that shows up on value changes.
Also, did you handle the case where the user should not be able to set a value smaller than the current sync days value?
@jayantbh I will add code to handle the less than currentSyncDays input case. What should be maximum sync days? 365,366?
I am thinking of some way to handle all settings collectively. Will share the approach soon.
I was thinking to not set a maximum... but, let's start by setting it to 366 days. @Kamlesh72
@samad-yar-khan do I need to call syncReposForOrg() after updating Sync Days?
@samad-yar-khan do I need to call syncReposForOrg() after updating Sync Days?
@Kamlesh72 yes you should. You can also ask the customer if they want to backfill data as per current changes or let the system handle in next sync cycle.
https://github.com/middlewarehq/middleware/pull/516#issuecomment-2288883466
@samad-yar-khan In future we might have more settings and they gets updated together. So asking customer for change might not be a good choice. I think it will be better to just call the sync in background. Can this be considered as Forced Sync?
@Kamlesh72 I agree with @samad-yar-khan. Either the data fills itself in the next iteration, or the customer force syncs. calling it quietly in the background might not be very intuitive for the user. We could give this control to the user. If we were so start the sync as soon as the user sets the setting, it would be better to show some message to the user that the sync has started or something along those lines. @jayantbh What are your thoughts on this?
@Kamlesh72 how would we handle multiple setting updations leading to multiple sync calls. Do you think this might overload the sync server?
@samad-yar-khan On every /sync request we are aquiring a lock on redis, so multiple sync calls need to wait till that. So I don't think this will overload the sync server.
If you are asking how we will manage multiple settings, then API call will only be made for settings which are updated by user. Once this PR gets merged, my Next PR will be to handle setting changes collectively. https://github.com/middlewarehq/middleware/pull/516#pullrequestreview-2232670225
@Kamlesh72 were the visual changes that I requested, made? Asking because a bunch of other discussion has happened, and I wonder if I missed it. :) No pressure, just ensuring that I didn't miss anything.
@jayantbh
https://github.com/user-attachments/assets/2a71e659-ec1d-4e1f-ba38-f9b85cab152a
Just one tiny change. Make the confirmation action (Save) be on the right, and the rejection action (Discard) be on the left.
That's generally how accept/reject CTAs work everywhere. :)
Good going on this though! 👍🏽
I've approved it, but I'd like @samad-yar-khan or @adnanhashmi09 to also cross check.
The initial PR and the PR in its current state is significantly different. Great growth @Kamlesh72 👏🏽 This must have been something to persevere through, but great job sticking to it.
Thanks a lot everyone! Your feedback and guidance have made all the difference. The learnings I got is invaluable.
LGTM @Kamlesh72 , good stuff! Merging this 🎉 cc: @samad-yar-khan @adnanhashmi09 @jayantbh
