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

[FEATURE] Add content changes widget #69

Conversation

NamelessCoder
Copy link
Contributor

Adds a new widget which shows:

  • Pending publishing state change, if any. Lists records
    which have starttime / endtime in the future.
  • Changed records along with name of user who made
    the change.

Attempts to select a total of $limit (default 5) records
while distributing the number of records equally between
the "pending" and "changed" sets, giving preference to
the "pending" set:

  • If limit is 5 and 3 "pending" were selected, only 2 "changed"
    will be selected.
  • If limit is 5 and no "pending" were selected, all 5 are
    selected from the "changed" set.

If there are no records in a group the table containing
the record data is not shown. if there are no results in
either of the two groups, a "nothing to show" message
is shown instead.

@NamelessCoder
Copy link
Contributor Author

Requires rebasing and change in new Fluid template file if #70 is merged first.

This was referenced Oct 20, 2019
@NamelessCoder
Copy link
Contributor Author

Rebase coming up.

Adds a new widget which shows:

* Pending publishing state change, if any. Lists records
  which have starttime / endtime in the future.
* Changed records along with name of user who made
  the change.

Attempts to select a total of $limit (default 5) records
while distributing the number of records equally between
the "pending" and "changed" sets, giving preference to
the "pending" set:

* If limit is 5 and 3 "pending" were selected, only 2 "changed"
  will be selected.
* If limit is 5 and no "pending" were selected, all 5 are
  selected from the "changed" set.

If there are no records in a group the table containing
the record data is not shown. if there are no results in
either of the two groups, a "nothing to show" message
is shown instead.
@NamelessCoder
Copy link
Contributor Author

@haassie The CGL checker is not very helpful - it can't be run locally without "composer install" which I try to avoid for sideloaded extensions, and the Travis build does not report the exact error (it is in "fix" mode but the changes are not seen).

Any chance you could make the Travis job more verbose about CGL?

@haassie
Copy link
Collaborator

haassie commented Oct 21, 2019

I added 308fa07 so we have a small diff snippet showing what is wrong and how to fix it.

@haassie
Copy link
Collaborator

haassie commented Oct 21, 2019

Do we want to check if the records shown are available for the specific user? Do they have the right permission etc? This would also be great in other widgets like the pages without meta description.

@NamelessCoder
Copy link
Contributor Author

Yep that would be a good addition, but a little bit difficult to implement (at least for the optimised query in this widget). The problem is that the access check is a post-processing and might remove too many items / the query has to select far too many items and crop the data set in PHP.

@haassie
Copy link
Collaborator

haassie commented Oct 21, 2019

Maybe we can do it "incremental"? So start with x number of items, check if there are any items removed, check the next x until it reaches the number of items we would like to show?

@NamelessCoder
Copy link
Contributor Author

That is one possibility, but we might end up dispatching a lot of queries for unprivileged users.

Copy link
Collaborator

@haassie haassie left a comment

Choose a reason for hiding this comment

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

Can we at least remove the pencil when a page is not editable for the current user? Now it will end up in nasty errors.

@@ -9,6 +9,7 @@
$widgetRegistry->registerWidget('sysLogErrors', \FriendsOfTYPO3\Dashboard\Widgets\SysLogErrorsWidget::class);
$widgetRegistry->registerWidget('t3News', \FriendsOfTYPO3\Dashboard\Widgets\T3NewsWidget::class);
$widgetRegistry->registerWidget('documentation', \FriendsOfTYPO3\Dashboard\Widgets\DocumentationWidget::class);
$widgetRegistry->registerWidget('contentChanges', \FriendsOfTYPO3\Dashboard\Widgets\ContentChangesWidget::class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Registration of widgets changed. Can you merge master into your branch and do some little refactoring in the registration?

foreach ($results as &$result) {
$result['type'] = $table;
$result['label'] = BackendUtility::getRecordTitle($table, $result);
$result['icon'] = $iconFactory->getIconForRecord($table, $result, Icon::SIZE_SMALL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not quite sure if we want coloured icons in the dashboard. We want to keep the dashboard simple and no overdue of color. On the other hand it might be a recognisable icon for the user.

@irenesacchi what do you think?

@NamelessCoder
Copy link
Contributor Author

@haassie Is there any way I could convince you to not drop the PHP registration API? It seems like a very annoying restriction - PHP allows you to do conditions etc.. exposes that you have references to your widget class file. I would absolutely keep things PHP-first.

@NeoBlack
Copy link
Member

@haassie Is there any way I could convince you to not drop the PHP registration API? It seems like a very annoying restriction - PHP allows you to do conditions etc.. exposes that you have references to your widget class file. I would absolutely keep things PHP-first.
No, especially this is the reason for the yaml registration because it is configuration and could be fetched once. no magic stuff within a registration.
Anyway, please tell me / us what is your need for "conditions etc..."

@haassie
Copy link
Collaborator

haassie commented Oct 24, 2019

I already had a discussion with Claus and next days I will be thinking if we have made the right decision. Have to check some use-cases.

@NamelessCoder
Copy link
Contributor Author

Anyway, please tell me / us what is your need for "conditions etc..."

TL;DR: YAML should be a secondary way to use public PHP API provided by EXT:dashboard.

To name a few use cases:

  • Manipulating default options of a widget
  • Using different widgets based on some setting
  • Storing widget settings outside YAML
  • Avoiding the problem of an invisible class reference

It gets even more pronounced if we include dashboards:

  • Changing the order of widgets in a dashboard or dynamically sorting them
  • Making dashboards only available to some users, some sites, etc.

But the whopper:

  • Not demanding an extension to contain the YAML file (if and when we make it possible to define a set of widgets/dashboards as global configuration detached from extension context).

I completely understand the reasons for YAML when it is appropriate, but the problem is that in many cases it really isn't appropriate - especially if it completely disables the ability to add things in PHP.

My choice would be to add YAML support as a secondary way to store widget/dashboard configurations or just provide defaults, but register the things contained in the YAML through public PHP API that is equally possible to use directly, thus skipping the need for a YAML file. Doing so also leaves you implementation agnostic and allow you to do things like take pageTSconfig as source of widget/dashboards configurations through simple adapters.

@haassie
Copy link
Collaborator

haassie commented Feb 25, 2020

The extension is now merged in core and we this repository won't be used anymore. The widget might still be an interesting one, so if you want to open up a forge ticket for it, or maybe even provide a patch, that would be awesome!

@haassie haassie closed this Feb 25, 2020
@NamelessCoder NamelessCoder deleted the features/content-changes-widget branch February 27, 2020 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants