When creating Journal entries, it would be useful to add single tags to entries. In this way, sets of config steps could be tagged; this would allow us to group journal entries around specific sets of config options.

For example, all the needed config steps to set up the WYSIWYG API could be tagged with "text editor"; the process of administering comments could be tagged with "comment admin", etc.

In this way, Journal could be used for documenting how a site is built/as a tool for supporting collaboration b/w teams of site builders, as well as a tool for creating step by step documentation.

As another example, let's say a site has a publishing/approval workflow set up. Using Journal with tagged entries, the steps needed to review and approve content could be laid out and annotated using Journal. This will be a great tool for end user docs.

Also, although this is ticketed as a feature request, this is work we are getting ready to do. However, we want to work with you and make sure that our work aligns with your goals, and that we aren't working at cross purposes -- more on the rationale behind this is available at http://www.funnymonkey.com/better-docs-better-sites

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Interesting.

One of the key questions is: how does one know which tags exist? (i.e. giving a clue, but also prevent duplicate tags and/or typos)

Since you asked for "the roadmap": #247635: Journal roadmap

bonobo’s picture

RE: which tags exist: autocomplete, like taxonomy, although we likely would not use taxonomy for this, as we would only want a flat hierarchy. Given that, in most cases, a small subset of fairly proficient users would be entering tags, autocomplete seems like an easy fix to this. There are other options we could explore as well (a "show all tags" link, for example), but for the first pass we would want to keep this as simple as possible.

RE the roadmap: yes, we saw that -- I see some clear overlaps between what we are suggesting and the items on the roadmap -- in particular, the arbitrary journal entries and the default journal entries for certain forms are things we could also add build out.

sun’s picture

I never looked into that... but I really wonder whether Taxonomy can also be used for other things besides nodes.

At least, IIRC, there was an internal setting for vocabularies to make them module-specific (not alterable, i.e. not assignable to nodes/content-types). What many don't know: The {users_roles}.tid column exists in Drupal's schema and was supposed to allow tagging of user roles (which was never implemented).

I wouldn't want to duplicate Taxonomy module (and all the taxonomy integration modules) with a half-baked implementation. So let's look whether we can (ab)use it.

bonobo’s picture

Seems like the main question is whether taxonomy can be used to tag other things besides nodes --

And really, our needs are much smaller than the range of what taxonomy offers; a full-blown implementation of taxonomy is definitely overkill.

Of course, in D7, this will all be much easier...

But given that we're targeting D6 for this work, we're thinking:

1. Investigate taxonomy -- see if it will work without any horrid abuses/hacks.
2. If Option 1 is not possible, then implement a lightweight categorization system that will support simple tagging with no hierarchy.

sun’s picture

That proposed procedure sounds reasonable to me. I'll try to lookup taxonomy integration possibilities in parallel if time permits.

marcp’s picture

#899: Classify *anything* in a taxonomy discusses and points out that this will be a great option in either D7 or D8.

Looking into the way that Views does its simple tagging is another idea.

bonobo’s picture

evoltech’s picture

Status: Active » Needs review
FileSize
14.85 KB

The following patch adds simple tagging as it is implemented in Views2. Also included is some formatting fixes made from your coder script. Also included is the unit test files that were not included in #474422: Journal entry does not save at admin/build/block (now in the tests directory as you prefer). The patch can be applied with "patch -p0 < journal_tag.patch". It was made against the most recent commit you made (built against a check out of the dev branch on 2009-06-21).

bonobo’s picture

FYI -- we have a patch for #480200: Export journal entries as csv, ordered, and/or unordered list ready to go -- it is dependent on the patch for this issue (in #8), so we will wait for this to land before submitting the patch over at #480200: Export journal entries as csv, ordered, and/or unordered list

sun’s picture

Status: Needs review » Needs work

ok, I didn't expect that you would merge a coder_format run into this patch - not sure whether that makes sense. I should have mentioned that coder_format - while being 99.9% concise - still has some flaws that cannot be sorted out programmatically, and thereby, some alterations need to be reverted manually afterwards. Listing those first:

- The second new line after $Id$ needs to be reverted.

- The indentation of arrays within another opening brace leads to a double-indentation of the array elements, as in journal_update_6102() - needs to be reverted.

