Review and Merge acts_as_journalized (Feature #123)

Can I just share my anticipation for this feature! Big fat +1.

I have plans next weekend and don't know if I'll have much time during the week. But we should keep it open for review for at least two weeks to make sure there is enough time to test it out (roughly until the end of the month).

I have a few use cases and ideas I want to test it out with.

Watch c9497f3 too, it added journal tracking to issue descriptions which I'd like to keep as a feature.

Could you branch it off of unstable in your repo? I don't expect it to be around for long enough for it to need a branch at chiliproject (those are for really long term branches like: Rails 3, permissions rewrite, etc).

Eric Davis wrote:

Could you branch it off of unstable in your repo?

Already done. All tests currently pass on my machine, I'll review it one last time before committing and pushing.

I don't expect it to be around for long enough for it to need a branch at chiliproject (those are for really long term branches like: Rails 3, permissions rewrite, etc).

Well, I'd have liked to have the branch go through the CI before merging it to unstable, but I'll just send a pull request :-)

So, the whole shebang merged to CP unstable is here: https://github.com/finnlabs/chiliproject/tree/journalized

I've scrubbed all the whitespace and newline noise out and I've reviewed the changes to core, I haven't reviewed the acts_as_journalized plugin as such in detail yet. The plugin currently comes with all the journal controller and views it's pegged to replace, and those have been deleted from the branch already, so there is some work left to integrate the plugin completely to core. As I said, I've reviewed the core changes and I'm OK with them, I'd like Tim to have another look though and one of the other core committers should do a final review.

There are also a few missing parts I haven't checked yet but that shouldn't be too hard to add if missing:

• The issue notes preview is probably missing (e06b5815),
• I'm not sure if the issue attachment deletion notice survived the merge (5d2899ee),
• Some new JournalsController actions are probably missing from the plugin (bea49ae2),
• 784bbccf added a test for a custom field that probably didn't make it over to the plugin,
• I'm not sure this test is relevant anymore as journals don't have details anymore,
• I've also removed the "visible" scope on journals added in 27177def because I haven't found it to be used anywhere, should probably be re-added for compatibility's sake.

All in all, whomever merges this to unstable should take good care to inspect if all functionality in the JournalsController, the Journal model and corresponding views is complete in the ones provided by the plugin.

I've reviewed the merge, looks good to me.

I've reviewed the branch and have a few questions [?], potential bugs [B], and notes [N]:

• [?] Does aaj replace acts_as_activity_provider with acts_as_activity?
• [?] Does aaj replace acts_as_event? acts_as_event still needed or should Project and WikiPage be ported to aaj?
• [?] Should we hook up aaj's tests into the main suite so they run also? e.g rake test => test:units, test:functional, test:integration, test:core_plugins...
• [?] So does acts_as_journalized provide "event/activity" tracking now too?
• [?] Where is VestalVersions pulled in?
• [?] aaj/test/tagging_test.rb show tagging. What are tags?
• [B] Attachment has it's files and documents activity provider removed but only documents added
• [B] JournalsController removed, Check that the old JournalsController and aaj/JournalsController are comparable
• [B] MailHandler lines 159-ish: might be calling init_journal too late, after the attributes have been set
• [B] Remove the RJS views from the plugin, eventually...
• [B] in generalize_journals db migration: # FIXME: Find some way to choose the right activity here
• [B] rm helper :foo from controllers
• [B] rm submodule
• [N] Could use some more rdoc in JournalFormatter
• [N] Journal::Versions#at looks interesting
• [N] Journals are STI: IssueJournal, WikiContentJournal
• [N] JournalsHelper removed (in aaj now)
• [N] Model#journal_editable_by?(user) is used for permissions
• [N] aaj replaces 'acts_as_versioned' (wiki content)

I still need to look over Felix's notes, run the code, or tried to use the API yet.

Oh and should we move the migrations into the core too?

I've ran the code on my local system and found a few bugs:

• Core - https://github.com/edavis10/chiliproject/tree/ticket/unstable/123-journalized (can't send a pull request, Github is 404'ing)
• Plugin - https://github.com/finnlabs/acts_as_journalized/pull/2
• There is also a bug in the down migration: j.update_attribute(:journalized_type, j.journalized.class.name) is only getting NilClass, preventing re-migrations

But other that these the my previous notes, I like the changes.

Idea: I was thinking, if we remove the submodule and at least finish the migrations (e.g. fix the bugs and move them into the core), then we could merge the current code and open bugs for each of the open issues above. That way 1) unstable won't diverse much further from journalized and 2) as we work on 2.0 in unstable we will be testing journalized. Thoughts?

