Javascript toggle flag link-type breaks unflagging

same here, removing user from friends only works when using confirmation form method, javascript toggle and normal link have no effect

same here too

This bug is still a problem with flag friend. When changing the flag over to JavaScript toggle a user can no longer remove a friend from the view all friends tab on the user profile.

Also, like stated in the original post ... if a user approves a friend request from the "Friend Request Tab" the Actions change from Approve|Deny to Remove Friend|Deny.

Is there any way we can have this fixed? Screenshots attached.

j

I confirm that using Javascript toggle link type instead of Confirm form breaks the unflagging.

This is very important hence the critical priority.

Does anyone have any leads on the issue?

Currently the "remove friend" link does not work with the ajax "toggle" type links. This patch is a workaround for the issue. It turns the "remove friend" toggle link into a confirm link. This will give you the confirmation form when clicking the link instead of attempting to unflag via ajax.

Regarding the patch from #7, is there a reason why we can't just support javascript links?

Here's my first attempt at it. This works with all three types of toggles. I added a bit of unfriending inside hook_flag() that seemed to do the trick. What do you think?

Maybe we need to change the version?

One more test..

Patch #14 doesn't resolve the problem. I can't delete friends from my list

Strange.. it fixed the problem for me.

Updating the title.

I won't be able to crack this one tonight, but I did confirm the issue with manual testing.

Also I tested the #14 patch, and it still doesn't work. I'll have to run this through a debugger tomorrow night to figure out why.

The problem seems to be that the link provides the action as "unfriend", and flag.module's flag_flag::user_access() does not understand it.

The issue seems to be with link on line 355 of flag_friend.module:
$link = str_replace(t('Approve'), t('Deny'), str_replace('unflag', 'unfriend', $flag->theme('unflag', $uid)));

This creates a link that looks like /flag/unfriend/friend/51 to match flag module's implementation of hook menu which looks for flag/%/%flag/%. So in our case, arg 1 is unfriend which is then passed into the access callback function and we get the error because the access callback, user_access, doesn't recognize unfriend as an action.

I changed line 355 to $link = str_replace(t('Approve'), t('Deny'), $flag->theme('unflag', $uid)); and the error disappeared, however, the database isn't being updated. I'm assuming it is because somewhere in flag_friend.module, it is grabbing the argument from the url and looking for unfriend....

Was hoping someone might have an idea as I haven't been able to find that yet. I don't really understand what the flag_page function does besides error checking...

This was bugging me so I kept working on it. Looks like there are two issues:

1. flag module invokes hook_flag_unflag after confirming there is an entry in flag_content for the entity_id being unflagged. Flag friend only uses that table to identify pending friendships. Once a friend has been confirm, the rows from that table referring to the flagged friendship are deleted and added to flag_friend table.

2. there is a unflag function in flag_friend. However, it is only called when the unflag friend form is submitted. This doesn't happen with javascript links. Additionally, with the solution above to the user_access error, the flag_friend_unfriend function doesn't work because event is no longer 'unfriend'

Not entirely sure how to address these issues without some significant changes to flag_friend... in particular, it seems like the tables for flag_friend and flag_content are duplicative and it might make more sense to use the flag_content table to hold the friendship information (as it is only 3 columns) and have a flag_friend_pending table for when relationships haven't been accepted?

Not entirely sure how to address these issues without some significant changes to flag_friend...

That's pretty much where I got stuck when I was poking at this issue weeks ago.

in particular, it seems like the tables for flag_friend and flag_content are duplicative and it might make more sense to use the flag_content table to hold the friendship information (as it is only 3 columns) and have a flag_friend_pending table for when relationships haven't been accepted?

Actually, I was thinking along the same lines. It just seemed bizarre to me that the module removes the flag after accepting friendship. Creating a flag_friend_pending table would work for the immediate future. Eventually, I want to use fieldable flags for that purpose, if practical.

Great to hear we're thinking along the same lines. I actually went ahead and made some changes to make it workable for a site I'm working on, though I had to hack flag module to change where the module_invoke call was made for unflagging.

I dont mind taking a stab at a patch to make the changes mentioned above (i.e., using the flag_content table and having a flag_friend_pending table -- won't include the flag module hack of course) if that works for you?

maybe a dumb question, but what do you mean by fieldable flags?

I dont mind taking a stab at a patch to make the changes mentioned above (i.e., using the flag_content table and having a flag_friend_pending table -- won't include the flag module hack of course) if that works for you?

I'd love to have a crack at it myself, but you'd probably finish it before I will, so have at it! ^_^

maybe a dumb question, but what do you mean by fieldable flags?

This is a Flag 3.x thing. Flags actually can have fields in the same way that nodes have fields. The idea is to make flags more versatile. This also eliminates the need modules like flag_abuse. My eventual plan is to:

• Fix major bugs in the "Alpha" version and post a 1.0.
• Create a 2.x branch that uses Flag 3.x.
• Leverage fieldable flags in Flag_friend 3.x to create G+like "circles" or friend groups.

I didn't realize flags could have fields -- interesting... I'm actually working on a flag application module (you can approve / deny flags) and would be interesting to integrate with that...

as for the updates, it's obviously a huge job and i really wanted to avoid one massive patch so here is the beginning of the work. This makes the module better integrated with flag module and does the flagging / unflagging all within hook_flag. The patch should now allow people to flag / unflag with javascript turned on but it still needs a lot of work, specifically:

1. I didn't test non-javascript friend flags and they will likely be broken
2. The views integration doesn't work entirely -- this still needs work
3. Still need to create an upgrade path as I changed the flag_friend table to have a status column and i changed friend_uid to content_id to match the flag_content table (this could probably be changed to use fid instead of uid / content_id but i was tired...)
4. I didn't test the rules integration or look at the messages but the links seem to still work (outside of views...)

pete

sorry - was trying git format-patch and obviously screwed it up... here's the patch (just using git diff)

saw that failure coming, sorry for not changing the status... will address it with the next one and the larger issues still outstanding.

No problem. I'm *really* looking forward to this patch. It makes the entire module make much more sense.

My mistake - my patch does not work.

ok, this is going to error out because i didn't get to the views stuff but i think the module works for the most part - i really didn't touch the message parts so can't confirm that part is working 100%.

However, I did test the javascript friend and unfriend, non javascript and confirm form. everything seems to be working there but need the community to test it out.

I did not get to the views stuff. I'll be away for the next 10 days so thought it best to at least get this out in the community for people to test. happy to keep working on this when i get back.

pete

Slight update to the patch, removing some whitepace issues.

We're also missing an update hook. I'm poking at that right now. It appears we'll have to:

1. Rename flag_friend to something like flag_friend_old
2. Create a new table called flag_friend that matches hook_schema().
3. Loop through each row of flag_friend old.
   • Reflag the friended user if the flag is not present.
   • Insert a new row into the flag_friend table according to status.

It might be complicated enough to farm this out to Migrate.module just to keep the code simple.

yeah - i should have mentioned that, the update will be a little tough because we need to go through each existing flag in the old flag_friend schema and recreate two flags in the flag_content table (1 flag for each user). However, flag_content has the unique fcid so we would probably have to call a function from flag to properly create the flags. didn't get that far yet... back now so happy to keep working on this with you.

I have most of the update code written and sitting in my local repo, but I haven't had time to test it yet. I can try to put out a patch this evening with the changes.

So this will fail when testbot gets around to it. It does have some update code in it that I think works.

Basically, it renames the old friend table, creates a new one with the right schema. Then it takes any outstanding friend flaggings and stores them in the table too. Next, it wipes out all the flags, and starts re-flagging everyone based on the merged table. The rest of the module code should figure out the appropriate relationship when the flaggings are replayed.

Problem: It still lists everyone as pending. I'm suspect that's because we rely on rules to update the pending relationships to "both" relationships. Not sure yet what we have to do to fix that.

Okay, so the statuses stuck at pending were my fault. I didn't realize that each row in the flag_friend table was a bi-directional relationship.

This patch contains working and tested module update code.

Next task is to sort out the views stuff.

If you're going to weight the module in hook_enable(), you should also do so in an update hook for existing installations.

If you're going to weight the module in hook_enable(), you should also do so in an update hook for existing installations.

Good point. I'll add that tonight.

Adds update hook to change the order of module.

This sounds awesome, can't wait to test it out this weekend. With this done, seems like the next major piece is Views integration which I started to take a look at before. I'll keep working on it and post what I come up with. Great to see this coming together!

On another note, since we know that the javascript doesnt work for the current release, should that be mentioned on the project page adn that it is under development? Sorry to detract from the actual issue at hand.

On another note, since we know that the javascript doesnt work for the current release, should that be mentioned on the project page adn that it is under development? Sorry to detract from the actual issue at hand.

Technically, the only release for 7.x is clearly labeled as "alpha". Still, that doesn't keep people from using it.

The mega patch continues... This incorporates the work on views which I briefly tested but definitely needs review by the community. In short, the integration with views is now largely reliant on the flag module (you'll notice it in the default views that were updated). You can still use flag_friend to only show content created by friends but it may be a little confusing for some users. I'd be happy to write the documentation on that later...

Next task is updating the messages for non-java links, though I dont think it'll be too tough.

Was looking through the tests here to see where the patch failed... realizing that some of the tests will need to be rewritten (e.g., I removed the rewritting of the views link text to stick with what is defined by the friend flag but the test looks for "approve" or "deny")

I can try to look that over tonight. The power was out here over the weekend so I'm a little frazzled*.

*Minnesotan understatement.

hopefully everything is ok. i'm around for any updates/fixes until thursday but then out of commission until the following week, likely tuesday or wednesday. also - i thought i remember reading something about the spaces in the patch somewhere on this thread but couldn't seem to find it, so let me know if they are off.

Okay, the output of flag_friend_get_friends_query() was really bothering me. I've removed the array within array, and modified other functions accordingly. This actually cut out a few failed tests and errors.

Also I dealt with the whitespace issues.

Just loaded the patch and installed a clean site.

1. No matter what, none of the tabs under the "Friends" tab on my profile show any users at all.

2. As user 2, I'm able to access user 1's "Friends" tab. Was that always allowed?

3. It does seem like the "remove" Javascript link works now, but I can't fully test until #1 is addressed.

@socketwench - yeah, that return was a weird one, can't think of why I would have done that... As for the whitespace issues - maybe a dumb question but what exactly did you fix - i want to make sure i dont keep giving you patches with the same problem.

@mstef - Thanks for the feedback and testing!! Re: the issues:

1 -can you confirm that you have friends in the database (flag_friend table)?

2 - yeah, that's an interesting issue... two different things to consider here:
a) I believe the way the view is set up, you aren't actually seeing their friends, you are seeing your friends despite being on their tab. The argument isn't used because the relationship is the one that actually defines who flagged the item (the current logged in user or any user)... I'll have to take a look at this again tonight...
b) accessing the user's friend tab (I believe) was always available. I think that should be the case as administrators should determine if they want them open. To make them inaccessible, administrators could then validate the argument (i.e., a logged in user is looking at their profile, return TRUE else return FALSE). In this case, you would still see the table but get whatever the administrator defines as a return value in Views. OR, we'd need a custom menu callback function to check access permissions. If that returned false, the tab wouldn't be rendered. This could probably be configured in with the flag configuration but i think it would be something that could be considered as a separate issue once we have a stable release of the module with the patch applied.

3) Pending issue 1, can you check the database to see if the flag_content and flag_friend tables are being updated?

