Fixing #6560: In postgres, importing changing channel content moves channel to top of list #7688
Conversation
- replaced 'latest' by 'max', should be more db independent - rewrote the function preventing the query from being executed on every call
| @@ -749,11 +750,10 @@ def calculate_included_languages(channel): | |||
|
|
|||
|
|
|||
| def calculate_next_order(channel, model=ChannelMetadata): | |||
| latest_order = model.objects.latest("order").order | |||
There was a problem hiding this comment.
I think the only line that must be changed is this one, just replacing it by
latest_order = model.objects.filter(order__isnull=False).latest("order").order
would solve the issue without more changes needed.
@paulbusse do you see any inconvenience in doing just this change?
If you don't see any issue in it, I think it's better than doing the new aggregations this PR introduces
There was a problem hiding this comment.
bah, @paulbusse forget my previous comment, your solution is better for the case where no channels have been added yet.
Please, move the PR to be targeted at release-v0.14.x instead of develop and it's fine to be merged. Sorry for the noise.
There was a problem hiding this comment.
@jredrejo I kinda made the same journey :-)
Also reading this means that latest on a column with NULL values is not trustworthy. Django should standardize the usage of NULL values in the generated query, e.g. by adding NULLS LAST. MAX must, per the definition in the standard, skip over NULL values. (Just my €0.02)
Unless I'm overlooking something I'm not able to change the target of the PR.
There was a problem hiding this comment.
@jredrejo I kinda made the same journey :-)
Also reading this means that
lateston a column withNULLvalues is not trustworthy. Django should standardize the usage of NULL values in the generated query, e.g. by addingNULLS LAST.MAXmust, per the definition in the standard, skip overNULLvalues. (Just my €0.02)Unless I'm overlooking something I'm not able to change the target of the PR.
I have changed the target in the GH UI, but the PR includes, logically, all the commits that are different between develop and release-v0.14.x. If you can, the steps I follow in these cases are:
- From the issue6560b branch:
git checkout -b keep_commit - From the release-v0.14.x:
git branch -D issue6560b
git checkout -b issue6560b
git cherry-pick id_of_your_commit
git push -f
git branch -D keep_commit
If you have problems, don't worry, leave it as it is and we'll cherry-pick the commit from develop to release-v0.14.x
There was a problem hiding this comment.
I tried, but never done this before.
I got as far as the push. But that one gives an error
fatal: The current branch issue6560b has no upstream branch.
To push the current branch and set the remote as upstream, use
git push --set-upstream origin issue6560b
you forgot the -f to force the change ;)
(PS. The first command is git branch -D issue6560b, I assumed)
yes
Don't worry. We'll cherry pick it.
Summary
The root cause is explained here.
latestbymax, should be more db independentBTW. To get rid of the bad orders of you have to reorder them once after the patch is installed.
Reviewer guidance
Following tests have been executed:
If there are more test I need to run let me know.
No migration is provided.
References
Contributor Checklist
PR process:
Testing:
Reviewer Checklist
yarnandpip)