Feature/59178 journalize changes to project stages and gates #17595

toy · 2025-01-13T18:34:16Z

Ticket

OP#59178

What are you trying to accomplish?

Journal activation/deactivation of project life cycle steps and setting/changing/removing their dates.
Show those journals when «Project details» is selected in activities event type filter

What approach did you choose and why?

Not using feature flag, as both activation and editing of project life cycle steps are already hidden behind it
Not creating initial journals, as project stages and gates should not yet be used by anyone
Renamed event type itself from project_attributes to project_details, as project attributes is the name for project custom fields. Name project_details is also used in few migrations, but that is much less then usage of project_attributes. Created a compatibility alias for users that saved the link or have event types stored in session. OP#61285 to cleanup.

Merge checklist

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

dombesz

I found a few minor issues and left comments about them. Notably the N+1 issues would require attention, and the scenario below also produces interesting results:

Enable a stage
Set the date interval
Disable the stage

Result: The activities show only the date interval update from step 2. The activation and deactivation does not show up as they cancel each other out.
This looks misleading as the value can only be updated if the step is activated, and the logs show nothing about the field being active. One would assume the field was activated in a previous activity, but that is incorrect.

I propose to update the def aggregatable? method, so that journals are not aggregated when a field is activated (I think deactivation can be aggregated).

We do a similar journal split in case there are multiple notes ("comments") on the same journal. The aggregation is skipped when the user makes multiple comments within the aggregation window.

On a side note, I would rather have the PR unrelated changes in separate PR's in the future, because it makes reviewing a lot easier.

Overall it's a terrific job, thanks @toy 👏

dombesz · 2025-02-13T13:15:20Z

app/models/activities/base_activity_provider.rb

@@ -187,6 +187,7 @@ def event_params(event_data)
    params
  rescue StandardError => e
    Rails.logger.error "Failed to deduce event params for #{event_data.inspect}: #{e}"
+    {}


Was this a bug?

It was intentional, but I'm not sure if there is much sense in it. Normally the method returns a Hash, but in case of exception it logs it and returns true from Rails.logger.error which is unexpected by the only use in fill_events. So returning empty Hash prevents rescuing one exception to cause another. And I am not sure about it all, as this just hides the exception, and I don't know if there are maybe legacy cases for errors that should be hidden. So maybe the correct way is just to remove rescue and logging altogether.

dombesz · 2025-02-13T19:43:24Z

app/models/activities/fetcher.rb

      case scope
-      when :all
-        @scope = event_types
+      when :all, false, nil


Not sure about these values, should we also include other possible values, like true?

This handles the case when scope is falsey from options[:scope] || :all

dombesz · 2025-02-13T20:08:46Z

app/models/journal/project_life_cycle_step_journal.rb

+  alias_attribute :date, :start_date
+
+  def date_range
+    start_date..end_date if start_date || end_date


Is this a typo, should it be if start_date && end_date ? This will generate open ended ranges.

This was intended, maybe not really warranted, but I'm trying to prevent hiding data if in some conditions only one of dates is assigned

app/services/journals/create_service/association.rb

dombesz · 2025-02-13T21:32:53Z

app/services/journals/create_service/helpers.rb

+    delegate :sanitize,
+             to: ::OpenProject::SqlSanitization
+
+    def journable_id = journable.id


It seems quite arbitrary to move the journable_id into the helper and not move the journable_type method.
I found a few more methods that could also be part of the helper: data_sink_columns, data_source_columns, journable_data_sql_addition, text_column_names, journable_timestamp, journable_type, journable_table_name, data_table_name, cause_sql.

I extracted only methods that needed to be shared between classes handling associations sql and CreateService, but looking again it will be cleaner if delegation is used

dombesz · 2025-02-14T18:55:30Z

lib/open_project/journal_formatter/project_life_cycle_step.rb

+    step = Project::LifeCycleStep.find(key[/\d+/])
+
+    name = step.definition.name


These will create 2 * M * N + 1 queries. I would suggest taking a look at the JournalFormatterCache and perhaps at the JournalFormatter::NamedAssociation to eliminate it.

spec/features/projects/life_cycle/active_in_project_spec.rb

spec/features/projects/life_cycle/overview_page/dialog/update_spec.rb

spec/lib/acts/journalized/differ/association_spec.rb

dombesz · 2025-02-17T14:28:55Z

spec/lib/acts/journalized/differ/association_spec.rb

+      end
+
+      describe "requesting single value change" do
+        let(:multiple_values) { false }


The let(:multiple_values) { true } case does not seem to be covered.

I skipped that combination, as it is not used and supposedly doesn't need to be used.
The :joined is reproducing the behaviour before this PR which is used by all other formatters, where expectation is that all values are strings and multiple and joined with comma. So I added a way to not convert dates to string only to parse them back. But from what I saw multiple values are only needed when getting single attribute for custom fields, so I skipped it for multiple_attributes_changes tests.

