I've found a bug when using flag_note. Here is the basic code for reacting to content being un-flagged:


/**
 * Implementation of hook_form_alter().
 *
 * Deletes notes once a flag is deleted.
 */
function flag_note_form_flag_delete_confirm_alter(&$form, $form_state) {
  $form['#submit'][] = 'flag_note_flag_delete_confirm_submit';
}

/**
 * Additional submit handler added to the flag delete confirmation form.
 */
function flag_note_flag_delete_confirm_submit($form, &$form_state) {
  flag_note_remove_notes(array('fid' => $form_state['values']['fid']));
}

The problem with this code is that it is tied to a specific form rathe than using flag's own API to react to the remove event. This means that when content is unflagged another way (ie. action, flag page etc.) the note itself is not removed.

CommentFileSizeAuthor
#5 unflag.patch1.94 KBgunzip
#2 1029646_remove_notes_when removing_flag.patch817 bytesrickvug

Comments

gunzip’s picture

gunzip commented

this code removes notes once a _flag_ is deleted using the admin interface (not when some content is unflagged)

rickvug’s picture

rickvug commented
Title: Notes are not removed when un-flagging due to flag_note's reliance on modifying the confirmation form » Notes are not always removed when un-flagging. Use hook_flag as a general solution
Status: Active » Needs review
StatusFileSize
new 1029646_remove_notes_when removing_flag.patch817 bytes

@gunzip Ah yes, I see. Please disregard my comment about the code above. The issue I am having still stands however. The attached patch uses hook_flag to remove flag notes when a flag itself is removed. It fixes my problem with using flag_note with flag_page.

This approach should work for many other situations as well. For example, I believe that there already is code that handles removal of flags when deleting a node, user, comment etc. Assuming that hook_flag is called at this time this code will be called. This seams to make sense to me as it would remove much of the special casing for different types of flags. Perhaps the next step would be a review followed by removal of any special cased code that is no longer required.

gunzip’s picture

gunzip commented

looking at the flag module itself the code that "handles removal of flags when deleting a node, user, comment" does not call any hook or action, it just calls a sql delete in its nodeapi_hook so, afaik, hook_flag is not called so the nodepi code in flag note is required and cannot be removed. may you please elaborate more which parts of code are you referring to ?

anyway, i think you made a good point but the patch lacks some checks (permission ?) before being integrated, i'll look at that asap thank you.

rickvug’s picture

rickvug commented

@gunzip - Thank you for the initial review. I will have to look at this in more detail in a bit. Do you have any specific guidance as to what permission checks are required? Is the the unflag operation fired before or after all other checks have passed? Perhaps an updated version of the patch could follow with the permission check and test run of any code removal (if possible).

gunzip’s picture

gunzip commented
StatusFileSize
new unflag.patch1.94 KB

here is another patch that checks if history is enabled and for "delete flag notes" rights.

i'm not sure about this last part because if the user does not have the "delete notes" permission the unflag operation success but the note silently fails to be deleted and still persist in the db.

the alternative is to don't check for flag notes rights at all, assuming that if the user can unflag the content he can remove relative notes as well.

moreover i've deleted the relative code in the flag_note_page() function.

rickvug’s picture

rickvug commented
Title: Notes are not always removed when un-flagging. Use hook_flag as a general solution » React to hook_flag to remove notes when a flag is removed
Status: Needs review » Reviewed & tested by the community

I tested the latest patch and it works as advertised. Notes are kept on deletion if history is enabled but were deleted otherwise. The code is clear. I'd prefer && to "and" for consistency with core but there's no official guideline on this from what I can see.

As well, I can confirm that you are correct about the need for specific code to deal with node and user deletion. Ideally flag should remove flags only through its own API so that modules such as flag_note can react and clean up their own tables.

This is RTBC in my books.

gunzip’s picture

gunzip commented
Status: Reviewed & tested by the community » Fixed
YK85’s picture

YK85 commented

thanks!

Status: Fixed » Closed (fixed)

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