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

fix: default context for painted door experiment #218

Merged

Conversation

brian-smith-tcril
Copy link
Contributor

@brian-smith-tcril brian-smith-tcril commented Oct 13, 2023

There are multiple places where properties of the object provided by usePaintedDoorExperimentContext() are assumed to exist. This provides default (null) values for those properties when creating the context.

This PR is proposing a solution to #210 that would make it so #214 isn't needed.

@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1a51ac0) 96.46% compared to head (0cedeb0) 96.46%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #218   +/-   ##
=======================================
  Coverage   96.46%   96.46%           
=======================================
  Files         195      195           
  Lines        1839     1839           
  Branches      322      322           
=======================================
  Hits         1774     1774           
  Misses         60       60           
  Partials        5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@httpsmenahassan httpsmenahassan left a comment

Choose a reason for hiding this comment

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

I tested this locally and it runs fine. Assuming Cindy's comment gets addressed, this LGTM!

@brian-smith-tcril brian-smith-tcril force-pushed the dont-use-env-var-to-paint-the-van branch from aa34eee to 0a57d8d Compare October 17, 2023 18:24
@arbrandes arbrandes added the needs maintainer attention Issue or PR specifically needs the attention of the maintainer. label Oct 17, 2023
@brian-smith-tcril brian-smith-tcril force-pushed the dont-use-env-var-to-paint-the-van branch from 0a57d8d to b2f5a65 Compare October 18, 2023 15:47
@justinhynes
Copy link
Contributor

I'll be bringing this to our next team refinement session.

@brian-smith-tcril brian-smith-tcril force-pushed the dont-use-env-var-to-paint-the-van branch from b2f5a65 to 98e0742 Compare October 26, 2023 17:02
@arbrandes
Copy link
Contributor

@justinhynes, any objections to this?

@jsnwesson
Copy link
Contributor

Hey @arbrandes , since I'm on-call this week, I will approve this now that I understand that it wasn't meant to be merged alongside #214 !

There are multiple places where attributes of the object provided by `usePaintedDoorExperimentContext()` are assumed to exist. This provides default (null) values for those attributes when creating the context.
@brian-smith-tcril
Copy link
Contributor Author

@jsnwesson i just rebased this so it should be up to date now

@brian-smith-tcril brian-smith-tcril force-pushed the dont-use-env-var-to-paint-the-van branch from 98e0742 to 0cedeb0 Compare November 14, 2023 19:28
@jsnwesson jsnwesson merged commit 8f2ed77 into openedx:master Nov 14, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs maintainer attention Issue or PR specifically needs the attention of the maintainer.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants