ChiliProject is not maintained anymore. Please be advised that there will be no more updates.

We do not recommend that you setup new ChiliProject instances and we urge all existing users to migrate their data to a maintained system, e.g. Redmine. We will provide a migration script later. In the meantime, you can use the instructions by Christian Daehn.

Review and Merge acts_as_journalized (Feature #123)


Added by Holger Just at 2011-02-02 08:49 pm. Updated at 2011-05-30 09:34 pm.


Status:Closed Start date:2011-02-02
Priority:Normal Due date:
Assignee:Eric Davis % Done:

0%

Category:Libraries
Target version:2.0.0
Remote issue URL: Affected version:

Description

The acts_as_journalized plugin adds the generic capability to have a journal on every model in Redmine. It thereby integrates with the activity, events, and permission system of ChiliProject.

Currently, the code lives in finnlabs:master-journalized. See also the relevant Pull request into redmine.

Merge blockers

  • [B] 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
  • Move aaj/db/* into core
  • Remove submodule

Release Blockers

  • 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.
  • Check JournalsController missing code that wasn't merged
  • Check Journal model for missing code that wasn't merged
  • Check Journals views for missing code that wasn't merged
  • Move aaj/app/* into core
  • Move aaj/test/* into core
  • [B] Attachment has it's files and documents activity provider removed but only documents added
  • [B] MailHandler lines 159-ish: might be calling init_journal too late, after the attributes have been set. TO TEST
  • Move JournalsHelper to core?

Non blockers

  • [B] 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).
  • Remove aaj/test/tagging_test.rb and any other "tagging"
  • rm helper :foo from aaj controllers
  • [B] in generalize_journals db migration: # FIXME: Find some way to choose the right activity here
  • [N] Could use some more rdoc in JournalFormatter

Related issues

related to Bug #406: Check for missing Journal code from the AAJ merge Closed 2011-05-13
related to Bug #407: Add Journal#visible Closed 2011-05-13
related to Bug #152: Bug in Versioned#versions_count (acts_as_versioned) Declined 2011-02-06
related to Bug #397: FIXME in generalize_journals migration Closed 2011-05-13
related to Bug #398: Remove helper calls from IssuesController Closed 2011-05-13
related to Bug #399: Remove any leftover "tagging" from acts_as_journalized Open 2011-05-13
related to Bug #400: Review and fix the Activity event types Closed 2011-05-13
related to Bug #401: Move JournalsHelpers from aaj to the core Closed 2011-05-13
related to Bug #402: [AAJ] Check MailHandler Declined 2011-05-13
related to Bug #411: Issue Notes Preview Declined 2011-05-13
related to Bug #404: Move aaj/app/* to core Closed 2011-05-13
related to Bug #405: Move aaj/test/* to core Declined 2011-05-13
related to Bug #408: Check IssueTest#test_saving_twice_should_not_duplicate_jo... Closed 2011-05-13
related to Bug #409: [AAJ] Check that bugfix 784bbccf was merged Closed 2011-05-13
related to Bug #410: [AAJ] Check that attachment deletion messages were merged Declined 2011-05-13
related to Bug #403: [AAJ] Attachment has it's files and documents activity pr... Declined 2011-05-13
blocks Task #131: chiliproject.fr Open 2011-02-03
blocks Feature #122: User journal Open

Associated revisions

Revision b1ed5e4c
Added by Felix Schäfer at 2011-04-23 09:25 pm

Merge acts_as_journalized to unstable. #123

Revision 1f882883
Added by Eric Davis at 2011-05-06 11:08 pm

[#123] Attachment changes need to be prefixed

Revision 65953cdc
Added by Eric Davis at 2011-05-06 11:48 pm

[#123] Fix AttachmentsController test from 1f88288

Revision cc0c2165
Added by Eric Davis at 2011-05-06 11:48 pm

[#123] Fix test on postgres, .first isn't based on ORDER

Revision cd73bd71
Added by Eric Davis at 2011-05-06 11:48 pm

[#123] Fix test error due to un-reloaded object daddy objects

Revision 62c9fd21
Added by Eric Davis at 2011-05-13 06:49 pm

[#123] Removed the acts_as_journalized submodule

Revision b1845fcf
Added by Eric Davis at 2011-05-13 06:55 pm

[#123] Added latest edavis10:acts_as_journalized

Revision 0ca73a7f
Added by Eric Davis at 2011-05-13 07:20 pm

[#123] Fix the down migration for Journal#journalized_type

Revision 23ef9fed
Added by Eric Davis at 2011-05-13 07:56 pm

[#123] Port the send_notification feature to aaj

Revision adf57a64
Added by Eric Davis at 2011-05-13 08:11 pm

[#123] Move aaj migrations to core

Revision aa2b0a87
Added by Eric Davis at 2011-05-13 08:26 pm

[#123 #407] Deferred Journal#visible tests

Revision 0ba4134e
Added by Eric Davis at 2011-05-13 08:33 pm

[#123] Fix Journal notification test

Revision f79e209d
Added by Eric Davis at 2011-05-13 08:53 pm

[#123] Fix hardcoded test value with a dynamic one

Revision a80f8224
Added by Felix Schäfer at 2011-11-25 06:45 pm

Merge pull request #123 from mbreit/bugfix/698-issue-query-ruby19

[#698] Fix single-value query atoms in issue queries on Ruby 1.9

History

Updated by Holger Just at 2011-02-02 08:52 pm

  • Tracker changed from Feature to Task

Updated by Shane Pearlman at 2011-02-04 12:51 am

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

Updated by Felix Schäfer at 2011-02-20 06:05 pm

  • Target version set to 2.0.0

Updated by Eric Davis at 2011-03-18 05:57 pm

  • Category set to Libraries

Updated by Felix Schäfer at 2011-04-15 08:18 pm

For the record: I've begun merging the journalized branch to chiliproject 1.2.0 here: https://github.com/finnlabs/chiliproject/tree/master-journalized. Please note I haven't even yet begun testing this locally, this is purely a merge. Tim has tested it though and mentioned a few tests have to be adapted to the journalized stuff.

I think we could have it ready for review by monday or tuesday, Eric, would you have the time to review it by then?

  • Assignee set to Felix Schäfer

Updated by Eric Davis at 2011-04-15 09:19 pm

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.

Updated by Eric Davis at 2011-04-16 11:58 pm

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

Updated by Felix Schäfer at 2011-04-21 09:55 am

Felix Schäfer wrote:

For the record: I've begun merging the journalized branch to chiliproject 1.2.0 here: https://github.com/finnlabs/chiliproject/tree/master-journalized. Please note I haven't even yet begun testing this locally, this is purely a merge. Tim has tested it though and mentioned a few tests have to be adapted to the journalized stuff.

All tests pass locally on that branch, though I have only svn and git installed for the SCM tests. This is still on chiliproject 1.2 too.

Eric, if you don't want to have it merged to unstable straightaway, what do you think of a "journalized" branched off of unstable where I could merge the stuff and everyone could work on?

  • Status changed from Open to In Progress

Updated by Eric Davis at 2011-04-21 08:04 pm

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).

Updated by Felix Schäfer at 2011-04-22 01:34 pm

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 :-)

Updated by Felix Schäfer at 2011-04-23 08:11 pm

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.

Updated by Tim Felgentreff at 2011-04-29 08:29 am

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

Updated by Eric Davis at 2011-05-06 08:06 pm

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.

Updated by Eric Davis at 2011-05-06 08:08 pm

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

Updated by Eric Davis at 2011-05-06 09:57 pm

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

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?

Updated by Felix Schäfer at 2011-05-06 10:00 pm

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.

Updated by Felix Schäfer at 2011-05-06 10:23 pm

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)

Updated by Tim Felgentreff at 2011-05-07 07:04 am

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)

Updated by Felix Schäfer at 2011-05-09 09:25 am

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.

Updated by Eric Davis at 2011-05-13 04:42 pm

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 by Eric Davis at 2011-05-13 04:45 pm

Getting this ready to be merged into the core. I'll open separate issues for bugs/changes so we can all work on them separately (or decide to pass on them until post-2.0.0).

  • Assignee changed from Felix Schäfer to Eric Davis

Updated by Eric Davis at 2011-05-13 05:45 pm

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).

Updated by Eric Davis at 2011-05-13 07:15 pm

I fixed the major issues and merged acts_as_journalized into unstable. (dabe5ca)

  • remove submodule
  • move db migrations to core
  • fixed bug in down migration
  • fixed tests from the merge.

Now we just need to work on the remaining issues and we are done (check related and version 2.0.0's open issues)

Thanks everyone for your hard work on this. I'm happy we finally got it added.

  • Status changed from In Progress to Closed

Updated by Felix Schäfer at 2011-05-13 10:44 pm

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.

Updated by Felix Schäfer at 2011-05-13 10:45 pm

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

Updated by Eric Davis at 2011-05-30 09:34 pm

  • Tracker changed from Task to Feature

Also available in: Atom PDF