Skip to content

Conversation

@ulferts
Copy link
Contributor

@ulferts ulferts commented May 8, 2025

Ticket

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

What are you trying to accomplish?

This PR introduces functionality to journalize changes to project phase definitions associated with work packages. It uses the default state changes of journals (e.g. 'Project phase set to abc'). The difference is just in the way the values are displayed. Since project phases can both be inactive in the project as well not being configured in it at all, the formatter checks for the active state of the phase in the project. If it is inactive or not configured, "(Inactive)" is appended to the name of the definition.

Screenshots

image

Merge checklist

  • Added/updated tests

if record.respond_to? :name
record.name
else
record.subject
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra path should no longer be necessary as work packages have a name method implemented by now

@ulferts ulferts marked this pull request as ready for review May 8, 2025 08:19
@toy toy requested review from Copilot and toy May 8, 2025 15:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces functionality to display project phase changes in work package journal entries with a special formatting for inactive or unconfigured phases.

  • Adds a journal formatter that appends an inactive label for project phases that are inactive or not configured.
  • Updates tests and feature specs to cover these new formatted outputs.
  • Registers the new formatter in the Journal and WorkPackage models.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
spec/lib/open_project/journal_formatter/project_phase_definition_spec.rb Adds tests verifying proper rendering of phase definitions for both active and inactive states.
spec/features/work_packages/project_phases/linking_spec.rb Updates UI tests to assert that the journal entry displays the correct change message.
lib_static/plugins/acts_as_journalized/lib/journal_formatter/named_association.rb Adjusts the handling of nil values when formatting associated object names.
lib/open_project/journal_formatter/project_phase_definition.rb Implements the formatter that adds an inactive label based on project phase activity.
app/models/work_package/journalized.rb Registers the new journal formatter field for project phase definitions.
app/models/journal.rb Registers the new journal formatter to be used for project phase definition changes.
Comments suppressed due to low confidence (1)

lib/open_project/journal_formatter/project_phase_definition.rb:35

  • Ensure that I18n.t(:label_inactive) returns exactly 'Inactive' to match test expectations, as the spec uses a hardcoded '(Inactive)' string for validating inactive project phases.
if phase_active?(object)

def format_values(values, key, cache:)
klass = class_from_field(key)

values.map do |value|
Copy link

Copilot AI May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider adding a comment or logging for the case when 'klass' or 'value' is nil to clarify that skipping processing these entries is intentional.

Suggested change
values.map do |value|
values.map do |value|
# Skip processing if 'klass' or 'value' is nil. This is intentional to avoid
# handling invalid or incomplete data.

Copilot uses AI. Check for mistakes.
@toy
Copy link
Contributor

toy commented May 8, 2025

Not a problem of this PR, but it feels odd that if a work package is assigned to inactive phase, then from looking at activity it is assigned, but when looking at the work package, it is not

@toy
Copy link
Contributor

toy commented May 9, 2025

If definition gets deleted, it will be displayed as just (Inactive), probably better if it says that it was a deleted phase

@ulferts
Copy link
Contributor Author

ulferts commented May 12, 2025

If definition gets deleted, it will be displayed as just (Inactive), probably better if it says that it was a deleted phase

Well spotted. I added a special return value for this case in the formatter. That change also made the handling of a nil object in #phase_active? superfluous.

@toy toy requested a review from Copilot May 12, 2025 12:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR makes project phase changes visible in WP activity journals by introducing a specialized journal formatter for project phase definitions. Key changes include new tests covering various phase state transitions, updates to feature specs to verify journal entries, and the implementation of the new formatter in production code.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
spec/lib/open_project/journal_formatter/project_phase_definition_spec.rb Adds comprehensive unit tests for journal formatting of phase changes.
spec/features/work_packages/project_phases/linking_spec.rb Enhances feature tests to validate phase linkage and journal display.
lib_static/plugins/acts_as_journalized/lib/journal_formatter/named_association.rb Refactors value formatting for associated objects, removing previous fallback behavior.
lib/open_project/journal_formatter/project_phase_definition.rb Implements the formatter that displays phase names and inactive state.
config/locales/en.yml Updates locale keys for project phase journaling messages.
app/models/work_package/journalized.rb, app/models/journal.rb Registers the new journal formatter for project phase definitions.
Comments suppressed due to low confidence (1)

lib_static/plugins/acts_as_journalized/lib/journal_formatter/named_association.rb:55

  • Previously, a fallback returned record.subject when the record did not respond to :name; ensure that dropping this fallback and relying solely on object&.name is intentional.
record = associated_object(klass, value.to_i, cache:)


context "when setting a phase whose definition is deleted" do
let(:old_value) { nil }
let(:new_value) { -1.to_s }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect rubocop to annoy about this:

Suggested change
let(:new_value) { -1.to_s }
let(:new_value) { "-1" }

@ulferts ulferts force-pushed the implementation/62983-project-phase-journalized-for-work-packages branch from 2efe1d9 to f84e9c5 Compare May 12, 2025 15:39
@ulferts ulferts merged commit 6b70210 into dev May 13, 2025
14 of 15 checks passed
@ulferts ulferts deleted the implementation/62983-project-phase-journalized-for-work-packages branch May 13, 2025 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants