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.

Sign up for GitHub

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

Jump to bottom

Allow user to set default reminder duration for new events #3001

Merged
merged 3 commits into from
May 7, 2021
Merged

Allow user to set default reminder duration for new events #3001

merged 3 commits into from
May 7, 2021

Conversation

tchernobog
Copy link
Contributor

@tchernobog tchernobog commented Mar 31, 2021 •
edited
Loading

This PR implements a new setting in the calendar web UI to allow the user to specify a default reminder to apply to newly created events.

This is particularly helpful when you want to create a batch of events in a row.

It should provide a workable solution to issue #629, albeit it is a global setting rather than per-calendar.

@tchernobog
Add label to slotDuration setting
222a025 
Add a label to slotDuration so that the user is not left
wondering what the setting refers to. I remember being
puzzled myself the first time I saw it.

Signed-off-by: Matteo Settenvini <matteo@member.fsf.org>
@tchernobog tchernobog force-pushed the feature/default-reminder-setting branch 2 times, most recently from d680b11 to 09b74a8 Compare March 31, 2021 21:17
@codecov
Copy link

codecov bot commented Mar 31, 2021 •
edited
Loading

Codecov Report

Merging #3001 (9e6b1c7) into master (69952ec) will increase coverage by 0.24%.
The diff coverage is 32.14%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3001      +/-   ##
============================================
+ Coverage     29.18%   29.42%   +0.24%     
- Complexity      116      121       +5     
============================================
  Files           155      155              
  Lines          5561     5607      +46     
  Branches        822      825       +3     
============================================
+ Hits           1623     1650      +27     
- Misses         3938     3957      +19
Flag Coverage Δ Complexity Δ
javascript 23.88% <32.14%> (+0.03%) 0.00 <0.00> (ø)
php 94.54% <ø> (+0.24%) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
src/components/AppNavigation/Settings.vue 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/store/calendarObjectInstance.js 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/views/Calendar.vue 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/store/settings.js 100.00% <100.00%> (ø) 0.00 <0.00> (ø)
...ud/apps/calendar/lib/Controller/ViewController.php 100.00% <0.00%> (ø) 7.00% <0.00%> (ø%)
...s/calendar/lib/Controller/PublicViewController.php 100.00% <0.00%> (ø) 6.00% <0.00%> (ø%)
...pps/calendar/lib/Controller/SettingsController.php 91.26% <0.00%> (+1.09%) 42.00% <0.00%> (+5.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69952ec...9e6b1c7. Read the comment docs.

@tchernobog tchernobog force-pushed the feature/default-reminder-setting branch 4 times, most recently from a145066 to 3c015df Compare March 31, 2021 22:04
@tchernobog
Copy link
Contributor Author

Hi @ChristophWurst, do you know who could perform a review? Thanks!

tcitworld
tcitworld reviewed Apr 8, 2021
View reviewed changes
Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

I'm conflicted about this. It's definitely not enough to close #629 since it's only a setting to add a simple timed based DISPLAY reminder and it doesn't consider events to be allDay to suggest appropriate slots. Also it crowds even more the bottom left settings area since we don't have #2304 yet.

Still, might be handy for some people in the mean time, so 🤷

lib/Controller/SettingsController.php Outdated Show resolved Hide resolved
@tchernobog
Copy link
Contributor Author

I'm conflicted about this. It's definitely not enough to close #629 since it's only a setting to add a simple timed based DISPLAY reminder and it doesn't consider events to be allDay to suggest appropriate slots. Also it crowds even more the bottom left settings area since we don't have #2304 yet.

Still, might be handy for some people in the mean time, so shrug

I know, and maybe I should reword the commit to avoid saying it fixes #629. But it was enough to cover my personal needs of having a default reminder set when creating events through the Web UI, which is the specific improvement I sought. I am not going after the bug bounty anyway.

@tchernobog
Add defaultReminder setting
bea17a7 
Add a setting to the app state that can be reused
when creating new events to set a default alarm / reminder.

Signed-off-by: Matteo Settenvini <matteo@member.fsf.org>
@tchernobog
Add user default reminder to new events
9e6b1c7 
Take the value from the saved settings for the default reminder,
and if it is a valid amount of seconds, add a new alarm
to newly created events.

Signed-off-by: Matteo Settenvini <matteo@member.fsf.org>
@tchernobog tchernobog force-pushed the feature/default-reminder-setting branch from 3c015df to 9e6b1c7 Compare April 8, 2021 10:43
@tchernobog
Copy link
Contributor Author

Hi @ChristophWurst, are there more changes you'd like me to perform? Thanks!

@tchernobog
Copy link
Contributor Author

@ChristophWurst, @tcitworld: any further feedback you'd like me to address? Thanks!

@tchernobog
Copy link
Contributor Author

Weekly ping :-)

@tcitworld tcitworld added 3. to review Waiting for reviews javascript Pull requests that update Javascript code labels Apr 26, 2021
@tchernobog
Copy link
Contributor Author

Hi @ChristophWurst, any major blocker to see this merged? I wanted to contributed a few more enhancements to the calendar app, but I would first like to see how it goes with this PR.

ChristophWurst
ChristophWurst approved these changes May 7, 2021
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code looks good!

@ChristophWurst ChristophWurst merged commit ead3fac into nextcloud:master May 7, 2021
@szaimen
Copy link
Contributor

szaimen commented May 7, 2021

🚀

@tcitworld tcitworld mentioned this pull request Jan 24, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcitworld tcitworld tcitworld left review comments

@ChristophWurst ChristophWurst ChristophWurst approved these changes

Assignees
No one assigned
Labels
3. to review Waiting for reviews javascript Pull requests that update Javascript code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tchernobog @szaimen @ChristophWurst @tcitworld