Flag_note integration

There was a bit of an issue with our 1.x version and flag_note #601724: Issue with flag abuse+flag note and I do think it would be nice to have them integrated somehow. I'm open to suggestions on accomplishing this. Haven't reviewed that module yet though.

I'm thinking it would take making abuse flags dynamic, as in being able to designate any flag as an abuse flag rather than the ore restricted approach that it takes now, where it only operates on three distinct flags only.

... actually now that I've just read the README for flag_note - it's a link type ... so this might just work out of the box - when you install flag_abuse, enable one of the abuse flags and choose the note link type. hrm...

hrm, so just took a few minutes to try it out - i've installed the 2.x-dev version of flag_note on a project i'm working on, set my existing abuse flag to use the link type, and HEY! it even gave me a modalframe! seem to work pretty well!

So, i'll not merge with flag note, but they can certainly be used together! :)

Hrm, actually it's a little bit wonky. When you go to reset the flag, it still comes up with the flag-note... I've altered the confirmation form, looks like we'll have to do something specific with this as well... reopening, updating title.

changing the flag's action to reset in hook_preprocess_flag is very likely the cause. It doesn't really serve much of a purpose anyway, so it's safe to remove that. Then I can just extend our form alter to check for the flag note form and change the text around for now.

So, this seems to work alright - let me know what you think.

Patch removes the alteration of the flag action, uses form_alter instead of form_FORM_ID_alter, then refactors the checks a little and will alter the flag_note_form if it finds it.

The one thing that is a little wonky, is that even thought we're altering the flag_note_form, this note is recorded and then immediately removed because we're in the process of 'removing' all flags... so it's really pointless to add a message at this juncture. But, at least it probably makes sense to a user now.

Of course part of this is that we'll probably want to add in the flag_note relationship and field to the default views we provide.

Attached patch does everything in #5 as well as uses hook_views_pre_view() to alter the default views and add in the flag note relationship and field.

seems to apply alright to me... make sure you have the latest DRUPAL-6--2 branch from cvs, download this file to the flag_abuse directory and (from command line) $ patch -p0 < flag_abuse-flag_note-895418_0.patch

--- terminal paste ---

[10:05][jbitner@sirkitbox:~/Sites/contrib/flag_abuse]$ ls
CVS					flag_abuse-flag_note-895418_0.patch	flag_abuse.module
cvs-release-notes.php			flag_abuse.info				includes
[10:05][jbitner@sirkitbox:~/Sites/contrib/flag_abuse]$ patch -p0 < *.patch
patching file /Users/jbitner/Sites/contrib/flag_abuse/flag_abuse.module
patching file /Users/jbitner/Sites/contrib/flag_abuse/includes/flag_abuse.views_default.inc

The patch is not in the correct format. It can only be applied to the sirkitree’s site.

You have to manually apply the patch by editing the files which are indicated in the patch file.
Remove the lines with the minus - sign and add the lines with the + sign to the corresponding files in the appropriate places.

I applied the patch and it works with a minor problem.

When the user resets a flag when he is returned to the view he gets a message

* Reset 1 flags.
+
The "Flagged message:" which is set in the abuse_node flag

The patch is almost there.

One issue I want to get noted is the "Make permanent nature of reset flags optional" http://drupal.org/node/917372

Manos
http://websynergy.gr/

The change implemented by the patch, where hook_form_FORM_ID_alter() is changed to hook_form_alter() makes all the flag checking and access checking on ALL FORMS. That's something you shouldn't want happening. For example, there is no check to see if $form['flag_name']['#value']; is actually available as field in the form. In my opinion the form alter at least should check which form is about to be altered, before performing all kinds of checks:

- * Implementation of hook_form_FORM_ID_alter().
+ * Implementation of hook_form_alter().
  */
-function flag_abuse_form_flag_confirm_alter(&$form, &$form_state) {
+function flag_abuse_form_alter(&$form, &$form_state, $form_id) {
+ if ($form_id == 'flag_confirm' || $form_id == 'flag_note_form') {
     global $user;
+    $account = $user;
+    $flag_name = $form['flag_name']['#value'];

Interested in this feature

Very interested...

If you're interested, please test the patch and report your findings.

what will happen with this on 2.x? is there a patch to test against that version?

EDIT: Sorry, got really confused about module version, actually using 2.x and knowing this issue is for 2.x. Ignore this comment, still a bit sleepy.

Patch indeed failed to apply with patch -p0 < patch. The problem is that the patch was created from a different root folder than the module itself, the following will work if you are running the patch from within the module:

patch -p5 < flag_abuse-flag_note-895418_0.patch

sirktree can apply the patch properly on #9 with -p0 because he does have the patch's actual directory structure on his computer.

Just as a guide for creating patches, from http://drupal.org/patch/create:

Check your directory!

Make sure others can use your patch:

• Drupal core patches: Run the diff command in the root Drupal directory where you're making a patch, the same directory that contains the index.php, cron.php, etc. files. Core patches should always be made from Drupal root.
• Contributed module/theme patches: Run the diff command in the module or theme's root directory. Example: /sites/all/modules/foobar.

With these guidelines, others can use your patch, even if they don't have the same directory structure as you.

From what I tested so far, its working. I am using flag_note with flag_abuse properly. I might be missing something but this worth committing and then fix if necessary.

Actually, looking it further the reset flag is still launching the modalframe and it's not properly modified, probably because I might have changed it a bit. In anyway, this module shouldn't alter flag_note form, it's just too prone to inconsistencies.

I will suggest and soon submit another approach.

The reset link should not really be handled by flag_note, so we need a way to tell flag_note not to add the flag_note behavior to the flag.

I am thinking of following the same approach as modalframe API which is to add a flagnote-exclude class to the flag link. I will then submit a patch to the flagnote module (which seems to be quite responsive) to exclude flag links with that flag. Thus any module doing something like yours might benefit from it.

actually, I think what I said on #19 might not be easy/the proper way to go. Will look more into this and report back.

I have been working today as I need both modules nicely working together. I finally worked in the same line as in #7, I did it a bit different.

I left the previous confirm alter the way it was and create a new alter for the flag_note form which is removing/changing the proper elements. Patch on #7 has mistakes on that front.

New additions to flag_note that I submitted today and were committed will help a bit more with the user part of the integration, I suggest trying the latest dev of flag_note when it's there.

Worth noting that I haven't reviewed the extra default views addition of this patch.

I also added one class (flag-abuse-reset) to the link so it can help theming.

Worth noting that #1024678: empty data can be stored on the db on flag_note needs reviewing/accepting to prevent empty data be stored when the modified form of this patch is submitted.

I have now a comment on the views modification of this part. It's adding a relationship on the flag_abuse_views_pre_view(). I don't think this is necessary or appropriate. This relationship remains hidden to the administration of the views. I even didn't realize it was there and I actually add the relationship myself.

I wouldn't add a relationship there, I rather leave that to the flag_note module user to set it up manually on the views. Another option is to add it on the default, but I think this is just too much of an integration. Views should remain configurable to the user as much as possible.

Attached is a patch w/o it.

subscribe

+1

+1

Drupal 6 is no longer officially supported. If you think this issue is still relevant for 8.x, feel free to re-open.