Allow configuration of sorting for Project Lists #15288

klaustopher · 2024-04-17T08:27:46Z

Implements https://community.openproject.org/work_packages/51671

frontend/src/stimulus/controllers/dynamic/sort-by-config.controller.ts

ulferts

The code structure is heading into the right direction. Unfortunately I found a larger number of issues that need to be worked on before merging.

From a user perspective I struggled with removing a sorting. It took me a while to understand that I could set it to the blank value again. This is how I see it specified as well but it felt akward.

Brush up design of component by adding descriptive text, placeholder and margin:

Specification:

https://www.figma.com/file/YCCMdJWkrtP9YSmf49Od0i/Project-lists?type=design&node-id=1830-33701&mode=design&t=F9bkDN6kccdsRZxi-0

Implementation:

Persist sorting when saving a list. Currently the sorting falls back to the "Project hierarchy"
Add feature spec that covers both the changing of unpersisted lists as well as saving the sort order upon persisting a list and then querying for it again e.g. in: https://github.com/opf/openproject/blob/dev/spec/features/projects/persisted_lists_spec.rb and https://github.com/opf/openproject/blob/dev/spec/features/projects/projects_index_spec.rb
Sorting by "Project hierarchy" is currently mutually exclusive with sorting by any other column. The reason for this is the inability to sort by a second/third argument on each level. That is why the projects table upon clicking on the hierarchy sorting trigger resets the sorting to only work with the "Project hierarchy".
Sorting by "Project hierarchy" only makes sense in a single direction. When having selected that option, the "Asc"/"Desc" toggle should be removed I think.
When exporting a project list (e.g. to PDF) the sort order should be applied as well.

app/components/projects/configure_view_modal_component.html.erb

ulferts · 2024-05-07T14:29:11Z

app/components/projects/configure_view_modal_component.rb

+    @selectable_columns ||= [
+      { id: :lft, name: I18n.t(:label_project_hierarchy) }
+    ] + helpers.projects_columns_options
+  end


I think this method would be better placed in the SortByComponent since that component is the only one using the method. It could then even become a private method.

No, it cannot be part of the SortByComponent as this component does not know that it is used in a project query context. So we need to pass in the selectable columns and this place is the right place, as it is the last component in the hierarchy that knows that it is dealing with a ProjectQuery

ulferts · 2024-05-07T14:31:46Z

app/components/projects/configure_view_modal_component.html.erb

@@ -29,11 +33,20 @@
                                              dragAreaLabel: I18n.t(:'queries.configure_view.columns.drag_area_label'),
                                              appendToComponent: true
                                            }%>


Now that the SortByComponent is in, it would make sense to extract the part about the columns into its own component that could then also be reused in the administration. There wouldn't be too much template code in it but a number of helper methods could be moved into it.

Isn't this PR about implementing the sorting functionality? Extracting the column stuff is a perfect candidate for a Code maintenance follow up ticket. I'd like not to extend the range of this PR as it already is way to much for what it does.

ulferts · 2024-05-07T14:49:41Z

app/components/queries/sort_by_component.rb

+
+  def current_orders
+    JSON.dump(query.orders.map { |order| [order.attribute, order.direction] })
+  end


This code serializes the query's order so that it can be placed in the form. I guess using the params-from-query controller will not work since the values would be overwritten. And I am still unsure about whether the js based approach turns out to be a good one (currently also discussing with @toy).

But if we go this way, then I'd think extracting the serialization into a shared helper right away would make sense as I would then expect the functionality to be reused time and again.

Looking at the JS code which does the same, do we need to have this code in here at all? We could let the JS set the value upon initialization.

This serializes the queries orders to the exact format how they need to be added to the query string. Can we PLEASE PLEASE PLEASE, not throw a reformatting of the query string in this PR, as we did in other places. We can do that in a follow up and change the serialization format ... But I don't know why we should ...

ulferts · 2024-05-07T14:53:38Z

app/components/queries/sort_by_component.rb

+      # Keys from the order can be symbols, strings or regexes
+      selectable_columns.select do |column_option|
+        all_order_keys.any? { |order_key| order_key === column_option[:id] }
+      end


I would move the code finding the available orders into the Queries::Orders::AvailableOrders as that reduces the interface exposed and only keep the part about the interception between the orders and the columns in here.

This will not work as for example the lft virtual column is not mentioned in the selects, so it would not work there.

ulferts · 2024-05-07T15:34:21Z

frontend/src/stimulus/controllers/dynamic/sort-by-config.controller.ts

+  declare readonly inputRowContainerTarget:HTMLElement;
+
+  connect():void {
+    this.inputRowTargets.forEach((row) => {


You should be able to also use inputRowTargetConnected. Not necessary, though.

Why would I want to do this. As we are dealing with multiple targets here, the TargetConnected method would fire 3 times and thus this code would be executed 3 times.

My suggestion was to use the inputRowTargetConnected not with this.inputRowTargets.forEach but rather with only the single row that is connected when the element is called. In either case, it was just a suggestion.

lookbook/previews/queries/sort_by_component_preview.rb

klaustopher · 2024-05-07T17:32:39Z

I don't know how you got the screenshot you posted, but there is definetly spacing, so I don't know what commit you tested. I'll add the headlines. But the way how the dropdowns are styled are how the primer component looks. There is no chevron indicator, and I don't think we should apply a special styling here...

klaustopher · 2024-05-07T17:43:26Z

Sorting by "Project hierarchy" is currently mutually exclusive with sorting by any other column. The reason for this is the inability to sort by a second/third argument on each level. That is why the projects table upon clicking on the hierarchy sorting trigger resets the sorting to only work with the "Project hierarchy".

Sorting by "Project hierarchy" only makes sense in a single direction. When having selected that option, the "Asc"/"Desc" toggle should be removed I think.

This is a bit annoying as we are already doing special things like adding the lft as a sortable column, because it is nowhere defeined. Now we also need to treat it specially and remove the order buttons. Could we make the sorting by lft somehow explicit, and define inside of the Order class what directions it supports and then forward this info to the frontend? Also, in the WorkPackages Angular view, it's not even possible to add the hierarchy sorting in the Sort By dialog

klaustopher · 2024-05-08T06:42:18Z

When exporting a project list (e.g. to PDF) the sort order should be applied as well.

The sortBy params are correctly forwarded to the export, here's an example:

Opening

http://localhost:3000/projects?columns=name+created_at+public+cf_13&sortBy=[["cf_13","desc"],["name","desc"]]

I see that the export link correctly points to

http://localhost:3000/projects.csv?columns=name+created_at+public+cf_13&sortBy=[["cf_13","desc"],["name","desc"]]

So that looks correct. I added a debug output to the Projects::ExportJob and when I output the query there, the orders param gets converted into a hash and it has the wrong order in there:

worker  | ****************************************************************************************************
worker  | #<Queries::Projects::Orders::NameOrder:0x000000032cf5af08>
worker  | #<Queries::Projects::Orders::CustomFieldOrder:0x000000032cf5a8a0>
worker  | ****************************************************************************************************

So, something goes wrong when the sortBy params are converted to the order for the export job. I'll take a look what's going wrong internally there. But that should be a problem that has already been in the code for exporting code.

Also, looking at the job params that get stored in the database, we already have a wrong order:

{
  "job_id": "2277295d-025e-4383-8dfe-e436e8c0022e",
  "locale": "en",
  "priority": 7,
  "timezone": "UTC",
  "arguments": [
    {
      "user": {
        "_aj_globalid": "gid://open-project/User/4"
      },
      "query": {
        "user": {
          "_aj_globalid": "gid://open-project/User/4"
        },
        "orders": {
          "name": {
            "value": "desc",
            "_aj_serialized": "ActiveJob::Serializers::SymbolSerializer"
          },
          "cf_13": {
            "value": "desc",
            "_aj_serialized": "ActiveJob::Serializers::SymbolSerializer"
          },
          "_aj_symbol_keys": [
          ]
        },
        "filters": [
          {
            "name": {
              "value": "active",
              "_aj_serialized": "ActiveJob::Serializers::SymbolSerializer"
            },
            "values": [
              "t"
            ],
            "operator": "=",
            "_aj_symbol_keys": [
              "name",
              "operator",
              "values"
            ]
          }
        ],
        "selects": [
          {
            "value": "name",
            "_aj_serialized": "ActiveJob::Serializers::SymbolSerializer"
          },
          {
            "value": "created_at",
            "_aj_serialized": "ActiveJob::Serializers::SymbolSerializer"
          },
          {
            "value": "public",
            "_aj_serialized": "ActiveJob::Serializers::SymbolSerializer"
          },
          {
            "value": "cf_13",
            "_aj_serialized": "ActiveJob::Serializers::SymbolSerializer"
          }
        ],
        "group_by": null,
        "_aj_symbol_keys": [
          "filters",
          "orders",
          "group_by",
          "selects",
          "user"
        ]
      },
      "export": {
        "_aj_globalid": "gid://open-project/Projects::Export/7"
      },
      "mime_type": {
        "value": "csv",
        "_aj_serialized": "ActiveJob::Serializers::SymbolSerializer"
      },
      "_aj_ruby2_keywords": [
        "export",
        "user",
        "mime_type",
        "query"
      ]
    }
  ],
  "job_class": "Projects::ExportJob",
  "executions": 0,
  "queue_name": "default",
  "enqueued_at": "2024-05-08T06:41:44.632141000Z",
  "scheduled_at": null,
  "provider_job_id": null,
  "exception_executions": {
  }
}

ulferts

All is fine now functionality wise. I'd still prefer some changes to the code but yes, we can also do it in subsequent PRs.

ulferts · 2024-05-08T16:15:58Z

Good job on digging up the reason for the export problems, @klaustopher

klaustopher force-pushed the projects-list-sort branch 6 times, most recently from c54a2cd to 14faf9a Compare April 24, 2024 06:01

klaustopher force-pushed the projects-list-sort branch from 82cfd74 to e0b5dd4 Compare April 24, 2024 14:17

github-advanced-security bot found potential problems Apr 24, 2024

View reviewed changes

frontend/src/stimulus/controllers/dynamic/sort-by-config.controller.ts Fixed Show fixed Hide fixed

klaustopher force-pushed the projects-list-sort branch 6 times, most recently from 6c48d96 to d1b6dc6 Compare April 30, 2024 12:54

klaustopher force-pushed the projects-list-sort branch from d1b6dc6 to 376974f Compare May 6, 2024 08:16

klaustopher added 13 commits May 7, 2024 10:13

Introduce Tab Panels for Configure view in project lists

e1f1d5d

Move forms around the tabs

8ff2b3f

Implement the sorting form

ca784f0

Move the SortBy stuff into its own component

21589f2

Properly use columns for sorting

a01802b

Extract some detection methods into ruby class

cc2fa7d

Extract all sorts into a main compoonent so we can bind the controller

009bd7c

Enable sortBy param when saving the query

cff90d7

Only show active fields and link as a target

9e8fb0b

Implement JS for SortByConfigController

91e5ae3

Always return something

d2ed549

Selectable fields are inserted

fb35c1d

Handle displayed status completely in JS

4717be3

klaustopher force-pushed the projects-list-sort branch from 58f2050 to 4717be3 Compare May 7, 2024 08:13

Finish the JS implementation for fields

fd88ce4

klaustopher added 3 commits May 7, 2024 15:18

Use proper Primer styling and remove debug output

7dccb82

Add i18n

1ca4138

Add lookbook

4586be0

klaustopher added the needs review label May 7, 2024

klaustopher marked this pull request as ready for review May 7, 2024 13:37

ulferts self-requested a review May 7, 2024 14:07

ulferts removed the needs review label May 7, 2024

ulferts reviewed May 7, 2024

View reviewed changes

Also persist sortBy param when saving query

1de614d

klaustopher and others added 7 commits May 8, 2024 09:23

Change serialization of orders to be an array instead of a hash

0986f7f

Remove temporary debug ouput

fdea33a

Fix layout, add texts and correct styling

6e9a7b2

Merge branch 'dev' into projects-list-sort

c2accd1

Allow saving sortBy after merge

40be919

Simplify styling

ba1f3af

Project hierarchy is only allowed in isolation

7167252

ulferts approved these changes May 8, 2024

View reviewed changes

ulferts merged commit cb13a32 into dev May 8, 2024
9 of 10 checks passed

ulferts deleted the projects-list-sort branch May 8, 2024 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuration of sorting for Project Lists #15288

Allow configuration of sorting for Project Lists #15288

klaustopher commented Apr 17, 2024

ulferts left a comment •

edited by klaustopher

ulferts May 7, 2024

klaustopher May 7, 2024

ulferts May 7, 2024

klaustopher May 7, 2024

ulferts May 7, 2024

ulferts May 7, 2024

klaustopher May 7, 2024

ulferts May 7, 2024

klaustopher May 7, 2024

ulferts May 7, 2024

klaustopher May 7, 2024

ulferts May 8, 2024

klaustopher commented May 7, 2024

klaustopher commented May 7, 2024

klaustopher commented May 8, 2024 •

edited

ulferts left a comment

ulferts commented May 8, 2024

Allow configuration of sorting for Project Lists #15288

Allow configuration of sorting for Project Lists #15288

Conversation

klaustopher commented Apr 17, 2024

ulferts left a comment • edited by klaustopher

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klaustopher commented May 7, 2024

klaustopher commented May 7, 2024

klaustopher commented May 8, 2024 • edited

ulferts left a comment

Choose a reason for hiding this comment

ulferts commented May 8, 2024

ulferts left a comment •

edited by klaustopher

klaustopher commented May 8, 2024 •

edited