Yes, there is an entry in the flag_friend table:

I just started over and tested this:

1. Uid 1 goes to profile of Uid 2 and clicks "Add friend". [good]
2. The text changes to: "Friend requested. Cancel?" [good]
3. As Uid 1, I go to my "Friends" tab and I see the pending request. [good]
4. As Uid 2, I go to my "Friends" tab and everything is empty. [bad]
5. As Uid 2, I go to the profile of Uid 1 and see the approve link. [good]

Since I can only approve the request by going to the user's profile, I went ahead and did that as Uid 2.

Now..

1. As Uid 2, "View all friends" shows Uid 1. [good]
2. As Uid 2, "Pending Friends" shows Uid 1. [bad]
3. As Uid 2, "Friend Requests" shows Uid 1. [bad]

I didn't make any changes to the user/%/friends view.

This is awesome, thanks for the detailed feedback. I'll check it out tonight and confirm whats going on.

@yaworsk Mostly I just looked through the patch in dreditor, found any red blocks at the end of a line, then deleted them in the code. When I was finished doing that, I reapplied the patch to check if git found any whitespace errors. It's a minor thing, but it's part of the coding standards.

@socketwench, thanks for the heads up. I definitely appreciate the standards and wanted to stop causing the issue. Turned on whitespace indicator in the editor for now and saw a few of them.

@mstef, ignore my previous comment about permissions. i was way off - didn't think about a random user being able to access another users pending / received requests and being able to access the link.

Attached is a new patch (@socketwench, I applied your patch but it looks like it was missing the creation of the 2 views handler files so I recreated them). In this patch, I've added a new field handler for the friends links - it is largely reliant on what existed before I deleted the previous one. I've also added an access plugin - this was the only way I could come up with restricting access so only profile owners can see their pending / received friend requests. It's a simple access check to see if the UID of the logged in user matches the path argument (arg(1)). This also hides the tab if the check returns false which is a bonus over views argument validation and keeps php code out of the view.

Happy to hear your thoughts and tests - it's getting late and I wont be able to test extensively until tomorrow night.

Does that patch address the problems that I outlined?

4. As Uid 2, I go to my "Friends" tab and everything is empty. [bad]
- you're friends "All friends" tab will be empty, friend requests will show that UID 1 has asked to be your friend and you can confirm

2. As Uid 2, "Pending Friends" shows Uid 1. [bad]
- I believe this is addressed

3. As Uid 2, "Friend Requests" shows Uid 1. [bad]
- This should be addressed

I tested late last night but I believe all was working - Sorry for not clarifying that the patch addresses the issues you raised. Just wanted to mention, if you accept the friend request on "Friend Requests" tab - the friend won't be removed from that table until a page reload if javascript is enabled...

+++ b/flag_friend.installundefined
@@ -13,7 +13,22 @@ function flag_friend_install() {
+  db_update('system')
+  ->fields(array('weight' => -10))
+  ->condition('type', 'module')
+  ->condition('name', 'flag_friend')

This actually is causing some problems with themeing the flags. When flag_friend runs before the flag module, there's no $account->content['flags'] for flag_friend_user_view() to theme. Thus, flag_friend_create_link() is never run, and we never get a Deny link. We still get an approval link since flag_friend_preprocess_flag() runs.

Do we really need to run flag_friend before flag? It seems bound to cause more wierdness like this.

EDIT: Universe only knows why the tests looking for the Deny link still fail. I haven't figured that out yet.

1. "Confirm Friend" link on the "Friend Requests" goes to the confirmation form page, even though JS was selected as the option.

So far everything else seems like it's working..

Does anyone else get confused with the two sub-tabs of "Pending Friends" and "Friend Requests". I get the difference... but I often look to the pending tab when I want to approve a friendship..

Also.. did you make the change to hide the friends tab from other users, because it's still accessible.

Third times a charm.... Here's what was fixed from the previous. Unfortunately I don't have time to test because I'm headed out for the long weekend here in Canada.

1. The confirmation page was coming up because i tried to get cute and tried to make a link myself instead of just using the flag module. That should be fixed now.

2. I've updated all the default friend views so none should be accessible to anyone but the profile owner (determined by the argument passed in by the path, ie, $user->uid == arg(1)).

@socketwench -- I can't remember why we needed that but it was in response to an issue I was encountering. Of course I wasn't specific in my comment as to what that issue was... I'll have to look at it when I'm back. Sorry.

@mstef -- I agree, it can be a little confusing. I modeled the default views after the previous and I think there was 3. Additionally, I'd have to checked if that could be changed in the default set up but I think that's something we can do later once we have a stable patch/release.

Nice work!

1) There's no link to deny a friend request. Should there be? Was there before?

Other than that it looks great to me. Everything seems to be working.

Would be great if someone else could make a run through it as well.

Ha! Never considered/noticed that, thought everyone generally just wants to be friends.... Ill update that when I'm back. I'll also do a full walk through, tough after just having written the code, don't always consider / notice things without a fresh perspective

Thanks for all the feedback / help

@yaworsk That's actually the specific issue caused by the module weight I mentioned in #61. Changing the weight back to 0 makes the Deny link reappear.

Changing the module weight back to 0 doesn't work for me.

i plan on checking this out tomorrow / on the weekend. also need to figure out why i changed the module weight. i think it was related to unflagging but have to look at it, dont remember off the top of my head.

ok, looked into this. Didn't find the reason why I changed the module weight but that's beside the point right now.

I was able to create the deny link without a problem, however, did attach a patch as it doesn't do anything which is a bit of a problem. Long story short, there is no reciprocal flag when a user has been flagged as a friend, so the approve link works because it essentially allows a user (User B) to flag the original user (User A) as their friend. However, if user B wants to deny the friend request, the flag link will return false from the flag module because the action is 'unflag' but no flag exists, user B has never flagged user A so there is no unflag action (i know this is probably confusing). This is a problem because hook_flag isn't invoked until after the check to see if content (in this case, the user) is flagged:

if ($action == 'unflag') {
      if ($this->uses_anonymous_cookies()) {
        $this->_unflag_anonymous($content_id);
      }
      if ($flagged) {
        $fcid = $this->_unflag($content_id, $uid, $sid);
        module_invoke_all('flag', 'unflag', $this, $content_id, $account, $fcid);
      }
    }

This was the original issue we were running into when we started this rewrite. I'm not sure what the solution is here... no simple fix that I can see but I've been looking at this for a while. Open to suggestions.

What if we assume that friendships may *not* be reciprocal? We create a new status, FLAG_FRIEND_CONFIRMED, or something like that, and make that the ultimate determination if a friendship is approved. I wanted to remove that limitation anyways.

I dont know if I follow you. So, right now, a user flags another user and one flag is created in flag_content (content_id is the user who was flagged, UID is the actual flagger). In the flag_friend table, we create an entry for that flag (linked by fcid) with a status of 4 or something, whatever, flagged but not approved relationship is.

once that user who was flagged also flags the flagger as a friend (or in our case, they "approve" the relationship), a new flag is created in flag_content reversing the content_id and UID (so now there are 2 flags) and the flag_friend table entry is updated to relationship confirmed (its 2 or something).

When you say, we assume a friendship may not be reciprocal, do you mean, the flagged friend doesn't have to approve / deny the request? so like a twitter style following?

Was just thinking more about this... what if we called hook_menu_alter and changed the function call when a flag link is clicked? We create a function in flag_friend that looks to see if we are talking about a deny link and if so, we call our own function to act on the delete the 1 existing flag and if not, we pass off the menu call to the original flag function call.

This still allows us to use flag module to create the links but for us to interject in the process. For those wondering, the reason we can't just unflag the existing flag is because the person denying the request, isn't the owner of the actual friend flag that has been created. So if we tried to delete the 1 flag, we get a permission denied message from the Flag module but the user wouldnt' have permission to delete someone else's flag.

