Skip to content
This repository has been archived by the owner on Jul 9, 2022. It is now read-only.

Add changeAdminStatus #658

Closed
wants to merge 3 commits into from
Closed

Add changeAdminStatus #658

wants to merge 3 commits into from

Conversation

AstroCB
Copy link
Contributor

@AstroCB AstroCB commented Jun 14, 2018

Adds a changeAdminStatus function to allow the API to alter the admin status of users in group chats (naming suggestions welcome).

re: #624

@ravkr
Copy link
Contributor

ravkr commented Jun 14, 2018

could you do

git remote add upstream https://github.com/Schmavery/facebook-chat-api
git fetch upstream
git rebase upstream/master
git push -f

it would get rid of all these duplicated commits in every pull request :/

@AstroCB AstroCB force-pushed the master branch 4 times, most recently from e4db993 to 5fc94b4 Compare June 14, 2018 01:37
@ravkr ravkr mentioned this pull request Jun 14, 2018
@AstroCB AstroCB force-pushed the master branch 4 times, most recently from 32b4acc to d88f21f Compare June 14, 2018 06:50
@AstroCB
Copy link
Contributor Author

AstroCB commented Jun 14, 2018

Good call – thanks @ravkr

utils.getType(threadID) === "Function" ||
utils.getType(threadID) === "AsyncFunction")
) {
throw { error: "please pass makeAdmin and threadID as the second/third arguments." };
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these are required fields, we can probably just check their types rather than checking if they're not a callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, another holdover from the basis file I used.

@Schmavery
Copy link
Owner

Thanks @AstroCB
@ravkr does this PR do the same thing as #659? What's your concern with this one? I don't see much difference other than extra fields in the form, can someone confirm whether these are sent by the browser? If so, we should mimic that.
Or is there something else I'm missing?

@ravkr
Copy link
Contributor

ravkr commented Jun 14, 2018

Yes, both PR are doing the same thing
But mine looks a bit cleaner (no extra fields)
I copied that request from messenger.com

I'm not saying that one PR is better that the other, but I just wanted to show what I've made

@Schmavery
Copy link
Owner

@AstroCB did you find these fields are required/used by the browser when you looked or do you think we can just remove them?

@AstroCB
Copy link
Contributor Author

AstroCB commented Jun 14, 2018

Nope, they’re just holdovers from another file that I used as the basis for this – should be good to remove.

I can take them out and clean up the param checks, but it might just be easier to merge @ravkr’s version.

@Schmavery
Copy link
Owner

@ravkr 's change has no docs though 😛
At this point idk which one to spend time reviewing, if you two want to decide if/how you want to split this up, I don't see a problem with adding this functionality.

@ravkr
Copy link
Contributor

ravkr commented Jun 15, 2018

You can review mine PR
I'll add docs soon

@AstroCB
Copy link
Contributor Author

AstroCB commented Jun 15, 2018

Made the discussed changes last night and forgot to publish – they're up now, but either way works for me.

@ravkr
Copy link
Contributor

ravkr commented Jun 18, 2018

@Schmavery what's the decision?

@Schmavery
Copy link
Owner

Thanks for helping out @AstroCB, we ended up merging @ravkr's branch <3

@Schmavery Schmavery closed this Jul 30, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants