As discussed..

Replace the checkbox list on the view message page *and* the tag select in the filter widget with an autocomplete textfield. I already did the widget replacement for participants here, they should be similiar: [##441042]

We should try to keep this relatively simple, and then improve/extend it.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

NaheemSays’s picture

Status: Active » Needs review
FileSize
11.31 KB

WIP is attached.

It also removes a few extra things which we may want to keep. I was working on top of #441042: Replace participant select so that may need to be applied first.

Not finished totally, but its enough to get some comments.

What do you think?

(one of the newly added queries needs to use the query builder, but I need to know if the rest of the approach is right or not)

NaheemSays’s picture

FileSize
27.6 KB

And a screenshot.

NaheemSays’s picture

FileSize
13.16 KB

Improved patch:

1 - Removes all the stuff removed in above patch.
2 - The weighting of the tags field is configurable in the admin UI.
3 - The mentioned query is now also using the query Builder.
4 - As the the purpose of this patch - the tag selection is via a textfield instead of the check boxes.

This one is really cnr.

Berdir’s picture

Code looks code, a few things..

- I think removing the tags page stuff is fine, we need to re-write that from scratch anyway (if we want to have a table with paging and stuff)
- We may need to add the hidden/public flag to the tags table before this, so that we can make the current tags "public" before this will add new tags. Or, another possibility, keep the tags private and don't update them. Then let the admin make them public instead of the other way around.
- What about doing something similiar as we do with the filter widget - collapse be default, make it expanded if there are tags
- Regarding default value, what do you think about moving the tag fieldset above the messages?

+      $tag = str_replace(' ', '-', $tag);
+      $tag = str_replace('#', '-', $tag);

- Are you sure that we can't use urlencode/urldecode? It would convert things like "#" to "%23". It doesn't look nice but it should imho work. Atleast twitter can do it :) If not, we would need to go the other way around.. define what's allowed and remove everything else with preg.

+      if ($tag == '') {

- empty()... :)

- autocomplete looks good, except you use "user name" once in the comment. I am wondering if we could use a helper function so that we don't have to copy paste 90% of that function.

NaheemSays’s picture

FileSize
13.29 KB

1. replaced the "user name" with "tag" as it should have been
2. Used empty()
3. got rid of the str_replace. there is a urldecode() used in privatemsg_filter_get_filter.
4. As for positioning, I left it as is - I have no opinion on its default positioning one way or the other.

NaheemSays’s picture

Title: Replace tag checkboxes/select with autocomplete textfield » Replace tag checkboxes with autocomplete textfield
FileSize
13.7 KB

Updated to have the fieldset uncollapsed if the thread has some tags already set.

NaheemSays’s picture

FileSize
14.42 KB

And another small change to a query - only use the thread_id in the query if one has been set. This allows the query to be more useful and I have replaced the query in the tag select widget box with this too.

NaheemSays’s picture

FileSize
14.89 KB

ok, this should be an almost final version of this patch as I think I have done everything.

Additions to the last patch:

1. I have re added the str_replace to replace spaces with dashes because the filter format in the get query for multipletags is "tag1 tag2 tag3" and if a tag contains a space, it will cause havoc there (but will continue to work with the save filter option.):

+      // Multiple tags are separated in the get query with a space, so to remove confusion, spaces will be replaced with '-' in tags.
+      $tag = str_replace(' ', '-', $tag);

2. Fixed the access checks. I have also modified the permissions, leaving only two: "use privatemsg_filter" and "use privatemsg_filter tags". We could potentially just use the one there, but I like allowing people to search without the ability to use tags. comments?

(the administer permission will be used from the privatemsg module - no need to have a separate one is there?)

Berdir’s picture

- I think we still need the "create tags" permission. For example, a site admin could allow the users to use the existing tags but not to create new ones. So, for a user without that permission, it would display a message like "Tag xy was ignored because you are not allowed to create new tags" instead of Step 3. Administer tags is something different as that allows access to the (not yet existing) page at admin/settings/messages/tags.

- To be more consistent with the autocomplete field, we could use "," to separate tags in the URL, I already converted the author filter do to so.

NaheemSays’s picture

FileSize
15.27 KB

Done both.

I must say though that not being allowed to create new tags seems weird when using a textfield. Then again that will be useful when adding the folder metaphor.

NaheemSays’s picture

FileSize
15.1 KB

This patch no longer requires #441042: Replace participant select to go first and should apply cleanly straight to 1.x-dev branch.

NaheemSays’s picture

FileSize
15.02 KB

Re-uploading - style fixes as in the last patch there was a line introducing spaces at the end of a function name. there are no functional changes, just getting rid of:

-function privatemsg_filter_form_submit($form, &$form_state) {
+function privatemsg_filter_form_submit($form, &$form_state) {  
Berdir’s picture

Status: Needs review » Needs work

Another look through the patch, it seems to work fine...

- As we rename/remove permissions, we should also have a update function to rename it in the DB, but I have to figure out how that would work..
- As we rename them anyway, what about giving them proper names? :) "create privatemsg_filter tags" doesn't sound really understandable. The permissions are already listed in the "privatemsg_filter" group, so it should be obvious that they belong to that module. What about:

- use filter widget
- use tags
- create tags
- administer tags (do we need that or can we merge that with administer privatemsg, not sure)

+      $tag = trim($tag);
+      $tag = $tag;
+      if (empty($tag)) {

- What are you exactly doing in the second line ? :)

+        drupal_set_message(t('Tag "@tag" was ignored because you do not have permission to create new tags.', array('@tag' => $tag)));

- Afaik, we don't need the ' "' '. Instead, you can use %tag, which will use theme_highlight()

NaheemSays’s picture

Status: Needs work » Needs review
FileSize
15.82 KB

ok, this should fix teh above.

Fixed the problems and then also added an update function to update the permissions table with the new permission names (which are "filter private messages", "use private message tags" and "create private message tags").

NaheemSays’s picture

FileSize
15.82 KB

and another patch - this one has the permission as "tag private messages" instead of "user private message tags".

NaheemSays’s picture

The update function on this patch gives me the following error when visiting update.php:

Parse error: syntax error, unexpected T_VARIABLE in /var/www/vhosts/therevival.co.uk/httpdocs/sites/all/modules/privatemsg/privatemsg_filter/privatemsg_filter.install on line 75

Leaving as cnr as when Berdir was on irc, he could not reproduce - but then again neither could I. but I was using the wrong version of the install file.

NaheemSays’s picture

FileSize
16.41 KB

New patch that fixes above issue (was unprinted utf8 characters found by Berdir - probably a c&p error somewhere, but now fixed) and also added documentation for the two queries.

NaheemSays’s picture

Updated patch with some string changes/additions.

NaheemSays’s picture

FileSize
17.15 KB

rerolled again.

tayzlor’s picture

am i correct in thinking this patch does not yet give per-user tags, but it is just replacing the checkbox mechanism with an autocomplete field so the user will not be shown xxxx amount of checkboxes for each tag, and will only have one autocomplete field instead??

NaheemSays’s picture

yes.

The suggestions will also only be those used by that user, so the tags will *appear* to be per user, but allow for greater extensibility.

ilo’s picture

Reviewed:

- privatemsg_filter.install :
* easy, two permission changes on update - ok, verified

- privatemsg_filer.module:
* Not heavily tested yet, there two many functionalities to test, I guess it would deserve a simpletest test case. From a first sight everything works fine, just should considered that:
- filter by tag, only includes the tags the user has applied.
- tagging a message will show all tags for now
The good part is that as always, patch has enough comments :)

Before I go on testing..

The callback messages/filter/tag-autocomplete has an access argument of 'filter private messages'. Should this argument be "tag private messages" instead? I've no seen any link between the filter and the autocomplete functionalities.

NaheemSays’s picture

from a functional perspective I consider filter and tag to be the same thing as the end purpose is to filter the messages. IMO.

NaheemSays’s picture

FileSize
17.15 KB

oh, I get what you mean!

attached is an updated patch that changes that one permission.

lyricnz’s picture

- entering a trailing comma in the textfield is preserved : what does that mean?
- having the tags in a separate fieldset seems like overkill: suggest it might be nicer in the main message fieldset
- is there any way to view all the global tags anymore? (and presumably edit/delete bad tags)

Feature requests:
- it would be good to have a global kill-switch that disabled message tagging entirely
- tags must be (imho) private, not shared/global like they are now. Or, at least, this should be an option.

lyricnz’s picture

Status: Needs review » Needs work

Type typing in "../" in your autocomplete field. Big javascript error.

Berdir’s picture

We already discussed the ../ thing. conclusion: it is a core #autocomplete bug, not much we can do about that...

- tags must be (imho) private, not shared/global like they are now. Or, at least, this should be an option.

The only thing that is not yet private is the autocomplete, we may need to change that. But except of that, you do not see other users tags anymore. If two users enter the same tag, they use the same row in pm_tags, but they don't know that.

- it would be good to have a global kill-switch that disabled message tagging entirely

'tag private messages' permission....

- is there any way to view all the global tags anymore? (and presumably edit/delete bad tags)

#442130: Move tags page to /admin and add public flag

litwol’s picture

Status: Needs work » Fixed
Issue tags: +pmsg ux

Thanks guys :)

We are now only a few days away from rc3. Going to let few more people test the -dev build on their sites before i tag rc3.

lyricnz’s picture

FYI, I searched for an existing Drupal Core issue about the ../ thing, and didn't find one - so raised #516418: autocomplete allows "../" input to fetch different path causing javascript errors.

lyricnz’s picture

crosspost.

Status: Fixed » Closed (fixed)
Issue tags: -filter, -pmsg ux

Automatically closed -- issue fixed for 2 weeks with no activity.