Draft: [72234] Workflows UX improvement: Show workflows by type

[dfriquet]

Ticket

https://community.openproject.org/wp/72234

What are you trying to accomplish?

• Global workflows shows an index page for all types present in the system
• Quick filter for name
• When opening a workflow for a type, the workflow table is shown as-is
  • Type selection at the top is removed

Screenshots

What approach did you choose and why?

1. type_id is now a required URL param, not a optional query param;
2. The old WorkflowsController#show in now WorkflowsController#summarized to prevent any confusion with a usual RESTful show action.

Merge checklist

• Added/updated tests
• Added/updated documentation in Lookbook (patterns, previews, etc)
• Tested major browsers (Chrome, Firefox, Edge, ...)

[myabc]

I'm a slow convert to the AS convenience method:

Suggested change

	unless %i[index edit].include?(@state)
	unless @state.in?(%i[index edit]

[myabc]

I realise this is WIP, so apologies if I've jumped the gun with giving feedback!

We should probably add an action menu with "Edit" item for consistency with other BorderBoxTable implementations. We may also want to show some visual cue that these are types (using __hl_type_* classes).

Having said that, for me all this begs the question as to whether workflows shouldn't be moved under Administration > Work packages > Types > Type name in the page structure. While we would have to contend with making the grid, filters and sticky tables work well together, we wouldn't need to render a separate types list page.

[myabc]

I believe the following should work - thanks to Capybara Accessible Selectors (which is where the have_heading selector comes from in fact).

Suggested change

	within "[role=table]" do
	within(:role, :table) do

[myabc]

CAS also provides a row selector.

Suggested change

	expect(page).to have_css(".Box-row[role=row]", count: 3)
	types.each do |type|
	expect(page).to have_css(".Box-row[role=row] a[href='#{edit_workflow_path(type)}']", text: type.name)
	end
	expect(page).to have_row(:row, class: "Box-row", count: 3)
	types.each do |type|
	expect(page).to have_row(:row, class: "Box-row", count: 3) do |row|
	expect(row).to have_link(type.name, href: edit_workflow_path(type))
	emd
	end

See also:

• spec/features/workflows/summary_spec.rb
• spec/components/op_primer/border_box_table_component_spec.rb

[myabc]

When you're simply asserting that something renders—rather than testing user interactions—I'd advocate for writing component specs. They're relatively cheap.

I'd also argue that feature specs shouldn't assert that specific classes are shown (unless it's legacy frontend code that uses CSS classes rather than data attributes as handles or for applying behaviour).

There are some shared examples available here with this in mind (N.B. although they probably work with feature specs, they're intended for use with component specs):
spec/support/shared/components/border_box_table_component.rb

[myabc]

We should probably establish the habit of accessibility testing with all new feature specs. This is possible with our RSpec-AXe integration:

      expect(page).to be_axe_clean.within("#content")

The only caveat is that you'll need to switch to the Selenium driver for this to work.

[myabc]

N.B. this feature will run without JavaScript unless you explicitly add the :js.

[dfriquet]

I realise this is WIP, so apologies if I've jumped the gun with giving feedback!

@myabc No worries, thank you for the early feedback, it helps me to figure out the project test strategy. To be honest, I just tried to replicate the existing specs about workflows for my new page. As this piece of software has been untouched for quite some time, maybe it’s not on par with the current testing standards.

I’ll try to come up with something more up-to-date on the reviewable PR 👌

[psatyal]

We should probably add an action menu with "Edit" item for consistency with other BorderBoxTable implementations. We may also want to show some visual cue that these are types (using __hl_type_* classes).

Let's keep it simple here. We have precedence for BorderBox tables without a more icon, or even an edit action (for example admin/openid_connect/providers or admin/saml/providers). It's best for these UI-related questions to be in the work package, though, please.

Also, let's not change the positioning of the Workflow page in Admin setting. This is not in the scope of this feature. And such a change should ideally also be discussed within the work package rather than the PR, since it would be quite a structural change.

The changes to the workflow page in the context of the current epic is a result of prior discussions, notably between @oliverguenther and Niels, who's set the scope for this. If there are better ideas, we can certainly consider them, but not in a PR comment; else, we'll have trouble keeping everyone in sync on decisions that were already taken. They might not always be the best decisions or ones we agree with; they can always be discussed/challenged, but ideally not during implementation in a PR comment.

[dfriquet]

I don’t like this implementation details noise. The simplest way to get rid of it is to instantiate the actual TableComponent:

Suggested change

	let(:table) do
	instance_double(
	Workflows::TableComponent,
	columns: %i[name],
	main_column?: false,
	mobile_columns: %i[name],
	mobile_labels: %i[name],
	column_title: "Type",
	has_actions?: false
	)
	end
	let(:table) { Workflows::TableComponent.new(rows: [type]) }

That’s obviously the version closest to real life — I had to figure out the right return values for the instance_double to render the expected HTML and not some variations of the a11y roles. But we tend no to instantiate real objects to prevent tight coupling… Is that such a big deal here, considering that the TableComponent and RowComponent are tightly-coupled by a naming convention?

Another way of testing the content of the row I found in the repository is in the TableComponent spec itself. You’ll find my attempt in the file below, line 64. I usually prefer unit-testing classes in their own spec file, but with this tightly-coupled classes, a broken RowComponent will break the TableComponent spec anyway… And in the end, the spec does not read that bad when there is just a few test cases like in my case.

Is there a preferred/deprecated/use-your-own-judgement way?

Found examples:

• Meetings::TableComponent and Meetings::RowComponent specs;
• Admin::EnterpriseTokens::TableComponent;
• Projects::RowComponent.

[myabc]

@psatyal sorry! I should have kept the design "thinking out loud" for the ticket or open door.