- The multi-line re-alignment of variable assignments (as in journal_patch_view()) is most often wrong and needs to be reverted.

I'd prefer to focus on coding-style clean-ups in a separate issue and patch.

Regarding the actual changes:

+ * @todo Test this.  The db_drop_primary_key call may be causing some issues.

Can be removed. The update is known to work, but may be reported as "failed", because Drupal core does not provide API functions to check for the existence of a primary key or table indexes.

+  db_add_field($ret, 'journal', 'tag', array('type' => 'varchar', 'length' => 255, 'default' => '', 'description' => t('A tag used to group/sort journal entries.')));

Can we use the same "format" like in journal_schema() here? I.e. a multi-line array, putting column definition keys into the first line, and the description into the second. (Note that this is not (yet) part of Drupal's coding standards, but I write those DB field definitions in this way to ease visual diffs between 5.x and 6.x, and hence make back-ports easier.)

+    'page arguments' => array(3),
...
+    'page arguments' => array(4),

Both can be removed. The menu system always passes any additional arguments in the current path to the page callback.

+  $items['admin/journal/ajax/autocomplete/tag'] = array(
+    'access callback' => 'user_access',

user_access is the default access callback and can thus be omitted.

+    '#description' => t('Enter an optional tag for this journal entry; it is used only to help sort journal entries on the !report_page.', array('!report_page' => l(t('report page'), 'admin/reports/journal'))),

Hyperlinks inside t() are always written inline: <a href="@journal-url">report page</a> and you use the @ placeholder along with url() as value.

The first part of this description should start with Optionally ....

The second part could be shortened a bit: only used to filter entries in the journal. ("journal" linked)

Also, I believe that the tag should come after the entry field, no?

-function journal_view() {
-  $sql = "SELECT j.*, u.name FROM {journal} j INNER JOIN {users} u ON j.uid = u.uid";
+function journal_view($tag = '') {
+  $sql = "SELECT j.*, u.name FROM {journal} j INNER JOIN {users} u ON j.uid = u.uid WHERE tag LIKE LOWER('%s%%')";

The tag is optional - hence, the query has to conditionally use an additional WHERE clause - otherwise you could not list all entries (with or without tag) anymore.

+  $result = pager_query($sql . $tablesort, 50, 0, NULL, array($tag));

The array() around $tag can be removed.

+          l($entry->tag, 'admin/reports/journal/'. $entry->tag),

Wrong string concatenation; use a space before and after the period. (Always follow coding standards for HEAD -- probably also means that you used coder_format from 6.x?)

That's all for now (didn't look at the test though).

Q: Views outputs the tags above the views -- shouldn't we do that as well or will that be part of the other views patch?

evoltech’s picture

FileSize
12.09 KB

+ * @todo Test this. The db_drop_primary_key call may be causing some issues.

Can be removed. The update is known to work, but may be reported as "failed", because Drupal core does not provide API functions to check for the existence of a primary key or table indexes.

OK. This was a bit confusing, I just wanted to make sure it was documented.

Q: Views outputs the tags above the views -- shouldn't we do that as well or will that be part of the other views patch?

I am not sure exactly what you are talking about here. The next patch that deals with #480200: Export journal entries as csv, ordered, and/or unordered list, incorporates views, yes. In the default views that will be included with the patch for #480200: Export journal entries as csv, ordered, and/or unordered list journal entries are included before the tag.

I have re-rolled the patch with the recommendations you made in #10.

evoltech’s picture

FileSize
12.43 KB

Ooops there was supposed code included for adding the journal forms into a fieldset. This is the same patch as above with those additions.

sun’s picture

  db_add_field($ret, 'journal', 'tag', array('type' => 'varchar', 'length' => 255, 'default' => '', 
    'description' => t('A tag used to group/sort journal entries.')));
  db_add_field($ret, 'journal', 'tag', array(
    'type' => 'varchar', 'length' => 255, 'default' => '',
    'description' => t('A tag used to group/sort journal entries.'),
  ));

Which of both is cleaner and more readable?

+  $items['admin/journal/ajax/autocomplete/tag'] = array(
+    'access arguments' => array('access journal'),
+    'type' => MENU_CALLBACK,
+    'page callback' => 'journal_autocomplete_tag',
+  );

Here, 'page callback' should be the first element.

+    '#title' => t('Journal'),
     '#weight' => $journal_weight,
     '#tree' => FALSE,
+    '#type' => 'fieldset',
+    '#collapsible' => TRUE,
+    '#collpased' => FALSE,
+

Turning this into a fieldset is a separate issue, and perhaps a won't fix, too. (Additionally, spot the typo in collpased ;)

+    '#title' => t('Journal tag'),

I'm not sure whether a user groks this at first sight.

   if (!empty($form_state['values']['journal_entry'])) {
-    journal_add_entry($form_state['values']['journal_entry'], $form_state['values']['journal_location']);
+    journal_add_entry($form_state['values']['journal_entry'], $form_state['values']['journal_location'], $form_state['values']['journal_tag']);
   }
   unset($form_state['values']['journal_entry'], $form_state['values']['journal_location']);

Like the other values, we do not want 'journal_tag' to stay in the form values. system_settings_form() will define it a system variable otherwise.

+  if (!empty($tag)) {
+    $sql .= " WHERE tag LIKE LOWER('%s%%')";
+  }
...
+    $result = db_query_range("SELECT DISTINCT tag FROM {journal} WHERE LOWER(tag) LIKE LOWER('%s%%')", array($string), 0, 10);

The autocomplete does it properly. For the journal view, you are comparing lowercase with anycase. That might work on some database servers where LIKE does an case-insensitive matching, but not on all.

           l(truncate_utf8($entry->location, 32, FALSE, TRUE), $entry->location),
+          l($entry->tag, 'admin/reports/journal/' . $entry->tag),

What happens if my tag is "The-best-way-to-configure-Wysiwyg-in-Drupal" ?

--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ tests/journal.test	22 Jun 2009 23:43:08 -0000

I don't understand why this file is added again? Did you CVS update your working copy?

evoltech’s picture

FileSize
12.97 KB
+  $items['admin/journal/ajax/autocomplete/tag'] = array(
+    'access arguments' => array('access journal'),
+    'type' => MENU_CALLBACK,
+    'page callback' => 'journal_autocomplete_tag',
+  );

Here, 'page callback' should be the first element.

Sure, but why?

           l(truncate_utf8($entry->location, 32, FALSE, TRUE), $entry->location),
+          l($entry->tag, 'admin/reports/journal/' . $entry->tag),

What happens if my tag is "The-best-way-to-configure-Wysiwyg-in-Drupal" ?

It works as expected.

--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ tests/journal.test 22 Jun 2009 23:43:08 -0000

I don't understand why this file is added again? Did you CVS update your working copy?

Added again? I am not sure exactly what you mean. This shows that it is added for the first time right?

The patch below addresses all issue you have in #13, except for removing fieldset. I think having journal in a fieldset is important now that there are a few form entries in it as it improves usability of the module.

marcp’s picture

Actually I like this the best:

<?php
  db_add_field($ret, 'journal', 'tag',
    array(
      'type' => 'varchar', 'length' => 255, 'default' => '',
      'description' => t('A tag used to group/sort journal entries.'),
    )
  );
?>

But since it really doesn't matter functionally, what's gonna help get this patch through quickest when all the technical hurdles have been dealt with?

Let's forget about the fieldset -- that was my horrible idea to start with. It can be dealt with at the theme layer if anyone cares about doing it.

And, finally, thank you for helping us keep this progress moving along.

evoltech’s picture

FileSize
12.88 KB

Rerolled w/out fieldset.
Also removed the ability for admin/reports/journal/ to match full tags, as it was pointed out that this is a better job for views. The auto complete function however will match substrings, case insensitively.

sun’s picture

Again, journal.test is in CVS already: http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/journal/tes...

Please make sure your working copy is up to date.

evoltech’s picture

FileSize
10.54 KB

Weird. I thought cvs up would detect the new directory, but it didn't. Please see the new patch.

sun’s picture

-  // Add journal entry field.
   $form['journal']['journal_entry'] = array(
...
+  // Add journal entry field.
+  $form['journal']['journal_tag'] = array(

The original comment should stay and the new one seems to be inappropriate ;)

-  $result = pager_query($sql . $tablesort, 50);
+
+  if (!empty($tag)) {
+    $sql .= " WHERE tag LIKE '%s'";
+    $result = pager_query($sql . $tablesort, 50, 0, NULL, $tag);
+  } else {
+    $result = pager_query($sql . $tablesort, 50);
+  }

1) Wrong coding-style for if...else control structure.

2) You can pass additional arguments to database queries - they just won't be used. So all you need to do here is to conditionally enhance the SQL.

           l(truncate_utf8($entry->location, 32, FALSE, TRUE), $entry->location),
+          l($entry->tag, 'admin/reports/journal/' . $entry->tag),

What I previously meant here was that we are purposively truncating the location if it is too long, to have a sane journal table output. Since tags can equally have an arbitrary length, I think we should also truncate tags, somewhere between 12 and 20 chars.

+    $result = db_query_range("SELECT DISTINCT tag FROM {journal} WHERE LOWER(tag) LIKE LOWER('%%%s%%')", array($string), 0, 10);

I think you can remove the array() around $string.

+    $journals = array();
+
+    // Add a few journal entries.
+    for ($i = 0; $i < 10; $i++) {
+      $journal_entry = $this->randomName(8);
+      $journal_tag = $this->randomName(8);
+      $edit = array (
+        'journal_entry' => $journal_entry,
+        'journal_tag' => $journal_tag,
+      );
+      $this->drupalPost('admin/build/block', $edit, 'Save blocks');
+      $journals[$journal_entry] = $journal_tag;
+    }

You can write this cleaner, and we also don't need that many different tags:

    // Add a few journal entries.
    $journals = array();
    for ($i = 0; $i < 3; $i++) {
      $journals[] = $edit = array (
        'journal_entry' => $this->randomName(8),
        'journal_tag' => $this->randomName(8),
      );
      $this->drupalPost('admin/build/block', $edit, 'Save blocks');
    }

...

+      // Verify that the location is listed.
+      $this->assertText('admin/build/block', t('Verify that the location is listed.'));

That is already tested elsewhere.

+      $this->drupalGet('admin/reports/journal');
+      $this->assertText($entry, t('Verify that the journal entry is listed.'));
+
+      // Verify that we can filter by tag.
+      $this->drupalGet('admin/reports/journal/'. $tag);
+      $this->assertText($tag, t('Verify that the journal entry is listed when filtered by tag.'));
+
+      $this->drupalGet('admin/reports/journal/list/'. $tag);
+      $this->assertText($tag, t('Verify that the journal entry is listed when filtered by tag.'));

1) Please have a look at the assertion messages of the other tests I committed. The messages should provide some clue of what has been asserted (or not).

2) You are testing that the journal entries appear and that you can invoke the journal report with a tag and it is displayed. However, you are not testing that the filtered journal only contains entries with the tag to filter for, and you are not testing that the log contains all entries regardless of their tags when there is no tag to filter for. It should additionally be verified that the tags appear in the non-filtered log.