Here is the menu code from flag i am talking about overriding. we would just change the page call back to our own page call back:

  $items['flag/%/%flag/%'] = array(
    'title' => 'Flag', 
    'page callback' => 'flag_page', 
    'page arguments' => array(1, 2, 3), 
    'access callback' => 'user_access', 
    'access arguments' => array('access content'), 
    'file' => 'includes/flag.pages.inc', 
    'type' => MENU_CALLBACK,
  );

What I'm suggesting is that remove the need for the user who was flagged to flag the flagger to approve the relationship. Instead, we have the approve link go to somewhere in hook_menu() and then we set the status in {flag_friend} to mark the relationship as approved.

hmm. I'd never thought about that. so essentially we'd be using hook_menu_alter to change the function call back on an approve link? otherwise we'd need to almost duplicate the code from flag module regarding creating links, etc. since we have the patch all but done for the JS original JS issue (except for the deny link), should we fix the deny issue, potentially with teh hook_menu_alter solution I mentioned above, commit that and then look at possibly changing up the system - it could coincide with changes that are needed for a stable flag 3.x release?

ok, here's a new patch which should have the deny link working properly. Hoping someone can test it - I did on both the user account page and the views listing page but only with js enabled.

I know you can just look at the patch but thought it easier to include some notes on what I did in this case:
1. overrode the default flag module function via hook_menu_alter to call our own. We check to see if it was a deny link that was clicked and if so we call our own function otherwise we just hand it off to the flag module like normal

2. I passed in the token to hook_flag, it's default set to NULL in all other circumstances.

3. I copied some code from the flag module to handle the actual JS click action. However, there is error checking with that but in our case, it happens after the db queries are called to delete the other user's flag. Not sure this is best. Happy to hear thoughts.

4. I removed the weight change for flag_friend.module in the install file. It was in deed causing issues creating the deny link on the user account page.

5. updated the views link handler to add the deny link.

Hopefully this is the last of it and we have a stable patch to be applied.

1) Looks like the "Deny" link using the confirmation form does not work.

2) I get this error when clicking "Deny" link using the normal link: "Notice: Undefined variable: action in flag_friend_flag() (line 175 of /var/www/d7/sites/all/modules/contrib/flag_friend/flag_friend.module).".

3) Fatal error: Call to undefined function dsm() in /var/www/d7/sites/all/modules/contrib/flag_friend/flag_friend.module on line 500.

really, i left a dsm in there? i definitely thought i did a search for that.... will check it out. Thanks for testing.

Did you test out the JS side of it?

Yes -- I only came across the issues I reported. All else seemed fine to me.

Great, thanks for all the help testing! Updated the patch to address the undeclared variable. Noticed a bigger issue actually passing in the token and addressed that too (I was actually just passing in TRUE, not the token value). Removed the dsm.

tested this again friends using js and non js and all worked ok for me. didn't test the confirmation message setting as we agreed not to address that yet.

I can't find any issues -- nice work!

Hello and thanks for all the efforts. I wonder if the patches posted here are compatible with the latest patch mentioned in this issue: #1866040: Flag 7.x-3.x compatibility

@mstef - great news, thanks for all the work testing the patches!

@Sinan, no, the patches won't be compatible. I haven't looked at the other issue in depth but know that Flag 3.x introduced some table changes. There will need to be a new version of flag friend compatible with the 3.x version of flag. However, the goal here was to get flag friend stable with 2.x before working on integrating it with flag 3.x, as when we started, flag 3.x wasn't stable.

:...(

Thanks for the explanation. Hope we can also see the fix for Flag 3.x soon.

Would speed things up if you could also review this patch to confirm whether there are any bugs. Once we can get it reviewed / tested and applied, I'm happy to look at getting a stable version to integrate with flag 3.x. i dont think it will take a lot of work but that is just based on a preliminary look at the new flag module and assuming there were no flag api changes.

I haven't looked at the other issue in depth but know that Flag 3.x introduced some table changes.

Not just that, API changes as well. Ideally, I would like to make Flag_Friend use only APIs and leave Flag's tables alone.

That's good to know about the API. Completely agree regarding Flag Tables, would make maintenance a lot easier. Had to go beyond the api in some parts of this patch for a couple reasons but with more time, i think that could be addressed.

Well... it would be great if the code in the patch would follow the Drupal coding standards, it is quite hard to read at the moment.

• comments missing whitespace, not having a period at the end of the sentence, longer than the 80 char line limit, missing capitalization
• missing doxygen, missing parameter description in doxygen for non trivial parameters, "* Implements hook_views_access" should be "* Implements hook_views_access()."
• the else in the if-else code should start on a new line
• indentation issues in various places
• extra whitespaces at the line endings or in the function header

The doxygen is off in some places, for example:

/**
  * Record friend status based on existing flags.
  */

which should be

/**
 * Record friend status based on existing flags.
 */

Also there are other possible issues.

• The flag_friend table is missing a primary key, the code should define it and not the DBMS.
• the update hooks are not protected enough, if for some reason someone needs to run them multiple times, then they will experience lots of pain
• Assume the worst, if there are tons of data that needs to be moved in the update hook, it is best to process them in batch to avoid time outs
• flag_friend_page doesnt handle when the token is not provided in $_REQUEST, it will throw notice if a user starts typing in urls
• flag_friend_flag needs to be better written to avoid duplicate data written into the DB, or erroring out due to duplicate data (race conditions)
• copy paste issue - $fcid2 = flag_friend_get_flags($flag, $content_id, $account->uid, $reset = NULL);
• this just does not feel right : $link = str_replace(t('Approve'), t('Deny'), $flag->theme('unflag', $uid));

So we have yet another problem: The automated test case at line 125 fails. At first, I thought it must have been pretty simple, but after a few days of trying to wrap my head around it I finally figured out we're hitting another design problem:

1. User A flags user C as a friend.
2. User C denies the friendship request.
3. Drupal serves up Flag module's confirmation form, flag_friend alters it cosmetically.
4. User C clicks "confirm". This kicks off the following process:
   1. flag_confirm_submit()
   2. flag('unflag', 'friend', $uid_a);
   3. flag_flag->flag('unflag', 'friend', $uid_a, NULL)

The problem is, that that last call *should* be flag_flag->flag('unflag', 'friend', $uid_c, $uid_a) because it's user C that was flagged, not A. Altering the confirmation form from Flag does not work and cannot work for this case because we can't force Flag to switch around the two remaining parameters. When friend flags were mutual, this was fine as hook_flag() took care of everything afterward.

This leaves us two options on how to move forward:

1. Implement hook_menu() and have Flag_friend handle denial form ourselves. We can then invoke flag() programmatically.
2. Remove the confirmation step entirely, perhaps replacing it with an alternate "block this user" mechanic.

Option 1 would be easier, but option 2 is where I would really want to go. I'm unsure if we'd need to implement an entirely new flag for blocks, or just add a "FLAG_FRIEND_BLOCKED" status to our existing friend flag.

I addressed some of the issues that nagba mentioned. Compared to #82:

• Various coding-style fixes
• Added primary key to the flag_friend table
• Converted the flag_friend_update_7005() to use batch processing, to make sure it doesn't fail if we have a lot of users.

Sorry for my disappearance...

@nagba, sorry. Not following the coding standards was definitely my fault. I'll be sure to in any follow up work on this. Regarding the other issues, seems like Coornail addressed some. I'm happy to check out the rest.

@socketwench, yeah, that's the problem with the confirmation... we did address it with the javascript part by using hook_menu_alter and then adding our own function call if it is a friend flag. Is the issue you are identifying separate from there? I completely agree, teh best way to do this isn't through hook_menu_alter but at the time, just wasn't sure how else to do it and thought it best to get a working version going. Then the next step would be to look at a total overhaul to do things properly and incorporate the flag fields change that was also mentioned earlier.

Also, i'm thinking that we are starting to get away from the original javascript doesn't work description... I think that issue has been resolved, though maybe not in the best fashion. And with the identification of the additional issues in the recent posts, I'm wondering what the best way is to organize the work on this (i.e., apply this patch, close this thread and open separate issues for those identified above)? Just throwing it out there.

Managed to get all the User tests to pass, finally. Also refactored flag_friend_flag() and added some documentation.

@@ -547,14 +725,15 @@ function flag_friend_form_submit($form, &$form_state) {
     }
     flag_friend_message_email($status, $flag, $content_id, $user);
   }
-  // the only time we want to unfriend, is if $status = FLAGGED or APPROVAL
-  elseif ($status === FLAG_FRIEND_FLAGGED || $status === FLAG_FRIEND_APPROVAL) {
-    flag_friend_unfriend($action, $flag, $content_id, $user, $status);
-  }
   elseif ($status === FLAG_FRIEND_PENDING) {
     // Clean message when user cancel own request.
     flag_friend_message($action, $flag, $content_id, $user->uid);
   }
+  elseif ($status === FLAG_FRIEND_APPROVAL) {
+    $flaggingUser = user_load($content_id);
+    flag($action, $flag->name, $user->uid, $flaggingUser);
+  }
+
   //flag.module doesn't understand 'unfriend'
   $form_state['values']['action'] = 'unflag';
 }

This is the key part. After poking at the code for the better part of two weeks, it occurred to me that we could add an additional case in flag_friend_form_submit() for FLAG_FRIEND_APPROVAL to handle the failing friendship denial case. We invoke flag(), causing the original flagging to be removed as if done by the original user that created the flagging.

The original flag_form_submit() handler still gets called, unfortunately. I haven't looking into how or if we should remove that.

Managed to get the tests to pass locally. It seemed we had some of the cases reversed by mistake.

Okay... Maybe a bad patch? *tries again*

Correction in *.info to specify the correct Flag version.

One more time...

Applied a patch from https://drupal.org/node/2073853 to specify the correct Flag version in the info file. Hopefully that'll appease Testbot.

Reroll...

#99: 1433122.99.jsUnflaggingMajor_DenyLinkFix.patch queued for re-testing.

Okay, I've been bashing away at this patch trying to get it to go green.

*crosses fingers*

awesome! great to finally see green. what can I do / how do you want to proceed?

To put it simply:

We need manual confirmation that the patch works as it should. Then we can cut a new beta release. ^_^

Can't find any issues after a quick run-through of scenarios with all toggle types. I didn't look at the code or patch itself though.

khm...

+files[] = includes/flag_friend_handler_argument_numeric.inc¬
+files[] = includes/flag_friend_handler_field_links.inc¬

i see other issues as well, i'll try to make either a list of them or a fixed up patch

@mstef Thank you!

@nagba I kinda figured there'd be some lingering issues. Looking forward to your patch.

Found an issue, just haven't had the time to fix it - I think this was also why I had changed the module weight. The hook_user_delete function doesn't work properly and so flags aren't removed flag_friend table properly.

Right now, as written, we try to grab the fcids from flag_content but those won't exist because flag module implements hook_user_delete first and removes them.

I can update the patch or wait for nagba's patch

go for it, i'll just make a diff

I *really* don't want to change the module weight if we can avoid it. It already caused the links to render incorrectly.

I'm inclined to either implement hook_module_implements_alter(), or implement a cron task to clean up the table later. When we update the module to Flag 3.x, the status data will be stored with the flagging. I'm willing to accept a slightly hacky solution for Flag 2.x that'll be removed later.

EDIT: Another option would be to just store the uids in the flag_friend table. That way, we can delete the database entry without needing the flagging. Again. I'd much rather do this since the flag_friend table will probably just go away with the Flag 3.x update anyways.

Changes:

• Re-added the content_id and uid columns to {flag_friend}.
• Changed the update hooks accordingly.
• Removed the update hook that changes the module weight.
• Changed many -- but not all, see below -- of the queries to flag_content to flag_friend.
• Removed duplicate views handler files as per #112.

Many of the queries we make to {flag_content} are to match up a content_id and/or uid with an fcid (flagging_id). I'd like to reduce or eliminate those queries where I can, since I really don't want flag_friend to muck about in another module's table. Admittedly, that may be unrealistic given the state of the Flag 2.x API. There are a few cases where that doesn't work (flag_friend_get_fcid()), or should but causes test breakage (flag_friend_get_flags()).

PS: I also initially misspelled 'duplicate' above as 'druplicate'. >_<

Looks like a bug in the patch on line 1836. In the views handler flag_friend_views_handler_field_status.inc, the class is incorrect. It should be:

+ class flag_friend_views_handler_field_status extends views_handler_field {

Any idea when all this will be integrated and a new release out? I really need a stable version ASAP.

Thanks,
David

Maybe I'm missing something, but flag_friend_flag_flag() never seems to check the token validity, and flag_friend_flag_unflag() deletes data before checking it.

It seems like the token check should be in flag_friend_flag() or maybe in an access callback?

Also, the JSON code doesn't seem to use the preferred Drupal 7 API:
https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...

Call or return that so the result of the page callback is NULL, rather than directly invoking drupal_exit().

I have a few recommendations / bug fixes for this patch:

1. As reported already, the status field handler has the wrong class:

class flag_friend_views_handler_field_status extends views_handler_field {

2. The status field handler should have options for both pending approvals & requests:

in option_definition():
$options['pending_approval'] = array('default' => 'Pending Approval');
$options['pending_requests'] = array('default' => 'Pending Requests');

in options_form():
$form['pending_approval'] = array (
'#type' => 'textfield',
'#title' => 'Pending approval name',
'#default_value' => $this->options['pending_approval'],
);
$form['pending_requests'] = array (
'#type' => 'textfield',
'#title' => 'Pending requests name',
'#default_value' => $this->options['pending_requests'],
);

3. The rendering for the status field handler is not correct. It should pull the friend status based on content_id and user_id:

function render($values) {
global $user;
$flag = flag_get_flag('friend');
$content_id = $values->uid;
$status = flag_friend_determine_friend_status($flag, $content_id, $user->uid);

switch ($status) {
case '0' :
case '1' :
return check_plain($this->options['friends']);
break;
case '2' :
return check_plain($this->options['not_friends']);
break;
case '3' :
return check_plain($this->options['pending_approval']);
break;
case '4' :
return check_plain($this->options['pending_requests']);
break;
}
}
}

4. The status filter handler should allow you to filter by Friends, Not Friends, or Pending Friends as follows:

function value_form(&$form, &$form_state) {
parent::value_form($form, $form_state);
$form['value'] = array(
'#type' => 'radios',
'#title' => t('Relationship Status'),
'#options' => array(
'1' => t('Friends'),
'2' => t('Not Friends'),
'4' => t('Pending Friends'),
),
'#default_value' => !empty($this->value['type']) ? $this->value['type'] : 1,
);
}

5. The 'friend_link' handler should also include a filter by current user:

$data['flag_friend']['friend_link'] = array(
'title' => t('Friend Link'),
'help' => t('Flag Friend Link. Only used for creating links for users who have been flagged as by a friend'),
'field' => array(
'handler' => 'flag_friend_views_handler_field_links',
),
'filter' => array(
'handler' => 'views_handler_filter_user_current',
),
);

6. The link field handler is incorrect. It's not consistent with the relationship between the content_id and current user_id. Here is my recommendation for the correct implementation:

class flag_friend_views_handler_field_links extends flag_handler_field_ops
{
function render($values)
{
global $user;
$flag = flag_get_flag('friend');
$content_id = $values->uid;
// what's the status?
$status = flag_friend_determine_friend_status($flag, $content_id, $user->uid);

$link = NULL;
if ($status == FLAG_FRIEND_APPROVAL) // flagged, waiting for your approval
{
$link = $flag->theme('flag', $content_id);
$link .= str_replace(t('Approve'), t('Deny'), $flag->theme('unflag', $content_id));
}
elseif ($status == FLAG_FRIEND_PENDING) // flagged, waiting for their approval
{
$link = $flag->theme('unflag', $content_id);
}
elseif ($status == FLAG_FRIEND_UNFLAGGED) // not friends
{
$link = $flag->theme('flag', $content_id);
}
elseif ($status == FLAG_FRIEND_FLAGGED || $status == FLAG_FRIEND_BOTH) // friends
{
$link = $flag->theme('unflag', $content_id);
}
return $link;
}
}

Thanks dczaretsky!

New patch contains changes as per #120.

It seems like the token check should be in flag_friend_flag() or maybe in an access callback?

@pwolanin I can't actually figure out where the token parameter came from. It's certainly not in the non-patched version of flag_friend. Furthermore, Flag 2.x doesn't even have the token parameter in the API. Where did this code come from?

checked for more issues

Fixed various coding style issues (comments, indentation, extra whitespaces, etc.)

I ran into the following issue:

1. User A flags user B as a friend.
2. User B denies the friendship request and the 'Deny' link changes to "Add friend".
3. User B clicks on the 'Add friend' link that just showed up -> we get a dialog with an error message (and the link points to user B itself, which is wrong).

Looks like the problem was in flag_friend_flag_unflag().

-      print drupal_json_encode(flag_build_javascript_info($flag, $content_id));
+      print drupal_json_encode(flag_build_javascript_info($flag, $account->uid));

Attached patch that addresses this issue.

124: 1433122.124.jsUnflaggingMajor_DenyLinkFix.patch queued for re-testing.

what flag version and flag friend are you guys using i am having some serious issue after using this patch all my other flags have continuous ajax loading when a unflag actions happens after applying my only working flag is flag friend even on a new install of d7.. Note normal unflag links has a white background

Good day drupalers ,

Newbie and maybe off topic question, but
Does the patch #123 contains all previous commits or shall i apply every patch in the thread ? :)

Thank you !

-arrived here from yaworsk's great youtube channel-

@sol0matrix80 You need to be using the git version of flag_friend, and Flag 7.x-2.1. Currently, no version of flag_friend functions with Flag 3.x. Also, please don't use this patch on a production website. We're still sorting things out and different versions of the patch are NOT interoperable.

@RamezAshraf Typically, you only need to apply the last patch in the thread.

Sorry for going MIA - seen a lot has happened since then - just confirming, are we still testing away? I can take a look at the patch in #127 to test it

@yaworsk I was hoping I'd get some confirmation that at least #123 worked, but #124 showed up shortly thereafter and I haven't a clue why that broke it. I've also been busy porting Flag to Drupal 8 so my attention has lapsed here.

124: 1433122.124.jsUnflaggingMajor_DenyLinkFix.patch queued for re-testing.

Apparently the patch at #124 *does* work, but testbot is failing on D.O because it's pulling down the wrong Flag version (3.2).

Reopening https://drupal.org/node/2078309

So here's the deal. Testbot does have a bug that prevents this patch from going green on anything but the most current Flag version. While the infra team is working on it, I decided to port the current patch to Flag 3.x.

Since I never want to see this sprawling issue again, I'm probably going to commit the changes as soon as this upload goes green.

Committed to DEV: http://drupalcode.org/project/flag_friend.git/commit/48d0c99

Please do not reopen! Please submit new issues instead! Thank you!!!

@socketwench thanks ill retest the new patch with your recommendations