-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
refactor: make console logs and network performance buttons toggled ON by default #23900
refactor: make console logs and network performance buttons toggled ON by default #23900
Conversation
hey @namit-chandwani i'm actually torn on whether we should default these to true 🙈 am going to tag @daibhin to get his opinion too I think you'll need to add a migration for this something like |
Feels like the right call to me. The other option I guess would be to hide the settings until you've enabled Replay but I think that would hurt discovery of how powerful Replay is (console logs and network performance being some of the most powerful). It would be great if we could also update the copy of the |
Thanks for the timely review @pauldambra and @daibhin! Request you to have a look. TIA |
…ctor/session-replay-toggles
5b73d72
to
adfe554
Compare
Will resolve the test cases failing in the workflow shortly |
…ctor/session-replay-toggles
…ctor/session-replay-toggles
@pauldambra @daibhin I've fixed all the failing backend test cases now. Seems like we're good to go |
migrations.AlterField( | ||
model_name="team", | ||
name="capture_console_log_opt_in", | ||
field=models.BooleanField(blank=True, default=True, null=True), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this change affects existing rows... so I don't think we have any reason to worry about table locks here
@daibhin are you ok to help get this over the line? |
Hey @daibhin, I’ve made the changes to the unit test as suggested in the review comment: #23900 (comment) now. Please review the changes at your convenience and let me know if there are any further adjustments required. Thanks! |
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
ee6fe50
to
10c3362
Compare
@daibhin The CI check: https://github.com/PostHog/posthog/actions/runs/10398937870/job/28847237140?pr=23900 was failing after the latest change that I had pushed I've fixed it now. Seems like we're good to go |
Hey @daibhin, I noticed that the latest Storybook workflow is failing: link to the failing workflow. The issue seems to be coming from the Interestingly, this PR doesn’t include any changes that would affect the Pipeline section. So can you please suggest how we should address this? |
619daf1
to
0beaf18
Compare
@namit-chandwani it is most likely a flakey test. I suggest rebasing off master and we can kick the CI off again |
@daibhin It's done. Request you to trigger the CI now |
Hey @daibhin, I appreciate your efforts on the review. As this is my first contribution to the PostHog project, I’m really looking forward to seeing it completed :) TIA |
@namit-chandwani hey, sorry for the slow turnaround on this one. I'm working across a bunch of things right now and haven't been quick enough here to beat the latest migration. Would you be able to rebase again and update the migration? I can kickoff another CI run in that case and hopefully get things to pass. Alternatively I can recreate your changes on my own branch and have a quicker feedback loop that way |
@daibhin No worries at all! I understand how it gets with multiple things going on. |
@namit-chandwani fantastic! Fingers crossed for some 🟢 this time 🤞 |
@namit-chandwani went ahead and merged this one because I was afraid another migration would come otherwise. I'll leave the issue to you to close. Congrats on getting this in!! Thanks for sticking with us 🙏 |
@daibhin Thanks a ton for merging the PR! Your support and insights have been incredibly helpful :) Excited to see these changes in action and can’t wait to dive into more contributions. |
Linked Issues
Problem
Changes
Backend (models/team.py):
default=True
tocapture_console_log_opt_in
andcapture_performance_opt_in
fields in theTeam
model.True
when a new team instance is created.null=True
andblank=True
for both fields to ensure backward compatibility, preventing potential issues with older data containing empty or null values.True
when a new team instance is created, while maintaining compatibility with existing data.Frontend (SessionRecordingSettings.tsx):
checked
attribute logic forcapture_console_log_opt_in
andcapture_performance_opt_in
settings:currentTeam?.session_recording_opt_in ? !!currentTeam?.capture_console_log_opt_in : false
to!!currentTeam?.capture_console_log_opt_in
.capture_console_log_opt_in
andcapture_performance_opt_in
fields without depending onsession_recording_opt_in
.session_recording_opt_in
when determining the default state ofcapture_console_log_opt_in
andcapture_performance_opt_in
checkboxes.These changes improve the consistency and reliability of the log and performance capture settings by making their defaults explicit and simplifying the logic for determining their states, while ensuring backward compatibility with existing data.
Does this work well for both Cloud and self-hosted?
How did you test this code?
Backend Testing:
null
or empty values forcapture_console_log_opt_in
andcapture_performance_opt_in
remain unaffected.Frontend Testing:
capture_console_log_opt_in
andcapture_performance_opt_in
reflect the correct state based on the updated logic.session_recording_opt_in
is enabled and disabled.Screen Recording of Testing:
Feature.testing.recording.6.mp4
Type of change:
Checklist:
Follow-up tasks (if any):
Additional Comments