sun’s picture

I forgot to say: Thank you for working on this! :) Sorry, if my folllow-ups are a bit short and to the point - I'm quite busy with a project currently. :-/

And, btw, your patches have vastly improved and are much nicer to review now! Good job! :)

marcp’s picture

@sun - thanks for the quick reality check in #20 - we here at FunnyMonkey appreciate all the time that Dennison and you are putting in on getting this functionality in there.

evoltech’s picture

FileSize
10.41 KB

This patch addresses issues from #19.

marcp’s picture

I applied the patch with patch -p0 < journal_tag_5.patch and it applied cleanly. Looking at the comments in #19 and the patched code, it looks good.

The one thing I wasn't sure about in the comments in #19 was the "You can write this cleaner, and we also don't need that many different tags" comment. The two things that seem confusing in testJournalTag() are the $journals[] = $edit = array(... and the nested foreach ($journals as $journal) { loops.

The first one I'd clean up a little maybe with:

    for ($i = 0; $i < 3; $i++) {
      $edit = array(
        'journal_entry' => $this->randomName(8),
        'journal_tag' => $this->randomName(8),
      );
      $this->drupalPost('admin/build/block', $edit, 'Save blocks');
      $journals[] = $edit;
    }

The second one I haven't been able to figure out, but I also haven't run it!

@sun - have you had a chance to look this over?

bonobo’s picture

Hello, sun,

Any update on this?