github-actions · 2025-02-18T18:30:08Z

Caution

The provided work package version does not match the core version

Details:

Work package URL: https://community.openproject.org/wp/59178
Work package version: 15.6.0
Core version: 15.4.0

Please make sure that:

The work package version OR your pull request target branch is correct

toy · 2025-02-19T19:32:34Z

@dombesz Thank you for thorough review

https://community.openproject.org/work_packages/59178

…tion

…e_values

… steps

toy force-pushed the feature/59178-journalize-changes-to-project-stages-and-gates branch 11 times, most recently from cc2df36 to 663575a Compare January 24, 2025 11:41

toy force-pushed the feature/59178-journalize-changes-to-project-stages-and-gates branch 5 times, most recently from 1e77315 to f6f3d25 Compare January 29, 2025 11:04

toy force-pushed the feature/59178-journalize-changes-to-project-stages-and-gates branch 5 times, most recently from fc26427 to 60fdb61 Compare February 5, 2025 15:54

toy marked this pull request as ready for review February 5, 2025 16:16

opf deleted a comment from github-actions bot Feb 7, 2025

dombesz requested changes Feb 17, 2025

View reviewed changes

toy force-pushed the feature/59178-journalize-changes-to-project-stages-and-gates branch from 8483e3c to 03f1328 Compare February 19, 2025 17:18

toy force-pushed the feature/59178-journalize-changes-to-project-stages-and-gates branch 2 times, most recently from 15949ac to 9d4a5a5 Compare February 21, 2025 19:32

[#59178] Journalize changes to project stages and gates

4069ad8

https://community.openproject.org/work_packages/59178

toy added 29 commits February 21, 2025 20:43

test multiple values for same id field

1c7c312

options to get single value changes, or non joined changes of associa…

9a878a5

…tion

displaying project life cycle step change

351f02f

moved journal_formatter specs under open_project

8ac6e9f

clearer differ association method names

69abba9

document Differ::Association methods and rename all_values to multipl…

316d14d

…e_values

stricter check (including order) for project journal changes

7021a05

test porject life cycle step journaling

8426c57

refactor project attributes activity feature spec

bc1cd72

test activity when activating or changing dates of project life cycle…

6e233a8

… steps

fix and simplify some requires in specs

b156c14

use require_relative in specs

3e137d2

ensure that scope is an array, to not error if it is not

fb79dea

erblint + use label_tag helper

555ec5f

rename activity type project_attributes to project_details

cccc014

frozen_string_literal in changed specs

7eec9be

activity provider event_params should still return a Hash

618f32a

rename events.project_attributes to events.project_details

c0fc8a3

rework sorting of activity events

733fbc2

fix AbcSize cop namespace

eb685c7

simplify understanding errors by comparing objects instead of their ids

4e21590

cleanup before adding scope tests

5d91aeb

test activities fetcher scopes

347bed2

handle old project_attributes event type filter

f04d61a

simplify scope parsing

5ffe6c7

reorder results keys (start_date, end_date) for easier reading

2fe34f4

put association class names in a constant

df8de0a

use touch_and_save_journals

4603d1c

WIP remove grouping option for multiple_attributes_changes

a31bd23

toy force-pushed the feature/59178-journalize-changes-to-project-stages-and-gates branch from 9d4a5a5 to a31bd23 Compare February 21, 2025 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/59178 journalize changes to project stages and gates #17595

Feature/59178 journalize changes to project stages and gates #17595

toy commented Jan 13, 2025 •

edited

Loading

dombesz left a comment •

edited

Loading

dombesz Feb 13, 2025

toy Feb 18, 2025

dombesz Feb 13, 2025 •

edited

Loading

toy Feb 18, 2025

dombesz Feb 13, 2025

toy Feb 19, 2025

dombesz Feb 13, 2025

toy Feb 19, 2025

dombesz Feb 14, 2025

dombesz Feb 17, 2025

toy Feb 18, 2025

github-actions bot commented Feb 18, 2025

toy commented Feb 19, 2025

		step = Project::LifeCycleStep.find(key[/\d+/])

		name = step.definition.name

Feature/59178 journalize changes to project stages and gates #17595

Are you sure you want to change the base?

Feature/59178 journalize changes to project stages and gates #17595

Conversation

toy commented Jan 13, 2025 • edited Loading

Ticket

What are you trying to accomplish?

What approach did you choose and why?

Merge checklist

dombesz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dombesz Feb 13, 2025 • edited Loading

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

github-actions bot commented Feb 18, 2025

toy commented Feb 19, 2025

toy commented Jan 13, 2025 •

edited

Loading

dombesz left a comment •

edited

Loading

dombesz Feb 13, 2025 •

edited

Loading