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

Augment the category menu by system tags and already used categories #5161

Conversation

rotdrop
Copy link
Contributor

@rotdrop rotdrop commented Apr 24, 2023

This commit add all available "collaborative tags" and all already used categories into option groups of the tags-menu of the side-bar editor.

Determining the set of already used categories is a little bit ugly: it used the oc_calendarobject_props table which might be considered "internal". However, this is the Nextcloud calendar app which only talks to the Nextcloud calendar server. So using this "internal ingredient" might be acceptable.

This commit addresses and is a related to a couple of open issues:

#3735 Calendar Categories: Propose Categories already used

  • this should be fixed by this commit

#1644 Add own categories, delete default ones

  • this is partly fixed in the sense that collaboritive tags are now also proposed as calendar categories.
  • still default categories cannot be deleted
  • however, using option groups one at least has some sort of overview about the origin of the proposed category

nextcloud/server#29950 Save VEVENT CATEGORIES as vcategory

  • this issue is totally "ignored" by this commit as the proposed solution there is not needed (the categories are already there in the oc_calendarobject_props table)
  • that would have to be discussed there: but my impression that the tables and classed mentioned there are obsolete and no longer used.

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.39 🎉

Comparison is base (5328ff6) 22.36% compared to head (287342d) 22.76%.

❗ Current head 287342d differs from pull request most recent head b3af4c3. Consider uploading reports for the commit b3af4c3 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5161      +/-   ##
============================================
+ Coverage     22.36%   22.76%   +0.39%     
- Complexity      359      368       +9     
============================================
  Files           236      237       +1     
  Lines         11554    11629      +75     
  Branches       2250     2255       +5     
============================================
+ Hits           2584     2647      +63     
- Misses         8970     8982      +12     
Flag Coverage Δ
javascript 13.98% <0.00%> (-0.02%) ⬇️
php 65.53% <ø> (+1.13%) ⬆️

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

Impacted Files Coverage Δ
...nents/Editor/Properties/PropertySelectMultiple.vue 0.00% <0.00%> (ø)
...Properties/PropertySelectMultipleColoredOption.vue 0.00% <0.00%> (ø)
src/mixins/EditorMixin.js 3.61% <0.00%> (-0.05%) ⬇️
src/views/EditSidebar.vue 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rotdrop rotdrop force-pushed the feature/main/modifyable-default-categories branch 3 times, most recently from 25cdc17 to fc89c82 Compare April 25, 2023 07:38
@rotdrop rotdrop force-pushed the feature/main/modifyable-default-categories branch from fc89c82 to bc3e211 Compare April 25, 2023 08:29
@miaulalala
Copy link
Contributor

miaulalala commented Apr 25, 2023

Hi, thanks so much for your PR!

The system tags look great, that can stay as it is, but unfortunately, querying the oc_calendar_properties table from the calendar app is a big no no. The table, despite it's misleading name, doesn't belong to the calendar app but the dav app in server. If the dav app changes anything about the table, it could break the calendar app. So the custom tags need to be exposed via an API from the dav app to get them in the calendar app.

If you need help with that, let me know! You can find any calendar related code in apps/dav/lib/CalDAV.

@rotdrop
Copy link
Contributor Author

rotdrop commented Apr 25, 2023

Hi, thanks so much for your PR!

The system tags look great, that can stay as it is, but unfortunately, querying the oc_calendar_properties table from the calendar app is a big no no. The table, despite it's misleading name, doesn't belong to the calendar app but the dav app in server. If the dav app changes anything about the table, it could break the calendar app

  1. There is of course the possibility to just use the calendar PHP API from lib/public/Calendar/, fetch all user calendars, fetch all events, extract from each event the categories etc. This is not difficult but maybe somewhat inefficient.
  2. And then there are those vcategory and vcategory_to_object tables mentioned in Save VEVENT CATEGORIES as vcategory server#29950. These would be accessible through the ITagManager and ITag interfaces. However, those two tables appear to be unused (I think really just unused, not only unused by the dav app).

So a clean implementation which lives without further modifications of the nextcloud/server and/or the dav-app would have to use possibility 1).

@rotdrop rotdrop force-pushed the feature/main/modifyable-default-categories branch 2 times, most recently from 7ae8560 to 93f13e5 Compare April 25, 2023 10:43
@rotdrop
Copy link
Contributor Author

rotdrop commented Apr 25, 2023

1. There is of course the possibility to just use the calendar PHP API from `lib/public/Calendar/`, fetch all user calendars, fetch all events, extract from each event the categories etc. This is not difficult but maybe somewhat inefficient.

2. And then there are those `vcategory` and `vcategory_to_object` tables mentioned in [Save VEVENT CATEGORIES as vcategory server#29950](https://github.com/nextcloud/server/issues/29950). These would be accessible through the `ITagManager` and `ITag` interfaces. However, those two tables appear to be unused (I think really just unused, not only unused by the dav app).

So a clean implementation which lives without further modifications of the nextcloud/server and/or the dav-app would have to use possibility 1).

The current state of the code uses the public API from lib/public/Calendar/IManager. It simply slurps in all events and extracts the categories. The result array returned by IManager->search('') is really quite complicated. I hope that the array layout is stable and will not change.

@rotdrop rotdrop force-pushed the feature/main/modifyable-default-categories branch from 93f13e5 to f0b435d Compare April 25, 2023 10:49
@rotdrop rotdrop force-pushed the feature/main/modifyable-default-categories branch from ab10fc9 to f4ec5c6 Compare April 27, 2023 07:59
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 otherwise

lib/Service/CategoriesService.php Outdated Show resolved Hide resolved
lib/Service/CategoriesService.php Outdated Show resolved Hide resolved
@rotdrop rotdrop force-pushed the feature/main/modifyable-default-categories branch from e00bda8 to 70f9347 Compare April 27, 2023 11:21
@rotdrop rotdrop force-pushed the feature/main/modifyable-default-categories branch from 70f9347 to 50e44f3 Compare May 10, 2023 07:51
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 10, 2023
@rotdrop rotdrop force-pushed the feature/main/modifyable-default-categories branch from 50e44f3 to 82fad05 Compare May 14, 2023 22:38
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.

Works

@ChristophWurst
Copy link
Member

If it's in your skill set I would appreciate unit tests for the newly added classes

Copy link
Contributor

@JohannesGGE JohannesGGE left a comment

Choose a reason for hiding this comment

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

Tested a bit, seems to work 👍

@ChristophWurst ChristophWurst self-assigned this Jun 28, 2023
@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 28, 2023
*
* @return CategoryGroup[]
*/
public function getCategories(string $userId): array {
Copy link
Member

Choose a reason for hiding this comment

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

FYI I have moved the userId from an injectable to a parameter because that makes the service usable without a user context, e.g. if we make use of it in a background job or CLI task one day

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 28, 2023
This commit add all available "collaborative tags" and all already used
categories into option groups of the tags-menu of the side-bar editor.

This commit addresses and is a related to a couple of open issues:

nextcloud#3735 Calendar Categories: Propose Categories already used

- this should be fixed by this commit

nextcloud#1644 Add own categories, delete default ones

- this is partly fixed in the sense that collaboritive tags are now also
  proposed as calendar categories.
- still default categories cannot be deleted
- however, using option groups one at least has some sort of overview
  about the origin of the proposed category

nextcloud/server#29950 Save VEVENT CATEGORIES as vcategory

- this issue is totally "ignored" by this commit as the proposed
  solution there is not needed (the categories are already there in the
  oc_calendarobject_props table)
- that would have to be discussed there: but my impression that the
  tables and classed mentioned there are obsolete and no longer used.

Co-authored-by: Anna <anna@nextcloud.com>
Signed-off-by: Claus-Justus Heine <himself@claus-justus-heine.de>
@ChristophWurst ChristophWurst force-pushed the feature/main/modifyable-default-categories branch from 287342d to b3af4c3 Compare June 28, 2023 13:34
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish 3. to review Waiting for reviews and removed 3. to review Waiting for reviews 4. to release Ready to be released and/or waiting for tests to finish labels Jun 28, 2023
@ChristophWurst
Copy link
Member

@JohannesGGE @miaulalala please have a final test after my modifications

Copy link
Contributor

@JohannesGGE JohannesGGE left a comment

Choose a reason for hiding this comment

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

Tested and works.


The tag multiselect design issue is already known, right? (couldn't find a ticket):

Right after adding the tags:
image

After reopening the event sidebar:
Screenshot from 2023-06-28 18-25-46

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature request Feature: Categories
Projects
Development

Successfully merging this pull request may close these issues.

5 participants