Eric Davis wrote:

Idea: I was thinking, if we remove the submodule and at least finish the migrations (e.g. fix the bugs and move them into the core), then we could merge the current code and open bugs for each of the open issues above. That way 1) unstable won't diverse much further from journalized and 2) as we work on 2.0 in unstable we will be testing journalized. Thoughts?

The earlier we merge it to unstable the better. If we all agree on principle that we want to have it in there (and we all do as far as I can tell), by all means put it into unstable.

I'll try to answer what I can:

Eric Davis wrote:

I've reviewed the branch and have a few questions [?], potential bugs [B], and notes [N]:

• [?] Does aaj replace acts_as_activity_provider with acts_as_activity?

Yes. I didn't notice the name change because it's been replaced in core and most have the activity defined in the acts_as_journalized call directly, but if one acts_as_journalized model provides more than activity@s, the extra ones have to be added through @acts_as_activity.

• [?] Does aaj replace acts_as_event? acts_as_event still needed or should Project and WikiPage be ported to aaj?

From a quick look: I think acts_as_journalized calls acts_as_event for you, but I don't think it replaces it. Maybe Tim can weigh in on that one, I'll dig in deeper if I find the time.

• [?] Should we hook up aaj's tests into the main suite so they run also? e.g rake test => test:units, test:functional, test:integration, test:core_plugins…

acts_as_journalized should be included into core, this includes tests and models/views/controllers still in the plugin for the moment. I think that was part of my notes/needs to be done list.

• [?] So does acts_as_journalized provide "event/activity" tracking now too?

Activity yes, event partly. IIRC acts_as_event is just a "common interface" at least the search and the activity view use so they have a mapping which fields of the model correspond to the information the search/activity view presents. I don't think acts_as_journalized provides that, but it includes acts_as_event for you when you acts_as_journalized a model.

The whole activity part on the other hand is a common interface for the activity view so that the controller knows which attributes it should query on what model to get the "activity list", this list is then passed to the view which uses the acts_as_event interface to show stuff. I think the activity stuff is handled by acts_as_journalized completely.

• [?] Where is VestalVersions pulled in?

AFAIK acts_as_journalized is VestalVersions reworked for ChiliProject, not integrated into.

• [?] aaj/test/tagging_test.rb show tagging. What are tags?

Probably a remainder of the VestalVersions.

• [B] Attachment has it's files and documents activity provider removed but only documents added
• [B] JournalsController removed, Check that the old JournalsController and aaj/JournalsController are comparable

See notes above, there are some commits to inspect closely to make sure they made it through the merges.

• [B] MailHandler lines 159-ish: might be calling init_journal too late, after the attributes have been set

