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

Use graphql API #123

Merged
merged 11 commits into from
Apr 26, 2022
Merged

Use graphql API #123

merged 11 commits into from
Apr 26, 2022

Conversation

schloerke
Copy link
Collaborator

@schloerke schloerke commented Dec 28, 2021

@drmowinckels
Copy link
Member

Great stuff. Have not looked in detail and tested yet, but have an initial thought by looking at the commits and comments here.

I see currently, we are appending 2 to the new functions, which is fine for now, but perhaps there could be an argument toggle for api_version = c("v3", "gql") in the functions, that would call the relevant version? That could make it easy to switch version when needed now in the beginning while both are supported?

@schloerke
Copy link
Collaborator Author

@Athanasiamo I hope to eventually just have the original function names be implemented using the GraphQL queries. I've updated notes above to rename methods like get_events2() to get_events() when ready.


Given meetup.com will drop support for "v3" API sooner than later, I'd like to not add an api_version parameter.

From: https://www.meetup.com/meetup_api/

RESTful API support will stop in winter 2021.

@drmowinckels
Copy link
Member

That sounds totally reasonable to me, @schloerke ! I hope to get time to do initial tests over the weekend. Great work, and I'm learning a lot by looking at how you're approaching this :)

@GregSutcliffe
Copy link
Contributor

How's this looking so far? I was super busy last year, so I missed the ping in #118, but I've just realised the REST API has stopped - I'm getting 401 Unauthorised on my reports since 1st Feb.

I'm willing to pitch in and help, but I'm unsure where to start. Trying to run find_groups2() is currently erroring out for me:

> devtools::load_all()
ℹ Loading meetupr
> find_groups2('Ansible')
Error in gh_process_response(raw) : 
GitHub API error (400): 
Message: 

Errors:
                                                               message
 Variable "$topicCategoryId" of required type "Int!" was not provided.
Type .Last.error.trace to see where the error occurred
Called from: throw(cond)

Any pointers for digging in @schloerke? I'm reasonably familiar with GraphQL (done a fair bit with the GH API), but I haven't yet taken a look at the Meetup query explorer...

@benubah
Copy link
Collaborator

benubah commented Feb 10, 2022

@GregSutcliffe Try find_groups2('Ansible', topic_category_id = 546)

@GregSutcliffe
Copy link
Contributor

@benubah that works, thanks for the pointer. I guess I should go read up on what that parameter does - right now I have 2 calls to find_groups in my code, one for the "Ansible" string, the other for topic "1434182", so that I can catch groups in our field without Ansible in the title. I'm guessing I can't do that anymore, but I'll poke at it.

I can see that get_events2 is working as well (at least from a quick test), and I think I'm only using those two calls, so I can probably get my report back into a working state from here - great work, everyone! I'll be sure to feedback what I find, and if I have time I'll try to help with the remaining pieces ;)

@benubah
Copy link
Collaborator

benubah commented Feb 11, 2022

@schloerke I tested removing the topicCategoryId variable from the query so that we can just have find_groups2('r-ladies') and it works well and also automatically gives us the value topicCategoryId in the response. With this we don't have to guess what value to pass into topicCategoryId when calling find_groups2().
And we get a little more related records added to what we used to get before.
What do you think?

@schloerke
Copy link
Collaborator Author

@benubah I still believe it is useful to have the topic_category_id query parameters. Otherwise it will be very difficult to exclude other topic category ids.

With some testing on the meetup playground, I was able to turn

  $topicCategoryId: Int!
  # into 
  $topicCategoryId: Int = null

... and still have the query work. This would allow for find_groups2(topic_category_id = ) to default to NULL.

@benubah
Copy link
Collaborator

benubah commented Feb 11, 2022

@schloerke This appears better and works. There seems to be several undocumented stuff around the API. Does this mean we need to provide a list of topicCategoryIds (at least a few) in our documentation for new users to start with. I can't see a list of these ids documented anywhere on the Meetup.com website.

@benubah
Copy link
Collaborator

benubah commented Feb 11, 2022

I also see that there are many objects and parameters that we need that are hidden in the schema without documentation. Will just take some time to figure out a whole bunch of these hidden things. I'll be taking a look in the next few days.

@benubah
Copy link
Collaborator

benubah commented Feb 14, 2022

@schloerke Does find_groups2('ladies', topic_category_id = NULL) fail for you. At the Meetup playground, it returns an error, but also returns results.

  Meetup GraphQL API returned errors.
List of 2
 $ :List of 4
  ..$ message   : chr "Unknown time-zone ID: US/None"
  ..$ locations :List of 1
  .. ..$ :List of 2
  .. .. ..$ line  : int 49
  .. .. ..$ column: int 13
  ..$ path      :List of 6
  .. ..$ : chr "keywordSearch"
  .. ..$ : chr "edges"
  .. ..$ : int 0
  .. ..$ : chr "node"
  .. ..$ : chr "result"
  .. ..$ : chr "foundedDate"
  ..$ extensions:List of 2
  .. ..$ code     : chr "INTERNAL_SERVER_ERROR"
  .. ..$ exception:List of 1
  .. .. ..$ message: chr "Unknown time-zone ID: US/None"
 $ :List of 4
  ..$ message   : chr "Unknown time-zone ID: US/None"
  ..$ locations :List of 1
  .. ..$ :List of 2
  .. .. ..$ line  : int 50
  .. .. ..$ column: int 13
  ..$ path      :List of 6
  .. ..$ : chr "keywordSearch"
  .. ..$ : chr "edges"
  .. ..$ : int 0
  .. ..$ : chr "node"
  .. ..$ : chr "result"
  .. ..$ : chr "timezone"
  ..$ extensions:List of 2
  .. ..$ code     : chr "INTERNAL_SERVER_ERROR"
  .. ..$ exception:List of 1```

Copy link
Member

@drmowinckels drmowinckels left a comment

Choose a reason for hiding this comment

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

I've not yet tested the functions, but have had a peak through the code and made some comments on where I think there could be improvements.

@@ -8,7 +8,7 @@ name: with-auth

jobs:
with-auth:
runs-on: macOS-latest
runs-on: macos-latest
Copy link
Member

Choose a reason for hiding this comment

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

any reason you switched the other workflows to ubuntu, while this remains macos?

country
}
<< extra_graphql >>
}
Copy link
Member

Choose a reason for hiding this comment

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

so, if I understand correctly. Most queries to the graphql API will need a similar type of graphql template to retrieve the fields and values?

if (is.null(pb)) {
pb <- progress::progress_bar$new(
total = total_fn(graphql_res),
format = paste0(file, " ", pb_format)
Copy link
Member

Choose a reason for hiding this comment

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

change to paste(file, "pb_format) ?
neater imo

Comment on lines +127 to +131
"Authorization" = paste0(
.token$auth_token$credentials$token_type,
" ",
.token$auth_token$credentials$access_token
)
Copy link
Member

Choose a reason for hiding this comment

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

change to paste(file, "pb_format) ?
neater imo

country <- get_country(group)
group$country_name <-
if (length(country) == 0 || nchar(country) == 0) {
""
Copy link
Member

Choose a reason for hiding this comment

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

is it perhaps better to return NA_character rather than empty string?

@GregSutcliffe
Copy link
Contributor

Current branch is working for me to generate my reports, thanks to all for the hard work!

Given the Meetup REST API is now dead anyway, does it make sense to rename the methods (s/2//g) and merge what we, and then bring additional PRs for the rest of the calls? A partial library is better than no library at all, I think.

@benubah
Copy link
Collaborator

benubah commented Feb 22, 2022

Current branch is working for me to generate my reports, thanks to all for the hard work!

@GregSutcliffe were you able to find groups by topic_id?

Given the Meetup REST API is now dead anyway, does it make sense to rename the methods (s/2//g) and merge what we, and > then bring additional PRs for the rest of the calls? A partial library is better than no library at all, I think.

Working on this.

@benubah benubah mentioned this pull request Mar 15, 2022
6 tasks
@benubah
Copy link
Collaborator

benubah commented Mar 15, 2022

Will be continuing the GraphQL updates here: #138

@ledell ledell merged commit 21247bf into master Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants