Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update bc-papi.php #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

sunil-ca
Copy link

@sunil-ca sunil-ca commented Sep 20, 2017

New changes to Search method

News changes to Search method
@hmhnewmant
Copy link
Contributor

@sunil-ca can you please create a branch with all your changes so i can easily pull down to test? thanks!

@sunil-ca
Copy link
Author

sunil-ca commented Sep 28, 2017

@hmhnewmant I tried PUSH my changes using new branch (also sent email back with screenshot of the error), but looks like I do not have permissions to PUSH a branch.

I was getting - fatal: unable to access 'https://github.com/BrightcoveOS/Brightcove-Playback-API-Wrapper.git' : The requested URL returned error: 403

@hmhnewmant
Copy link
Contributor

@sunil-ca thanks for trying, sorry, that actually makes sense. Could you possibley combine all changes into one PR?

@hmhnewmant
Copy link
Contributor

@sunil-ca I figured out how to pull your PR locally, i'll have a look at them all, no need to do anything else right now

@sunil-ca
Copy link
Author

@hmhnewmant thanks Theresa

Copy link
Contributor

@hmhnewmant hmhnewmant left a comment

Choose a reason for hiding this comment

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

@sunil-ca same here, incorporating these changes in my next PR. will not merge this on
cc @rcrooks

@sunil-ca
Copy link
Author

sunil-ca commented Sep 29, 2017

@hmhnewmant when your search function is ready, I will compare with mine and will make changes at my end accordingly, thanks.

@sunil-ca
Copy link
Author

@hmhnewmant if you have not started working on your search, take this code and improve/change what you want after testing. I am happy to help, if you need any kind of help in testing this. Everything is setup on my end as we use brightcove api every day - I can be a very good resource to test any functionality.

@theresaweb
Copy link

@sunil-ca Please feel free to take a fork and run with it my code is very similar. I've also added a note to make sure that the Policy Key is search enabled.

@sunil-ca
Copy link
Author

@theresaweb how can I see your changes? I am new to gitHub, please guide me

@hmhnewmant
Copy link
Contributor

@sunil-ca I do not have control over permissions here, if you are able to see the branch I just pushed you will find my changes there, https://github.com/BrightcoveOS/Brightcove-Playback-API-Wrapper/tree/add-search

@sunil-ca
Copy link
Author

sunil-ca commented Sep 29, 2017

@theresaweb yes that is true, we need search enabled policy key. It took me some time to get that key. One more interesting thing. The new PlayBack API does not return video views when it return video data either by video_by_id or play_list_bay_id. Brightcove told me to use Analytics API to get video views. I have created Analytics API wrapper based on this PlayBack wrapper and did the required changes. It is working, I get the video views, but it is not as fast as PlayBack API.

@sunil-ca
Copy link
Author

@hmhnewmant @theresaweb @BrianFranklin @rcrooks @doktorj7 I need your suggestion, how to efficiently get video views. Now if I get playlist of 20 videos, I have to make video views call 20 times to get the views for each video. Any suggestion?

@sunil-ca
Copy link
Author

@theresaweb I saw your search changes, similar to mine. One suggestion, you need to urlencode $q being passed - see brightcove documentions -https://support.brightcove.com/cmsplayback-api-videos-search
Let me know if I am wrong, I can make changes on my end. I am doing this
public function search($type = 'videos', $terms = '', $params = NULL)
{
$terms = urlencode($terms);
$offset = (isset($params['offset']) && $params['offset'] != '')?$params['offset']:0;
$sort = (isset($params['sort']) && $params['sort'] != '')?$params['sort']:'updated_at';
$limit = (isset($params['limit']) && $params['limit'] != '')?$params['limit']:20;
$url = $this->url_read . $this->bc_account . '/' . $type . '?q='. $terms.'&sort='.$sort.'&offset='.$offset.'&limit='.$limit;
return $this->getData($url);
}

@sunil-ca
Copy link
Author

sunil-ca commented Sep 29, 2017

@theresaweb and it is my suggestion do not overload the function too much, $params can hold all - sort, limit and offset. Or any other needed in future.

@sunil-ca
Copy link
Author

sunil-ca commented Sep 29, 2017

@theresaweb I also just noticed search function which is just pushed missing offset, it is required if we are showing search result on multiple pages e.g. 1|2|3|4 . for the second page offset will be 20, if the first page showed, first 10 search results.

@theresaweb
Copy link

@sunil-ca thanks for your suggestions. please do fork the repo and use as you wish. it is mainly meant as a baseline suggestion. I will update as I feel necessary. For questions on methods to accomplish things with the apis I suggest you contact [email protected] or join one of the brightcove google groups

@sunil-ca
Copy link
Author

sunil-ca commented Sep 29, 2017

@theresaweb my suggestions is make this wrapper better based on what work I did recently and approved by our QA team here in my company, Please do not take personally. I am trying to improve this wrapper. And I already talked to Brightcove team a lot last week while working on this wrapper, they do not provide any support for wrapper. They only help with their api only. Sorry if hurt your feelings.

@theresaweb
Copy link

no feelings hurt! I am working on other things, please go forward with your suggestions as you please, I do not work for brightcove, this was just meant to give a start to those who need it. please enjoy :D

@sunil-ca
Copy link
Author

@theresaweb I am new to gitHub, this first set of interactions through this wrapper. I have no idea how things works here. This wrapper helped me a lot initially. So wanted to improve this as much I can based on my recent experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants