Added remove friend #298

ThatAlexanderA · 2018-06-04T14:27:02Z

Added remove friend url to the utils.py
Added removeFriend def to the client.py

madsmtm

I've made a few comments, and I fully understand if it seems confusing, the current system that _post and _get uses is super confusing, so feel free to ask me anything if you need clarification. Thanks for your contribution ;)

madsmtm · 2018-06-04T15:17:24Z

fbchat/client.py

-
+    """ 
+    def removeFriend(self, friend_id=None):
+        """param friend_id: The id of the friend that you want to remove"""


Perhaps you could add a short description of the method, along with a documentation of the return value?

Yes, I will add it soon.

madsmtm · 2018-06-04T15:35:38Z

fbchat/client.py

+            "confirm": "Confirm",
+        }
+        r = self._post(self.req_url.REMOVE_FRIEND,payload)
+        return r.ok


If r.ok is not true, an exception could have been thrown by using fix_request=False in Client._post. Maybe the return value should be used for detecting whether the removal of the person actually succeeded? I tested four scenarios:
If the client isn't logged in, it's redirected to the login page
If the client is logged in, but hasn't been active in a while, it's presented with an error message (No redirect)
If the operation was successful, the client is redirected to /removefriend.php?friend_id=[friend_id]&removed&__req=e&_rdr
And if the friend_id was invalid, it's redirected to /removefriend.php?friend_id=[friend_id]&removed&err=1431010&__req=e&_rdr

So, I think we should update _post to support the parameter allow_redirects, just like _cleanGet does, then use that, look at r['Location'], and determine whether the redirect url starts with /removefriend. If it does, but contains the err parameter, we can either throw an error, or just return False.

I allowed redirects in the _post, but I can't get the location, only with r.url. Can I use that?

Edit: Can we use this?
if "err=1431004" not in r.url: code here else: code here

The point of disabling redirects and using r['Location'] would be to reduce the number of requests that we need to send, but you can start by implementing it without, and just use r.url.

Considering testing the error code, though your described method would probably work, I would argue we should use two functions from urllib.parse, parse_qs and urlparse, and look at whether the dict parse_qs(urlparse(url).query) contains an error code.

ThatAlexanderA · 2018-06-05T16:46:09Z

I updated the code. I used print for tell if the remove was successful, but we can use something different if you want.

madsmtm

It looks good, now all I have is nitpicky comments. Also, maybe the entire method should be moved to just below the friendConnect method? Seems like it makes more sense in that group

madsmtm · 2018-06-05T19:10:15Z

fbchat/client.py

        payload = {
            "friend_id": friend_id,
            "unref": "none",
            "confirm": "Confirm",
        }
        r = self._post(self.req_url.REMOVE_FRIEND,payload)
-        return r.ok
+        dict = parse_qs(urlparse(r.url).query)


Be careful about overwriting dict, since its a builtin. Use a different, perhaps even a more descriptive name, like query

madsmtm · 2018-06-05T19:11:57Z

fbchat/client.py

-        return r.ok
+        dict = parse_qs(urlparse(r.url).query)
+        if "err" not in dict:
+            print ("Remove was successful!")


If you feel like it's needed, log output can be useful, but don't use print, since people can't selectively disable the log output. Use log.info or log.debug here instead

Just noticed that you addressed this in your comment

madsmtm · 2018-06-05T19:16:16Z

fbchat/client.py

+        Removes a specifed friend from your friend list
+        :param friend_id: The id of the friend that you want to remove
+        :returns error when the removing was unsuccessful
+        :returns True when the removing was successful


The syntax in the docstrings is quite important, since it gets executed and published on RTFD. :returns is wrong, it should be :return:, and there should only be one of them

madsmtm · 2018-06-05T19:17:40Z

fbchat/client.py

+    from urllib.parse import parse_qs
+except ImportError:
+    from urlparse import urlparse
+    from urlparse import parse_qs


When importing two things from the same module you can use commas, like this: from urlparse import urlparse, parse_qs

ThatAlexanderA · 2018-06-05T20:00:22Z

I updated the code. :)

madsmtm · 2018-06-07T11:48:19Z

Looks good, I'll merge it as soon as I get TravisCI working again

ThatAlexanderA · 2018-06-07T11:53:28Z

Okay, thank you! :)

* Added `removeFriend` method, #298 * Removed `lxml` from dependencies, #301 * Moved configuration to setup.cfg instead of setup.py

ThatAlexanderA added 5 commits June 4, 2018 13:29

Update utils.py

a264fac

Update client.py

c842be3

Added remove friend def

36742bf

Added Remove Friend URl

e3602e8

Update client.py

91778f4

ThatAlexanderA mentioned this pull request Jun 4, 2018

Method to remove friends? #294

Closed

madsmtm requested changes Jun 4, 2018

View reviewed changes

madsmtm added the enhancement label Jun 4, 2018

Updated removeFirend

b599033

madsmtm requested changes Jun 5, 2018

View reviewed changes

Changed dict to query, replaced print with log

c7cbbdd

madsmtm approved these changes Jun 7, 2018

View reviewed changes

madsmtm added 2 commits June 20, 2018 13:51

Small formatting fixes

67edd19

Merge branch 'master' into alexander_master

2b76d71

madsmtm merged commit 51d606a into fbchat-dev:master Jun 20, 2018

madsmtm added a commit that referenced this pull request Jun 20, 2018

Version up, thanks to @orenyomtov and @ ThatAlexanderA

c8f8b81

* Added `removeFriend` method, #298 * Removed `lxml` from dependencies, #301 * Moved configuration to setup.cfg instead of setup.py

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added remove friend #298

Added remove friend #298

ThatAlexanderA commented Jun 4, 2018 •

edited

Loading

madsmtm left a comment

madsmtm Jun 4, 2018

ThatAlexanderA Jun 5, 2018

madsmtm Jun 4, 2018

ThatAlexanderA Jun 5, 2018 •

edited

Loading

madsmtm Jun 5, 2018

ThatAlexanderA commented Jun 5, 2018

madsmtm left a comment

madsmtm Jun 5, 2018

madsmtm Jun 5, 2018

madsmtm Jun 5, 2018

madsmtm Jun 5, 2018

madsmtm Jun 5, 2018

ThatAlexanderA commented Jun 5, 2018

madsmtm commented Jun 7, 2018

ThatAlexanderA commented Jun 7, 2018

Added remove friend #298

Added remove friend #298

Conversation

ThatAlexanderA commented Jun 4, 2018 • edited Loading

madsmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThatAlexanderA Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThatAlexanderA commented Jun 5, 2018

madsmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThatAlexanderA commented Jun 5, 2018

madsmtm commented Jun 7, 2018

ThatAlexanderA commented Jun 7, 2018

ThatAlexanderA commented Jun 4, 2018 •

edited

Loading

ThatAlexanderA Jun 5, 2018 •

edited

Loading