The tests didn't pass how it was before because the "note" for the journal is only cleaned during the setting of the issue attributes and adding a "note" to an already inited journal doesn't work (or I didn't find how), with this change the tests passed and I think I did some script/console tests of the code, but no "real usage" test, so be my guest.

• [B] Remove the RJS views from the plugin, eventually…
• [B] in generalize_journals db migration: # FIXME: Find some way to choose the right activity here

The right activity?

• [B] rm helper :foo from controllers
• [B] rm submodule
• [N] Could use some more rdoc in JournalFormatter
• [N] Journal::Versions#at looks interesting
• [N] Journals are STI: IssueJournal, WikiContentJournal

Some are explicitly STIed, but I think unless you create the tables the general case is everything polymorphic journals.

• [N] JournalsHelper removed (in aaj now)

Should be moved to core along with the model/view/controller/tests?

• [N] Model#journal_editable_by?(user) is used for permissions
• [N] aaj replaces 'acts_as_versioned' (wiki content)

Felix Schäfer wrote:

I'll try to answer what I can:

Eric Davis wrote:

I've reviewed the branch and have a few questions [?], potential bugs [B], and notes [N]:

• [?] Does aaj replace acts_as_activity_provider with acts_as_activity?

Yes. I didn't notice the name change because it's been replaced in core and most have the activity defined in the acts_as_journalized call directly, but if one acts_as_journalized model provides more than activity@s, the extra ones have to be added through @acts_as_activity.

Exactly.

• [?] Does aaj replace acts_as_event? acts_as_event still needed or should Project and WikiPage be ported to aaj?

From a quick look: I think acts_as_journalized calls acts_as_event for you, but I don't think it replaces it. Maybe Tim can weigh in on that one, I'll dig in deeper if I find the time.

Right, acts_as_event is called by aaj for you, it shouldn't be called directly anymore unless you want to have events without activity for something. I thought since most of the time we have both calls in a class, we can just combine those in aaj.

• [?] Should we hook up aaj's tests into the main suite so they run also? e.g rake test => test:units, test:functional, test:integration, test:core_plugins…

acts_as_journalized should be included into core, this includes tests and models/views/controllers still in the plugin for the moment. I think that was part of my notes/needs to be done list.

• [?] So does acts_as_journalized provide "event/activity" tracking now too?

Activity yes, event partly. IIRC acts_as_event is just a "common interface" at least the search and the activity view use so they have a mapping which fields of the model correspond to the information the search/activity view presents. I don't think acts_as_journalized provides that, but it includes acts_as_event for you when you acts_as_journalized a model.

The whole activity part on the other hand is a common interface for the activity view so that the controller knows which attributes it should query on what model to get the "activity list", this list is then passed to the view which uses the acts_as_event interface to show stuff. I think the activity stuff is handled by acts_as_journalized completely.

• [?] Where is VestalVersions pulled in?

AFAIK acts_as_journalized is VestalVersions reworked for ChiliProject, not integrated into.

That's correct, aaj started when we wanted versioning and events for Versions, and I used VestalVersions for that, when I realized that we have some kind of versioning in the Wiki and Journals/JournalEvents are some kind of crippled versioning, too. So I forked VestalVersions to our github account and reworked it for Chili, to unify the different versioning schemes.

• [?] aaj/test/tagging_test.rb show tagging. What are tags?

Probably a remainder of the VestalVersions.

Yes, it is, I asked around here at the time and we weren't sure if we would want tagging at some point, so I left it, in case we want it some day. But no use-case has arisen, yet, so we can remove it, if you don't want "dead" code in there.

• [B] Attachment has it's files and documents activity provider removed but only documents added
• [B] JournalsController removed, Check that the old JournalsController and aaj/JournalsController are comparable

See notes above, there are some commits to inspect closely to make sure they made it through the merges.

• [B] MailHandler lines 159-ish: might be calling init_journal too late, after the attributes have been set

The tests didn't pass how it was before because the "note" for the journal is only cleaned during the setting of the issue attributes and adding a "note" to an already inited journal doesn't work (or I didn't find how), with this change the tests passed and I think I did some script/console tests of the code, but no "real usage" test, so be my guest.

• [B] Remove the RJS views from the plugin, eventually…
• [B] in generalize_journals db migration: # FIXME: Find some way to choose the right activity here

The right activity?

• [B] rm helper :foo from controllers
• [B] rm submodule
• [N] Could use some more rdoc in JournalFormatter
• [N] Journal::Versions#at looks interesting
• [N] Journals are STI: IssueJournal, WikiContentJournal

Some are explicitly STIed, but I think unless you create the tables the general case is everything polymorphic journals.

• [N] JournalsHelper removed (in aaj now)

Should be moved to core along with the model/view/controller/tests?

• [N] Model#journal_editable_by?(user) is used for permissions
• [N] aaj replaces 'acts_as_versioned' (wiki content)

One change I've noticed: the event types (used to display the icons on the activity list) are now object- and not event/journal-bound (a closed issue will return the issue-closed type for all its events, including past ones in which the issue was still open) and some are missing (issue-note/"a new issue note without issue edit" and issue-edit/"an issue edit" at least).

I'd vote to make that a new non-2.0 blocking issue as soon as acts_as_journalized is merged into unstable as it is purely cosmetic.

Felix Schäfer:

Agreed about the cosmetic "event types". It might be worth it to review how the activity stream is created too see if we can't pass some of that code/responsibility onto aaj. But yea, non-2.0 blocker.

Updated the description with a list of issues based on how much of a blocker they are. I'm working on the merge blockers now (just merged unstable into my aaj branch, ~120 errors).

Eric Davis wrote:

Agreed about the cosmetic "event types". It might be worth it to review how the activity stream is created too see if we can't pass some of that code/responsibility onto aaj. But yea, non-2.0 blocker.

I had identified the error and have a working fix, see https://github.com/finnlabs/chiliproject/commit/6131df3935cae773c9495c39519f7f5edec6683e, not sure it's the nicest method to correct it though. I can provide you with more info, but it should probably go in an extra ticket then.

Ah, it's already in #400, I'll post there.