Reports Feature Refactor + UI #17069

cconard96 · 2024-05-05T19:21:32Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-

This wasn't meant to be a rewrite of the report feature, but the required refactoring is extensive.

Removal of data retrieval and UI from front files.
Separation of data retrieval and UI (Prerequisite for HL API).
Twig templates for UI.
Addition of some generic report rendering methods for the various types of reports.
Cleanup of the display of some reports.
- Default report had counts with potentially confusing labels. It would list all assets by type, OS by name and then asset types (ComputerType, etc) by the itemtype name (Computer) in the same list. Now, the labels are like "Assets > Computers", "Operating Systems > Fedora Linux 38", "Types > Computers > QEMU", etc.
Conversion of forms to use GET instead of POST (both were used in different reports).
Unification of the report and its criteria (some reports have a URL just for the criteria and another just for the result).

cconard96 · 2024-05-11T02:21:41Z

Some help is needed regarding the graphs that are supposed to show for the two Infocom reports.

trasher · 2024-05-16T12:32:22Z

templates/components/datatable.html.twig

@@ -76,9 +76,9 @@
                    {% set super_header_label = super_header is array ? super_header['label'] : super_header %}
                    {% set super_header_raw = super_header is array ? super_header['is_raw'] : false %}
                    <tr>
-                        <th colspan="{{ total_cols }}">
+                        {% if super_header_raw != 'th_elements' %}<th colspan="{{ total_cols }}">{% endif %}


It sound strange to provide th elements as raw html here.

The alternative is changing the datatable template to allow a lot more customization for edge-cases.
I suppose it needs done eventually if we want to get rid of the HTMLTable* classes.

trasher · 2024-05-16T12:38:02Z

