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

feat: Add Bruno #2535

Merged
merged 9 commits into from
Aug 2, 2024
Merged

feat: Add Bruno #2535

merged 9 commits into from
Aug 2, 2024

Conversation

mickeymarse
Copy link
Contributor

As discussed on Discord, I'm helping with the creations of the API requestions collections via Bruno.

  • Continuing from where @kimadactyl left, I've added a few more requests and polished some of the old ones.
  • I am not experienced with GraphQL, so, please, feel free to leave feedback on all the queries.
  • Ultimately, I guess it could be easier if you can let me know all the requests/endpoints you would like to check, since I don't know the repo and the project as mush as you do.

Fixes #2520

@kimadactyl
Copy link
Member

kimadactyl commented Jul 13, 2024

Have a peak at the linked issue to see the queries we are actually using in production, will help next week if it's not making sense :)

ideally we have acceptance criteria written out before we start working on things but this is a bit of an experimental suck it and see ticket

@mickeymarse
Copy link
Contributor Author

My bad, I should've checked that. Thank you very much.
It seems doable. I will get back to you if I have further questions. 😊

@kimadactyl kimadactyl mentioned this pull request Jul 15, 2024
@kimadactyl kimadactyl changed the title Add Bruno (new PR) Add Bruno Jul 15, 2024
…and Trans Dimencion codebase as references

relates #2520
@mickeymarse
Copy link
Contributor Author

I've added, updated and renamed queries following the docs mentioned in the original issue.
Might be good to have a chat on Discord though when you have time.

@kimadactyl
Copy link
Member

kimadactyl commented Jul 19, 2024

This is looking really great! Awesome going.

A few minor points:

  • Let's move the hard coded vars into the dedicated vars section so the queries are nice and clean?
  • What's the diff between "All Events" and "Events Index"? I can see the queries are different but i think "index" is kinda a website-specific word that's a bit confusing in this context? Can these have more descriptive titles?
  • "Partner tag" should be called "Partnership" everywhere this is mentioned. "partnership" is a type of tag, this is one of the more confusing aspects of the code base, we are trying to stop referring to tags as it makes things more confusing!). The other types of tag are facility and category which are not really in use atm.
  • Ditto "articles by tag" is really "articles in partnership" and "partners by tag" is "partners in partnership"

@mickeymarse
Copy link
Contributor Author

Thank you very much for the feedback. I can definitely sort all of these things out.
This week I might not be around, but I have access to my laptop.
Do you need them done ASAP, or is it OK to wait for next week?

@kimadactyl
Copy link
Member

kimadactyl commented Jul 22, 2024

thankyou! no hurry, none of this is time critical!

@katjam
Copy link
Member

katjam commented Jul 24, 2024

Thanks for picking this up. Feel free to tag me if you have questions or need a hand. As Kim says, no rush at all!

…he function it's still buggy and needs fixing so the task couldn't be completed as nicely as expected.

relates #2520
@mickeymarse
Copy link
Contributor Author

* Let's move the hard coded vars into the [dedicated vars section](https://docs.usebruno.com/scripting/vars) so the queries are nice and clean?

Done. However, that function seems to be still buggy and there are a couple of unmerged PR on the Bruno repos, so it looks more confusing than it was before right now. I believe there are issues with the JS template literals interpolation.
Unless I didn't really get it, and I'm doing something wrong, of course.

* What's the diff between "All Events" and "Events Index"? I can see the queries are different but i think "index" is kinda a website-specific word that's a bit confusing in this context? Can these have more descriptive titles?

The index ones are a paginated list, which, once looking at it again, seems just like a repetition of the Filter query.
So, I would remove it from both and add a Partners by Filter query if you agree with this.

* "Partner tag" should be called "Partnership" everywhere this is mentioned. "partnership" is a type of tag, this is one of the more confusing aspects of the code base, we are trying to stop referring to tags as it makes things more confusing!). The other types of tag are facility and category which are not really in use atm.

* Ditto "articles by tag" is really "articles in partnership" and "partners by tag" is "partners in partnership"

If this was referring only to the title of the queries, I did it. Otherwise, not sure what you want me to rename.

Also, I'm now getting this error when trying to push my commits:

ERROR: Permission to geeksforsocialchange/PlaceCal.git denied to mickeymarse.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Hence, you're probably reading this without being able to see what I'm talking about.
Any idea why this is happening?

@kimadactyl
Copy link
Member

kimadactyl commented Aug 1, 2024

Sounds great, good work!

Hence, you're probably reading this without being able to see what I'm talking about. Any idea why this is happening?

Hmm - are you still logged in to git in the terminal?

I always get a bit confused when working on a forked version rather than just being a repo contributor. While we could just make you a repo contributor its nice for us to try out having change requests from outside the main repo too. I think github gets a bit confused though but fundamentally the PR should be from your github username not the gfsc one if that makes sense (im too hot!)

@mickeymarse
Copy link
Contributor Author

Hmm - are you still logged in to git in the terminal?

Problem solved. I was pushing to the wrong branch. 🙃
You can check the commits now.

@kimadactyl
Copy link
Member

kimadactyl commented Aug 1, 2024

This is looking fantastic. I fucked up the staging server so we can't merge into main until the letsencrypt bot resets my ratelimit tomorrow but will merge then. Awesome work thankyou! Woo!

@kimadactyl kimadactyl enabled auto-merge (squash) August 1, 2024 14:34
@kimadactyl
Copy link
Member

Not sure why the tests won't run. Gonna attempt turning it off and on again...

@kimadactyl kimadactyl closed this Aug 2, 2024
auto-merge was automatically disabled August 2, 2024 11:25

Pull request was closed

@kimadactyl kimadactyl reopened this Aug 2, 2024
@kimadactyl kimadactyl merged commit bdb9f0b into geeksforsocialchange:main Aug 2, 2024
2 checks passed
@kimadactyl
Copy link
Member

Ok, this doesn't touch any actual business logic files so I'm bypassing branch protection this once.

If it happens again with an external contributor, I think it might be something to do with our token configuration.

Thanks @mickeymarse!! Congrats on your first merge!

@mickeymarse mickeymarse deleted the add-bruno branch August 2, 2024 13:17
@kimadactyl kimadactyl changed the title Add Bruno feat: Add Bruno Aug 20, 2024
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.

Move API code examples inside codebase
3 participants