implementation/63384 On work package comment edit set attachment replacements

[akabiru]

When a comment is updated, replace attachments.

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

[Copilot]

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

Comments suppressed due to low confidence (1)

spec/features/activities/work_package/activity_tab_comment_editor_spec.rb:176

• [nitpick] For clarity, consider assigning Attachment.where(author: admin).last to a descriptive variable rather than using it inline.

expect(comment.reload.attachments).to contain_exactly(Attachment.where(author: admin).last)

[brunopagno]

Looks great. And so straightforward 👏👏👏

One thing to think about, though, is if these checks should be part of the journal contracts instead of living in the controller.

These should become more clear and easier to handle when we extract comments from journals. But at this point the line looks a bit blurry on where the responsibility of each action lies.

In any case, just food for thought, no changes necessary. The PR looks good 👍

[akabiru]

Looks great. And so straightforward 👏👏👏

One thing to think about, though, is if these checks should be part of the journal contracts instead of living in the controller.

These should become more clear and easier to handle when we extract comments from journals. But at this point the line looks a bit blurry on where the responsibility of each action lies.

In any case, just food for thought, no changes necessary. The PR looks good 👍

Good eye, I considered baking the claims service as a dependent service to Journals{Create,Update}Service but unfortunately those services don't extend the BaseService implementation that allows for this behaviour, and since this should be soon moved to comments- perhaps it's good to keep it decoupled from journals.