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

Improve directory filtering to exclude archived providers #616

Merged
merged 18 commits into from
Aug 28, 2024

Conversation

fershad
Copy link
Contributor

@fershad fershad commented Aug 23, 2024

This tiny PR improves the filtering of providers when the Directory list is rendered.

The previous logic would only exclude providers who were marked as showOnWebsite=false. This exposed an edge case where providers might be archived (archived=true) however might still have the showOnWebsite variable set to true (showOnWebsite=true). In this case the provider would incorrectly be shown on the Directory even though they are archived.

This PR fixes that.

Comment on lines +251 to +254
queryset = Hostingprovider.objects.filter(
showonwebsite=True,
archived=False
).prefetch_related(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrchrisadams The only change I made was here. But this PR is showing a bunch of other files being changed too. Is that because I'm merging it into Staging rather than Main?

Copy link
Member

@mrchrisadams mrchrisadams Aug 23, 2024

Choose a reason for hiding this comment

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

hi @fershad - yes, you're correct.

I think it might be because what's on staging might not be the same as master right now.

I've typically treated staging as something we're ok to git push --force rather than open up PRs to staging.

My reasoning is that while we care about a specific branch being merged into master, there are sometimes cases where we might want to see how something looks in staging, without then proceeding directly into merge it into master. We also make no guarantees about staging always working.

So, I usually open a PR against master and to see how something loooks I push to staging like this if I want feedback:

git push origin ca-card-350-fix-ipv6-support:staging -f                                                                                                                                           

and in this case we'd do something like

  1. update this PR to be a PR against master

  2. force push the change to staging with something like the command below

git push origin improve-directory-filter:staging -f                                                                                              

I'll try to respond in Zulip today if you DM me, otherwise we have the call on Monday to chat as well.

Ideally we'd have a whizzy thing to make ephemeral branches the way cloudflare pages offers, but that I think would involve a bunch of extra CI pipeline work to spin up ephemeral servers and resources, and other work has taken precedence.

Copy link
Member

@mrchrisadams mrchrisadams Aug 23, 2024

Choose a reason for hiding this comment

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

@fershad are you ok with updating the PR so it's a change against master, a bit like the way you have a comparison view here?

master...add-disclaimer-for-dataset

That should make the changest waaaay smaller.

Copy link

Eco-CI Output:

Label 🖥 avg. CPU utilization [%] 🔋 Total Energy [Joules] 🔌 avg. Power [Watts] Duration [Seconds]
Total Run (incl. overhead) 26.689 451.091 3.79 119
checkout 8.96 3.59424 1.80 2
pip install uv wheel 9.07 3.59487 3.59 1
pip install requirements 67.2367 38.7742 5.54 7
pytest 24.1852 405.128 3.86 105

🌳 CO2 Data:
City: Chicago, Lat: 41.8874, Lon: -87.6318
IP: 172.183.131.99
CO₂ from energy is: 0.175474399 g
CO₂ from manufacturing (embodied carbon) is: 0.033952343 g
Carbon Intensity for this location: 389 gCO₂eq/kWh
SCI: 0.209427 gCO₂eq / pipeline run emitted

@mrchrisadams mrchrisadams merged commit 9924adc into staging Aug 28, 2024
3 checks passed
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