-
Notifications
You must be signed in to change notification settings - Fork 190
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
License description rectification #4917
License description rectification #4917
Conversation
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.
Seems good to me, just one question about channel publishing. I asked @ozer550 to take a look too
|
||
# we would republish the channel | ||
for channel_id in channel_ids_to_republish: | ||
publish_channel(user_id, channel_id) |
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 know this was in the existing migration, but I also know that we did this in lockstep with emails we sent out. I'm not really sure if we should keep it or not.
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.
Yeah, I would be fine with dropping this. @ozer550 does that make sense to you too?
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.
We still might want to republish it? as making changes to license field would result in changed=True
, to_be_republished = not (base_channel.main_tree.get_family().filter(changed=True).exists())
this does the necessary check if there is some change that already exists and we would skip publishing for such cases.
Summary
References
Fixes #4916
Reviewer guidance
Not sure if removing the date filter is going to make the query completely nonperformant - but it's only an extra two years? Not sure.
I think the fix is straight forward and neatly regression tested, so careful eyes on how I have modified the rectification script would be appreciated.