-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Journal the addition and removal of File Links on Work Packages #13157
Conversation
ef2b6db
to
6b48768
Compare
f0994f5
to
ce0400a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top work! 🏆 The detailed PR description is a really nice touch 👍🏾
*I don't have enough context here so will leave the end approval to Jens :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is good and follows along the established patterns. Nothing in here surprised me in a bad way so the change itself is on a good track. I added some remarks but it should be easy to address.
What I have been wondering for some time is how to turn the Journals::CreateService
to become more flexible so that it is easier to journalize associations and to only do the journalization of e.g. custom_values where it is necessary. But this part does not need to be addressed within this PR.
I haven't tested the code myself as don't have the env set up for it. I'll see how much of a hassle it turns out to be. If I don't manage to do it, somebody else would have to cover that part.
db/migrate/20230713144232_create_storages_file_link_journals.rb
Outdated
Show resolved
Hide resolved
db/migrate/20230713144232_create_storages_file_link_journals.rb
Outdated
Show resolved
Hide resolved
modules/storages/app/services/storages/file_links/create_service.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amends look good. So from my side, this is good to merge at will.
- Adds a formatter for the file links diffs - Makes journal aware of the storables changes
…journal create service
…nsure that the addition and deletion aren't aggregated as one
c71fb03
to
f0626a7
Compare
Original ticket: https://community.openproject.org/projects/openproject/work_packages/42368
Disclaimer: This feature send me on multiple wild goose chases with a lot of dead ends, which was fun. xD
Activities ☑️
With the context from the issue, first question is: what constitutes an Activity? Any changes of the state to the object of interest. A file added, an assignee added, the creation of the object itself from the aether.
How does it work? ⚙️
Internally, every activity is an entry in the
Journal
model. Every new object there, has a version attached to it and we compare different versions against each other to create the activities. Each of these changes are adetail
inJournal
parlance.The actual mechanism of the comparison is kinda cool, but I'll leave you to explore that.
So, for
WorkPackage
s (or for anyjournalized
model) every time changes happen and are saved to the database, a newJournal
entry is created that points to the specializedJournal
model for that object. The specializedJournal
(Journal::WorkPackageJournal
in this case) it snapshots the model at the time of saving.When a
journalized
record is removed, so are all its journal entries.Attachments 📋
Attachments aren't properly an attribute of
WorkPackage
but an associated model. Also, if when you destroy an object, the journal entries go with it, so how does it show the added and removed files on screen (or via the API)?For those there's a couple of tricks. The
Journals::CreateService
instead of relying on theJournal::AttachmentJournal
it creates a special type of journal calledJournal::AttachableJournal
that points to anAttachment
and version of theJournal
that itself points to theJournal::WorkPackageJournal
.When you remove the
Attachment
anotherWorkPackageJournal
is added without theAttachableJournal
, thus making the diff between the records show that the previous file was missing.All this behaviour is dealt with within the
Journal::CreateService
massive SQL.The Solution 🧪
I've tried to mimic the behaviour already existing by introducing the
Journal::StorableJournal
(yeah, I'm terrible at naming) that will behave exactly as theJournal::AttachableJournal
. This meant editing the SQL inside theJournal::CreateService
and adding the necessary queries.Besides this, I had to add calls to the
Journal::CreateService
on both of our...FileLink::CreateService
and...FileLink::DeleteService
. The code is identical, but it felt like overkill to just create another abstraction for it (cue in Sandy Metz "the wrong abstraction is worse than repetition").I also needed to add a formatter for our
file_links
on theJournal
. This will allow us to change the rendered templates easily if needed. Besides this there was only a couple of touch-ups on other files to eager-load the new records.