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

🗺️ 662 Filter Programs by Region #704

Merged
merged 16 commits into from
Nov 22, 2023

Conversation

demariadaniel
Copy link
Contributor

@demariadaniel demariadaniel commented Nov 16, 2023

Description of changes

Filters programs by requested regions

Type of Change

  • Bug
  • New Feature

@demariadaniel demariadaniel changed the title WIP: Feat/662 filter programs by region 🗺️ 662 Filter Programs by Region Nov 16, 2023
@daniel-cy-lu
Copy link
Contributor

@joneubank @demariadaniel I have ticket #690 . Where I am to remove the "regions" field in the Program Type. I see Dan is using program.regions as part of his filter and this would be broken if I am to remove regions.

@demariadaniel
Copy link
Contributor Author

@joneubank @demariadaniel I have ticket #690 . Where I am to remove the "regions" field in the Program Type. I see Dan is using program.regions as part of his filter and this would be broken if I am to remove regions.

Thanks for mentioning, I saw that too, makes things confusing

@daniel-cy-lu
Copy link
Contributor

daniel-cy-lu commented Nov 16, 2023

@demariadaniel @joneubank Jon was at my desk looking at this PR. So we won't be using regions going forward. Instead we will be using dataCenter field. So maybe we do the same thing filtering by dataCenter. I have a ticket #693 that I will be adding dataCenter to the Program type. It had a blocker that just got resolved. I will work on it now, so Dan you can modify your filter.

@demariadaniel demariadaniel changed the title 🗺️ 662 Filter Programs by Region WIP: 🗺️ 662 Filter Programs by Region Nov 16, 2023
@demariadaniel demariadaniel marked this pull request as draft November 16, 2023 22:07
@demariadaniel
Copy link
Contributor Author

@demariadaniel @joneubank Jon was at my desk looking at this PR. So we won't be using regions going forward. Instead we will be using dataCenter field. So maybe we do the same thing filtering by dataCenter. I have a ticket #693 that I will be adding dataCenter to the Program type. It had a blocker that just got resolved. I will work on it now, so Dan you can modify your filter.

Thank you! I will revert this to a draft PR for now. I will follow your changes!

@daniel-cy-lu
Copy link
Contributor

@demariadaniel dataCenter is ready. It's a property in the gql Program type.

@demariadaniel demariadaniel marked this pull request as ready for review November 20, 2023 16:19
@demariadaniel demariadaniel changed the title WIP: 🗺️ 662 Filter Programs by Region 🗺️ 662 Filter Programs by Region Nov 20, 2023
Copy link
Member

@joneubank joneubank left a comment

Choose a reason for hiding this comment

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

It would be better to modify the existing programs query to add an optional filter by datacenter ID. We've definitely added new queries like this before but you can see the amount of noise that generates.

Can we rebuild it as a filter instead of as a new query?

@demariadaniel
Copy link
Contributor Author

It would be better to modify the existing programs query to add an optional filter by datacenter ID. We've definitely added new queries like this before but you can see the amount of noise that generates.

Can we rebuild it as a filter instead of as a new query?

Yes, good call. Updated in 821fde1

Copy link
Member

@joneubank joneubank left a comment

Choose a reason for hiding this comment

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

Great change, thanks. Minor request here.

src/schemas/Program/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ciaranschutte ciaranschutte left a comment

Choose a reason for hiding this comment

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

looks good to me

@ciaranschutte
Copy link
Contributor

@demariadaniel ah I thought my approval would be enough to merge. looks like you specifically need whoever requested changes approval. @joneubank

@demariadaniel demariadaniel merged commit 2a173b7 into develop Nov 22, 2023
@demariadaniel demariadaniel deleted the feat/662-filter-programs-by-region branch November 22, 2023 15:55
@demariadaniel demariadaniel mentioned this pull request Dec 5, 2023
2 tasks
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.

4 participants