-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Queue new calendars for immediate import #2379
Conversation
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.
This looks almost okay but I think I'd be happier if this logic was in the model as an after_save
hook. It can then be tested that saving a new calendar puts it into the queue
I am not sure if that the queue_for_import!
method returns a truthy value if it succeeds (or a falsey value if it fails).
Also if the calendar doesn't successfully get queued (which ideally should never happen) then is showing a message in the flash the most helpful feedback?
ok, have made those changes as requested - decided automatically queuing for import after any editing is always good - no one wants to be waiting around for their changes to be picked up after they've just made changes to something. |
ok I've made use of the |
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.
Looks good to me
Fixes #2304
Description
After a bit of digging it seemed like the easiest solution here was to improve the notification shown in the header and to also immediately import a calendar's events on creation. That means that the user doesn't get to specify the date to import from on their first import (it will just default to now) but they can change that at any point after the import, and changing it to a date in the future will remove any events that have already been imported if they didn't want current events showing for any reason. We could add a field in the form to let them control when the import begins from on their first import, but that just felt like another thing for users to have to think about at the create stage when most of them probably don't really care about this, so I think this would be better as a new feature if it comes up again.