feat(nimbus): Send results available notification#14616
Conversation
yashikakhurana
requested review from
freshstrangemusic,
jaredlockhart,
mikewilli and
relud
as code owners
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Okay looking good, good clear layout, good test coverage, let's give it a shot! thnx @yashikakhurana 🙏 🎉
Because - We want to notify users on slack when the weekly and overall results are available for the experiments This commit - Check whether the results are available, and if available, send the notification - Also makes sure to send the alert once per window such as overall and weekly Fixes #14409
| class AnalysisWindow(models.TextChoices): | ||
| WEEKLY = "weekly", "Weekly" | ||
| OVERALL = "overall", "Overall" |
There was a problem hiding this comment.
Could we generate this from the existing enum definition (either in schemas or jetstream client?
|
|
||
| return False | ||
|
|
||
| def has_results_for_window(self, window): |
There was a problem hiding this comment.
I'm getting a little confused about where different results functions like this should go now that some of them are here and others are in the results manager class. For example, here we have has_results_for_window and there we have get_window_results.
This isn't a comment about this particular function, just a question more generally: is the convention that we want all the actual results fetchers and mutators to go in results_manager and all the higher level boolean-type logic to go here?
There was a problem hiding this comment.
Yeah so, I think once we have the new results page launched I want us to go back through the infrastructure we have now for
- Jetstream loading
- Transforming into the visualization structure
- Storing
- Querying
Because we have many layers there that are probably unnecessary and can collapse, but I don't think we have the structure we'll want there in place yet, so yes littering methods like this around probably isn't ideal but I think we'll be able to clean all that up in a post launch cleanup phase.
3 participants
Because
This commit
Fixes #14409