[#59540] highlight closest predecessor in relations tab

[myabc]

⚠️ This PR is based off of #17806 - please review/merge that work first.

Ticket

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

What are you trying to accomplish?

Screenshots

What approach did you choose and why?

Merge checklist

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

[myabc]

@cbliard I've seen that there is a let_work_packages helper that takes an ASCII table string - I didn't use it in this case, but it might be a good candidate?

[cbliard]

Yes, that would be a good candidate.
The goal of this helper is to make it easier to scan and understand what's going on in the test.

Rewritten it could look like this:

  describe "#relation_group", "follows" do
    shared_let_work_packages(<<~TABLE)
      hierarchy    | MTWTFSS    | scheduling mode | predecessors
      predecessor1 | XXX        | manual          |
      predecessor2 | XX         | manual          |
      predecessor3 | XX         | manual          |
      predecessor4 |            | manual          |
      work_package |          X | automatic       | predecessor1 with lag 2, predecessor2 with lag 7, predecessor3 with lag 7, predecessor4 with lag 10
    TABLE

    let(:group) { mediator.relation_group("follows") }

    it "returns the closest relation" do
      expect(group.closest_relation).to eq _table.relation(predecessor: predecessor2)
    end

    it "returns a boolean" do
      expect(group.closest_relation?(_table.relation(predecessor: predecessor2))).to be true
      expect(group.closest_relation?(_table.relation(predecessor: predecessor1))).to be false
    end

    it "returns the relations with the closest first" do
      expect(group.closest_relations).to eq [
        _table.relation(predecessor: predecessor2),
        _table.relation(predecessor: predecessor3),
        _table.relation(predecessor: predecessor1),
        _table.relation(predecessor: predecessor4)
      ]
    end
  end

I used smaller lags and names, because having all the predecessors on one line makes the line quite big. Maybe I should add a column successors to make it look better.

Also, as the examples are about different methods, the output looks a bit strange with --format documentation:

WorkPackageRelationsTab::RelationsMediator
  RelationGroup
    #relation_group follows
      returns the closest relation
      returns a boolean
      returns the relations with the closest first

I would add a describe block for each and change some wording to make it clearer, like so:

  shared_let_work_packages(<<~TABLE)
    hierarchy    | MTWTFSS    | scheduling mode | predecessors
    predecessor1 | XXX        | manual          |
    predecessor2 | XX         | manual          |
    predecessor3 | XX         | manual          |
    predecessor4 |            | manual          |
    work_package |          X | automatic       | predecessor1 with lag 2, predecessor2 with lag 7, predecessor3 with lag 7, predecessor4 with lag 10
  TABLE

  describe "RelationGroup" do
    let(:group) { mediator.relation_group("follows") }

    describe "#closest_relation" do
      it "returns the closest follows relation" do
        expect(group.closest_relation).to eq _table.relation(predecessor: predecessor2)
      end
    end

    describe "#closest_relation?(relation)" do
      it "returns true if the given relation is the closest one, false otherwise" do
        expect(group.closest_relation?(_table.relation(predecessor: predecessor2))).to be true
        expect(group.closest_relation?(_table.relation(predecessor: predecessor1))).to be false
      end
    end

    describe "#closest_relations" do
      it "returns all follows relations with the closest first" do
        expect(group.closest_relations).to eq [
          _table.relation(predecessor: predecessor2),
          _table.relation(predecessor: predecessor3),
          _table.relation(predecessor: predecessor1),
          _table.relation(predecessor: predecessor4)
        ]
      end
    end
  end

The documentation output is nicer:

WorkPackageRelationsTab::RelationsMediator
  RelationGroup
    #closest_relation
      returns the closest relation
    #closest_relation?
      returns true if the given relation is the closest one, false otherwise
    #closest_relations
      returns all follows relations with the closest first```

[cbliard]

I'm hesitant to add a successors column.

What do you think?
Between

    shared_let_work_packages(<<~TABLE)
      hierarchy    | MTWTFSS    | scheduling mode | predecessors
      predecessor1 | XXX        | manual          |
      predecessor2 | XX         | manual          |
      predecessor3 | XX         | manual          |
      predecessor4 |            | manual          |
      work_package |          X | automatic       | predecessor1 with lag 2, predecessor2 with lag 7, predecessor3 with lag 7, predecessor4 with lag 10
    TABLE

and

    shared_let_work_packages(<<~TABLE)
      hierarchy    | MTWTFSS    | scheduling mode | successors
      work_package |          X | automatic       |
      predecessor1 | XXX        | manual          | work_package with lag 2
      predecessor2 | XX         | manual          | work_package with lag 7
      predecessor3 | XX         | manual          | work_package with lag 7
      predecessor4 |            | manual          | work_package with lag 10
    TABLE

which one reads better?

Also, only X, [, and ] count as recognized characters. Any other character can be used for space, meaning we can represent lag visually with another character to make it more "readable". What about -?

It would look like this:

    shared_let_work_packages(<<~TABLE)
      hierarchy    | MTWTFSS    | scheduling mode | successors
      work_package |          X | automatic       |
      predecessor1 | XXX--      | manual          | work_package with lag 2
      predecessor2 | XX-------  | manual          | work_package with lag 7
      predecessor3 | XX-------  | manual          | work_package with lag 7
      predecessor4 |            | manual          | work_package with lag 10
    TABLE

Would it be better? (until we have negative lags...)

[myabc]

This isn't necessarily the right place to test for this - and this expectation isn't particularly robust, given that there is only one "follows" relation in this scenario.

[cbliard]

It's fine for me: it tests that the relation is the closest one. That checks that everything it wired correctly.

When you say that it's not robust, I'm not sure to get it. In which ways could this assertion fail?

[myabc]

@cbliard as mentioned in the WP, this is ready for some initial feedback:

• is calling successor_soonest_start correct? - this is what I was most unsure about.

some more general questions:

• the work to calculate "closest" is in the component rather than the model (and also in Ruby rather than SQL - I've figured that we already have the models loaded in memory, so adding a custom comparator was the strategy I went with). I think there is scope for some later optimisation - from cursory glance it looks like we are iterating over relation groups unnecessary.
• this PR lacks feature specs - I am still trying to figure out where best to add.

[myabc]

(Leaving this in "Draft" as I don't consider this mergeable, just ready for initial feedback)

[cbliard]

It looks nice.

The feature works well, except when some of the predecessors do not have dates.
When that's the case, the one without dates is shown as the closest (if it has the oldest creation date), which does not makes sense.

Here is a screenshot:

I have not tested with all predecessors having no dates. I think that in this case it should not display any "Closest" label at all, but I think that's not specified yet.

I also made some other comments about the code and the tests.

[cbliard]

I think an instance should clearly reflect what it is to avoid confusion, here inspecting it would return something like #<Relation id: 1013, from_id: 1012, to_id: 1006, lag: 0, description: nil, relation_type: "follows"> which looks like a Relation instance, but it's not and it does not behave like it either.

It should show what it is.

What about this?

    def inspect
      "#<#{self.class.name} relation: #{relation.inspect}>"
    end

It would display #<WorkPackageRelationsTab::ClosestRelation relation: #<Relation id: 1013, from_id: 1012, to_id: 1006, lag: 0, description: nil, relation_type: "follows">> which feels less confusing to me.

Or even

    def inspect
      "#<#{self.class.name} soonest_start: #{soonest_start} relation: #{relation.inspect}>"
    end

    def soonest_start
      @soonest_start ||= relation.successor_soonest_start(gap: 0.days)
    end

that would help debugging: #<WorkPackageRelationsTab::ClosestRelation soonest_start: 2025-02-26 relation: #<Relation id: 1013, from_id: 1012, to_id:...

[cbliard]

I find the unit tests a bit hard to read, and the example descriptions are not really describing what's going on.

WorkPackageRelationsTab::ClosestRelation
  <=>
    with no lag
      with different due_date
        behaves like relation_a comes first
          left operand is less than right
      with different start_date
        behaves like relation_a comes first
          left operand is less than right
      when both due_date and start_date set due_date takes precedence
        behaves like relation_b comes first
          left operand is greater than right
      with the same due_date relation with oldest work_package (created_at) comes first
        behaves like relation_a comes first
          left operand is less than right

What about something that looks like this?

WorkPackageRelationsTab::ClosestRelation
  <=>
    with no lag
      when comparing two instances with different due dates
        compares with the respective due dates
      when comparing two instances without any due date and with different start dates
        compares with the respective start dates
      when comparing two instances with the same due date
        compares with the respective creation dates
etc.

I feel like it better communicates the intent.

[cbliard]

Also I find it a bit counter intuitive that the one with the greater predecessor end date is not the one with the greater ClosestRelation when doing comparison.

To put it in other terms,

• yesterday < today

So I would have expected

• ClosestRelation(predecessor: WorkPackage(due_date: yesterday)) < ClosestRelation(predecessor: WorkPackage(due_date: today))

And actually it is the other way around.
Is there a good reason? Cause I would find it more natural to be the other way around.

[cbliard]

Have you considered using some helper methods rather than shared examples?

For instance, writing it like this:

  def closest_relation(lag: 0, **wp_attributes)
    predecessor = build_stubbed(:work_package, **wp_attributes)
    relation = build_stubbed(:follows_relation, from: work_package, to: predecessor, lag:)
    described_class.new(relation)
  end

  context "when comparing two instances with different due dates" do
    it "compares with the respective due dates" do
      expect(closest_relation(due_date: today + 1.day)).to be > closest_relation(due_date: today + 2.days)
      expect(closest_relation(due_date: today)).to be < closest_relation(due_date: 2.days.ago)
    end
  end

More context about this:

One principle I like about rspec is to have a self-contained test, and using shared examples helps with brevity, but makes the reader jump back and forth to understand what the test does and how it works.

For instance, with "relation_a comes first", I'm not sure what "comes first" means, and I do not know how objects are built. Then when going to the implementation, I see result, and I have to jump to the subject definition, and then I have to check what -1 means. Is it a > b or the other way around?

And then after reading, I understand that coming first means it is the smallest when comparing (a < b).

I find it easier to read about when the assertion is expressed in one line like this:

expect(closest_relation(due_date: today + 1.day)).to be > closest_relation(due_date: today + 2.days)

And it's easy to add multiple different expectations in the same test case without much ceremony:

expect(closest_relation(due_date: today + 1.day)).to be > closest_relation(due_date: today + 2.days)
expect(closest_relation(due_date: today + 1.day)).to be > closest_relation(due_date: nil)
expect(closest_relation(due_date: 5.years.ago)).to be > closest_relation(due_date: today + 2.days)

It's not always possible though.

What do you think?

[myabc]

RSpec offers a very rich API*, and as such I sometimes succumb to the temptation to reach for a RSpec DSL methods like shared_example, forgetting that it is still just Ruby code. I can see that defining a closest_relation helper makes a lot more sense here - and that it'll express intent more concisely.

* some would argue too comprehensive!

And then after reading, I understand that coming first means it is the smallest when comparing (a < b).

You're right. I'm using "coming first" somewhat out-of-context. Of course, the item would come first if the #sort were called on an Array of ClosestRelation instances ([ClosestRelation.new(relation:), ClosestRelation.new(relation:)].sort), but that's not what we're testing here. (tbh I managed to confuse myself when writing some of these examples - so that is probably a bad sign!)

[cbliard]

Yes, that would be a good candidate.
The goal of this helper is to make it easier to scan and understand what's going on in the test.

Rewritten it could look like this:

  describe "#relation_group", "follows" do
    shared_let_work_packages(<<~TABLE)
      hierarchy    | MTWTFSS    | scheduling mode | predecessors
      predecessor1 | XXX        | manual          |
      predecessor2 | XX         | manual          |
      predecessor3 | XX         | manual          |
      predecessor4 |            | manual          |
      work_package |          X | automatic       | predecessor1 with lag 2, predecessor2 with lag 7, predecessor3 with lag 7, predecessor4 with lag 10
    TABLE

    let(:group) { mediator.relation_group("follows") }

    it "returns the closest relation" do
      expect(group.closest_relation).to eq _table.relation(predecessor: predecessor2)
    end

    it "returns a boolean" do
      expect(group.closest_relation?(_table.relation(predecessor: predecessor2))).to be true
      expect(group.closest_relation?(_table.relation(predecessor: predecessor1))).to be false
    end

    it "returns the relations with the closest first" do
      expect(group.closest_relations).to eq [
        _table.relation(predecessor: predecessor2),
        _table.relation(predecessor: predecessor3),
        _table.relation(predecessor: predecessor1),
        _table.relation(predecessor: predecessor4)
      ]
    end
  end

I used smaller lags and names, because having all the predecessors on one line makes the line quite big. Maybe I should add a column successors to make it look better.

Also, as the examples are about different methods, the output looks a bit strange with --format documentation:

WorkPackageRelationsTab::RelationsMediator
  RelationGroup
    #relation_group follows
      returns the closest relation
      returns a boolean
      returns the relations with the closest first

I would add a describe block for each and change some wording to make it clearer, like so:

  shared_let_work_packages(<<~TABLE)
    hierarchy    | MTWTFSS    | scheduling mode | predecessors
    predecessor1 | XXX        | manual          |
    predecessor2 | XX         | manual          |
    predecessor3 | XX         | manual          |
    predecessor4 |            | manual          |
    work_package |          X | automatic       | predecessor1 with lag 2, predecessor2 with lag 7, predecessor3 with lag 7, predecessor4 with lag 10
  TABLE

  describe "RelationGroup" do
    let(:group) { mediator.relation_group("follows") }

    describe "#closest_relation" do
      it "returns the closest follows relation" do
        expect(group.closest_relation).to eq _table.relation(predecessor: predecessor2)
      end
    end

    describe "#closest_relation?(relation)" do
      it "returns true if the given relation is the closest one, false otherwise" do
        expect(group.closest_relation?(_table.relation(predecessor: predecessor2))).to be true
        expect(group.closest_relation?(_table.relation(predecessor: predecessor1))).to be false
      end
    end

    describe "#closest_relations" do
      it "returns all follows relations with the closest first" do
        expect(group.closest_relations).to eq [
          _table.relation(predecessor: predecessor2),
          _table.relation(predecessor: predecessor3),
          _table.relation(predecessor: predecessor1),
          _table.relation(predecessor: predecessor4)
        ]
      end
    end
  end

The documentation output is nicer:

WorkPackageRelationsTab::RelationsMediator
  RelationGroup
    #closest_relation
      returns the closest relation
    #closest_relation?
      returns true if the given relation is the closest one, false otherwise
    #closest_relations
      returns all follows relations with the closest first```

[cbliard]

With the new it of ruby 3.4, this can be rewritten like this:

Suggested change

	within relation_row do
	expect(page).to have_css(".Label", text: "Closest")
	end
	within relation_row do
	expect(it).to have_css(".Label", text: "Closest")
	end

Not that useful, but I find it neat in conjunction with within.

[cbliard]

I realize I did not address your initial questions

• is calling successor_soonest_start correct? - this is what I was most unsure about.

Yes, definitely. This method is the one used in the scheduling (through WorkPackage#soonest_start defined in app/models/work_package/scheduling_rules.rb).

some more general questions:

• the work to calculate "closest" is in the component rather than the model (and also in Ruby rather than SQL - I've figured that we already have the models loaded in memory, so adding a custom comparator was the strategy I went with). I think there is scope for some later optimisation - from cursory glance it looks like we are iterating over relation groups unnecessary.

Yes, there is probably room for optimization, but let's focus on making it work first with reasonable performances.

• this PR lacks feature specs - I am still trying to figure out where best to add.

Maybe in existing feature specs about relations? I saw you updated one. Not sure if there are other ones existing.

[cbliard]

swapping the line with related_work_package4 with the line related_workPackage1 makes the test fail.

[myabc]

@cbliard fantastic! - thanks for the detailed review and great suggestions. I'll fix the bug with predecessors without dates, and then begin incorporating your other suggestions!