When trying to change report from the dropdown, it seems to rely on configured URI instead of current one (as it's done in master).
My GLPI URI is http://glpi.loclahost, changing report goes to http://localhost/glpi

trasher · 2024-05-16T12:43:29Z

I think I've never used that part before; I really do not know how it should work.

On default report, I have quite a different result:
on current main:

On your PR:

Event if the new display may be correct (as I do not understand the old one at all); at least the last line seems strange.

Also, please rebase, I had to switch between main and your branch, and I had to rerun dependencies install each time.

cconard96 · 2024-05-16T12:46:15Z

Event if the new display may be correct (as I do not understand the old one at all); at least the last line seems strange.

Also, please rebase, I had to switch between main and your branch, and I had to rerun dependencies install each time.

The new format still isn't great, but better. The last line indicates that it is the count for Computers with no type (----- being the display value for an empty selection). In the old report, it is the ----- on its own under the "Computer" line which is supposed to be a section heading, but there is 0 indication of that in the old report.

All the lines without a count that were in the old report are skipped in the new one to reduce noise.

trasher · 2024-05-16T12:54:57Z

"----" is the empty default for dropdowns, I'm really not sure that make sense for anyone outside this context -but I understand it was present before.
Maybe it's the good time to change for something more explicit?

cconard96 · 2024-05-16T12:56:29Z

"----" is the empty default for dropdowns, I'm really not sure that make sense for anyone outside this context -but I understand it was present before. Maybe it's the good time to change for something more explicit?

N/A is used in other places. Perhaps that but also in italics?

trasher · 2024-05-16T12:59:07Z

"----" is the empty default for dropdowns, I'm really not sure that make sense for anyone outside this context -but I understand it was present before. Maybe it's the good time to change for something more explicit?

N/A is used in other places. Perhaps that but also in italics?

It seems more easy to understand to me, yes

trasher · 2024-05-16T13:00:26Z

On report.infocom.php, I have the following error:

[2024-05-16 12:58:34] glpiphplog.CRITICAL:   *** Uncaught Exception TypeError: Unsupported operand types: int + string in /var/www/html/private/glpi/src/Report.php at line 1652

trasher · 2024-05-16T13:04:05Z

On report.infocom.php, I have the following error:

[2024-05-16 12:58:34] glpiphplog.CRITICAL:   *** Uncaught Exception TypeError: Unsupported operand types: int + string in /var/www/html/private/glpi/src/Report.php at line 1652

Problem is $item['anv'] can be '-'

cconard96 · 2024-05-16T13:07:31Z

On report.infocom.php, I have the following error:
[2024-05-16 12:58:34] glpiphplog.CRITICAL:   *** Uncaught Exception TypeError: Unsupported operand types: int + string in /var/www/html/private/glpi/src/Report.php at line 1652
Problem is $item['anv'] can be '-'

Ah. The phpdoc for Infocom::Amort is wrong.

trasher · 2024-05-16T13:11:59Z

Ah. The phpdoc for Infocom::Amort is wrong.

Well.. Most of "amort" part is wrong :D

What I do not understand is I have not that issue on main, but I also do not have the "ANV" column displayed:

cconard96 · 2024-05-16T13:16:05Z

What I do not understand is I have not that issue on main, but I also do not have the "ANV" column displayed

In French the label is VNC.

trasher · 2024-05-16T13:21:10Z

In French the label is VNC.

Indeed, I did not pay attention lang has switched (I usually work in english :D).

Another issue: I do not have the search form (Start/End date) with "Other financial and ..." (no error in logs).

trasher · 2024-05-16T13:28:52Z

On network report:

There is no "Network socket":

I have no location in my test DB, so no idea if anything wrong in this part, I only can use "By hardware". On main branch, I have data in the table for all equipements I can choose, but on your PR, I always have "No data".

Once search has been done, on main, dropdowns are no longer displayed. They are in your PR (good point I think), but current entry is not selected (maybe it's related to previous point).

trasher · 2024-05-16T13:30:49Z

End of my tests:

Loan report: seems I do not have any data, and I do no know what's it's related to,
Status report: OK!

cconard96 · 2024-05-16T13:33:07Z

On network report:

There is no "Network socket"

I have no location in my test DB, so no idea if anything wrong in this part, I only can use "By hardware". On main branch, I have data in the table for all equipements I can choose, but on your PR, I always have "No data".

Once search has been done, on main, dropdowns are no longer displayed. They are in your PR (good point I think), but current entry is not selected (maybe it's related to previous point).

Old report hid criteria if there were no items. Seemed confusing and an extra check that wasn't needed, so criteria is always visible now. All reports now have a single page for the search and display, so no more of the criteria going away and making it difficult to generate new reports.

Loan report is related to reservations (bad name).

trasher · 2024-05-16T13:37:00Z

Loan report is related to reservations (bad name).

OK, I have an SQL error (empty in are not allowed) trying to add a new reservation :/ I'll try that later

trasher · 2024-05-16T13:42:58Z

Loan report is related to reservations (bad name).

OK, I have an SQL error (empty in are not allowed) trying to add a new reservation :/ I'll try that later

ON another DB, I do not have the problem. Loan report is OK too.

trasher · 2024-05-16T13:45:04Z

So, remaining issues:

I do not have the search form (Start/End date) with "Other financial and ..." (no error in logs).
Network report, "By hardware" I have data in the table for all equipements I can choose, but on your PR, I always have "No data".
Network report, "By hardware" searched entry is not selected
when changing report from the dropdown, we should not rely on configured URL

cconard96 · 2024-05-20T11:53:36Z

So, remaining issues:

* [ ]  I do not have the search form (Start/End date) with "Other financial and ..." (no error in logs).

Report not implemented at all because it had no tabular data. The only thing it showed are the graphs which already seemed broken and I have no idea how they are supposed to work.

* [ ]  Network report, "By hardware" I have data in the table for all equipements I can choose, but on your PR, I always have "No data".
* [ ]  Network report, "By hardware" searched entry is not selected

Maybe fixed.

* [ ]  when changing report from the dropdown, we should not rely on configured URL

Fixed

trasher · 2024-05-20T13:41:53Z

So, remaining issues:
* [ ]  I do not have the search form (Start/End date) with "Other financial and ..." (no error in logs).
Report not implemented at all because it had no tabular data. The only thing it showed are the graphs which already seemed broken and I have no idea how they are supposed to work.

Unfortunately, I cannot help :/

All other issues are fixed; LGTM.

trasher · 2024-05-24T06:32:58Z

If the "Other financial and ..." report is broken; I propose to just drop it. ping @orthagh?

orthagh · 2024-05-24T07:46:49Z

If the thing is broken since a long time, you may remove it.
I would want a bit more investigation on the history of the report

trasher · 2024-05-24T08:37:10Z

I'll take an eye

trasher · 2024-05-24T08:59:29Z

OK, it seems my first tests were wrong. I've just tested again on a 10.0/bf fresh install; and graphs are working

cconard96 added ux ui wip labels May 5, 2024

cconard96 self-assigned this May 5, 2024

cconard96 force-pushed the ui/reports branch 2 times, most recently from ea7de06 to 5c7aa92 Compare May 8, 2024 01:39

cconard96 added help wanted and removed wip labels May 11, 2024

cconard96 marked this pull request as ready for review May 13, 2024 02:11

cconard96 changed the title ~~[WIP] Reports Feature Refactor + UI~~ Reports Feature Refactor + UI May 13, 2024

trasher reviewed May 16, 2024

View reviewed changes

cconard96 added 4 commits May 16, 2024 08:44

Reports Refactor + UI

50cba19

some fixes

82f9a5b

more cleanup

d369e81

lint

ae7bc8f

cconard96 force-pushed the ui/reports branch from d3b0a99 to ae7bc8f Compare May 16, 2024 12:44

fix issue where anv may be -

8c0bf83

cconard96 added 2 commits May 20, 2024 07:50

fix network report criteria

04b3611

do not use base_url

9cc085c

cedric-anne requested a review from orthagh May 21, 2024 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reports Feature Refactor + UI #17069

Reports Feature Refactor + UI #17069

cconard96 commented May 5, 2024 •

edited

cconard96 commented May 11, 2024

trasher May 16, 2024

cconard96 May 16, 2024 •

edited

trasher commented May 16, 2024

trasher commented May 16, 2024

cconard96 commented May 16, 2024 •

edited

trasher commented May 16, 2024

cconard96 commented May 16, 2024 •

edited

trasher commented May 16, 2024

trasher commented May 16, 2024

trasher commented May 16, 2024

cconard96 commented May 16, 2024

trasher commented May 16, 2024

cconard96 commented May 16, 2024

trasher commented May 16, 2024

trasher commented May 16, 2024

trasher commented May 16, 2024

cconard96 commented May 16, 2024

trasher commented May 16, 2024 •

edited

trasher commented May 16, 2024

trasher commented May 16, 2024 •

edited

cconard96 commented May 20, 2024

trasher commented May 20, 2024

trasher commented May 24, 2024

orthagh commented May 24, 2024

trasher commented May 24, 2024

trasher commented May 24, 2024

Reports Feature Refactor + UI #17069

Are you sure you want to change the base?

Reports Feature Refactor + UI #17069

Conversation

cconard96 commented May 5, 2024 • edited

cconard96 commented May 11, 2024

trasher May 16, 2024

Choose a reason for hiding this comment

cconard96 May 16, 2024 • edited

Choose a reason for hiding this comment

trasher commented May 16, 2024

trasher commented May 16, 2024

cconard96 commented May 16, 2024 • edited

trasher commented May 16, 2024

cconard96 commented May 16, 2024 • edited

trasher commented May 16, 2024

trasher commented May 16, 2024

trasher commented May 16, 2024

cconard96 commented May 16, 2024

trasher commented May 16, 2024

cconard96 commented May 16, 2024

trasher commented May 16, 2024

trasher commented May 16, 2024

trasher commented May 16, 2024

cconard96 commented May 16, 2024

trasher commented May 16, 2024 • edited

trasher commented May 16, 2024

trasher commented May 16, 2024 • edited

cconard96 commented May 20, 2024

trasher commented May 20, 2024

trasher commented May 24, 2024

orthagh commented May 24, 2024

trasher commented May 24, 2024

trasher commented May 24, 2024

cconard96 commented May 5, 2024 •

edited

cconard96 May 16, 2024 •

edited

cconard96 commented May 16, 2024 •

edited

cconard96 commented May 16, 2024 •

edited

trasher commented May 16, 2024 •

edited

trasher commented May 16, 2024 •

edited