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.

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

merge activity settings for calendars, events and todos #22134

Merged
merged 2 commits into from
Aug 26, 2020

Conversation

icewind1991
Copy link
Member

Signed-off-by: Robin Appelman [email protected]

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Aug 6, 2020
@icewind1991 icewind1991 added this to the Nextcloud 20 milestone Aug 6, 2020
@icewind1991 icewind1991 added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 6, 2020
@icewind1991 icewind1991 force-pushed the merge-calendar-activity-settings branch from 5fed97f to a712686 Compare August 6, 2020 16:00
@nextcloud nextcloud deleted a comment from faily-bot bot Aug 6, 2020
@icewind1991 icewind1991 force-pushed the merge-calendar-activity-settings branch from a712686 to f7d778d Compare August 6, 2020 16:07
@icewind1991 icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 6, 2020
@icewind1991 icewind1991 force-pushed the merge-calendar-activity-settings branch from f7d778d to bb5f55e Compare August 6, 2020 17:58
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Fine with me

@jancborchardt
Copy link
Member

Please review @nextcloud/calendar @nextcloud/tasks :)

@jancborchardt jancborchardt requested review from raimund-schluessler and rullzer and removed request for jancborchardt August 10, 2020 13:37
@georgehrke
Copy link
Member

What is the intention here? Is there any related Github issue?

@MorrisJobke MorrisJobke mentioned this pull request Aug 11, 2020
57 tasks
@rullzer
Copy link
Member

rullzer commented Aug 11, 2020

To have cleaner notifiation settings

@MorrisJobke
Copy link
Member

Before:
Bildschirmfoto 2020-08-11 um 21 53 30
After:
Bildschirmfoto 2020-08-11 um 21 53 18

@rullzer
Copy link
Member

rullzer commented Aug 12, 2020

Before:
Bildschirmfoto 2020-08-11 um 21 53 30
After:
Bildschirmfoto 2020-08-11 um 21 53 18

I think you switched before and after ;)

Copy link
Member

@georgehrke georgehrke left a comment

Choose a reason for hiding this comment

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

Code-wise the change is fine.

Not really happy about it though.

@tcitworld
Copy link
Member

Yeah, don't really like it either, pretty sure people won't be happy not having separate settings anymore. For instance, the deletion of a (shared) calendar is a pretty much important event that people may want to subscribe to without knowing of every event or task updates.

Why not use #22117 to create a proper dav group if the issue is too many checkboxes?

@rullzer
Copy link
Member

rullzer commented Aug 13, 2020

cc: @jancborchardt ^^^

@jancborchardt
Copy link
Member

To have the settings cleaner but also useful, we should not have the Calendar and Tasks settings in "Other activities" but rather a specific section "Calendar and Tasks" – I assume that’s what you meant @tcitworld, right?

In there we can then have entries for:

  • A calendar was modified
  • A calendar was deleted
  • A calendar event was modified
  • A calendar event was deleted
  • A task was modified
  • A task was deleted

Would that work @georgehrke @tcitworld?

@tcitworld
Copy link
Member

Right now we don't differentiate between calendar/event/tasks modifications or deletions, but yeah, that seems about right.

</settings>

<filters>
<filter>OCA\DAV\CalDAV\Activity\Filter\Calendar</filter>
<filter>OCA\DAV\CalDAV\Activity\Filter\Todo</filter>
Copy link
Member

Choose a reason for hiding this comment

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

Merging the settings fine by me, but the filter is really useful I think, as when you use fully interact with todos, the calendar changes are hard to find if they are all in one filter.

@nickvergessen
Copy link
Member

I think the list could be simplified, but this is the minimum:

  • A calendar was modified (created, deleted, shared, ...)
  • A calendar event was modified (created, deleted, updated same as files)
  • A task was modified (created, deleted, updated same as files)

@jancborchardt
Copy link
Member

I think the list could be simplified, but this is the minimum:

  • A calendar was modified (created, deleted, shared, ...)
  • A calendar event was modified (created, deleted, updated same as files)
  • A task was modified (created, deleted, updated same as files)

Then let’s do that and put it in a section "Calendar and tasks" so it’s not just in "Other". @icewind1991 can you do that?

@nickvergessen
Copy link
Member

No, he is on vacation

@icewind1991 icewind1991 force-pushed the merge-calendar-activity-settings branch from bb5f55e to aadf0f6 Compare August 25, 2020 17:11
@icewind1991
Copy link
Member Author

Changed:

image

@faily-bot
Copy link

faily-bot bot commented Aug 26, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 32341: failure

acceptance-app-files

  • tests/acceptance/features/app-files.feature:262
Show full log
  Scenario: unmarking a file as favorite causes the file list to be sorted again                          # /drone/src/tests/acceptance/features/app-files.feature:262
    Given I am logged in                                                                                  # LoginPageContext::iAmLoggedIn()
    And I create a new folder named "A name alphabetically lower than welcome.txt"                        # FileListContext::iCreateANewFolderNamed()
    And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
    And I close the details view                                                                          # FilesAppContext::iCloseTheDetailsView()
    And I see that the details view is closed                                                             # FilesAppContext::iSeeThatTheDetailsViewIsClosed()
    And I mark "welcome.txt" as favorite                                                                  # FileListContext::iMarkAsFavorite()
    And I see that "welcome.txt" is marked as favorite                                                    # FileListContext::iSeeThatIsMarkedAsFavorite()
    And I see that "welcome.txt" precedes "A name alphabetically lower than welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
    When I unmark "welcome.txt" as favorite                                                               # FileListContext::iUnmarkAsFavorite()
    Then I see that "welcome.txt" is not marked as favorite                                               # FileListContext::iSeeThatIsNotMarkedAsFavorite()
      Not favorited state icon for file welcome.txt in file list could not be found after 100 seconds (NoSuchElementException)
    And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()

@rullzer rullzer merged commit df99d8f into master Aug 26, 2020
@rullzer rullzer deleted the merge-calendar-activity-settings branch August 26, 2020 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants