WIP: Add import_redirects command to allow csv redirects import #204
Conversation
Adapted from Ambition with additions: - links directly to URL if page object cannot be found - wrapped in transactions.atomic()
|
Thanks for this Jacob, I have made some comments, let me know if they don't make sense. |
| """ Takes a full url. Roughly reproduces wagtail.wagtailcore.views.serve. | ||
| """ | ||
| parsed_path = urlparse(path) | ||
| try: |
There was a problem hiding this comment.
I don't think we should need to try/catch this (though doubtless useful during developnment), as we will be running this from the console and will see any exceptions there
| try: | ||
| site = Site.objects.get(hostname=parsed_path.netloc) | ||
| except Site.DoesNotExist: | ||
| import pdb; pdb.set_trace() |
There was a problem hiding this comment.
We should remove references to debugging tools, they will not be present on the production environment anyway.
Sidenote, have you tried pudb? I find it a bit more friendly once you get used to it.
https://github.com/inducer/pudb
| old_path = row[from_header] | ||
| new_path = row[to_header] | ||
|
|
||
| if old_path and new_path: |
There was a problem hiding this comment.
If you wrap old_path and new_path in strip() we would also catch any cells with junk whitespace in the csv, given that it is client provided data.
Also, it is good practice to avoid large nested if statements etc by doing a check like this first, and skipping this iteration of the for loop if we don't have the values we want. That way code doesn't end up drifting over to the right, and breaking line length linting rules, and it ends up a bit more readable, as you have a set of checks up front at the top.
eg rather than all this being inside and if statement, you could instead
if not old_path:
continue
if not new_path:
continue
and then just carry on with the import stuff beneath
|
|
||
| # We don't use .get_or_create because we want to support the | ||
| # --dry-run flag | ||
| with transaction.atomic(): |
There was a problem hiding this comment.
It is a good idea to use atomic transaction here, but the transaction.atomic bit should take the place of the try catch in each of these cases, or at least happen within the try part rather than wrapping it:
See Avoid catching exceptions inside atomic in https://docs.djangoproject.com/en/2.2/topics/db/transactions/
| if not dry_run: | ||
| redirect.redirect_link = target_url | ||
| redirect.save() | ||
| continue |
There was a problem hiding this comment.
This continue is not needed, as we have reached the end of the loop
| if not dry_run: | ||
| redirect.redirect_page = target_page | ||
| redirect.save() | ||
| except Page.DoesNotExist: |
There was a problem hiding this comment.
As above, probably best to keep these checks for situations which we don't want to create a redirect all together up front of the loop.
That way, readability wise, a developer can skip through those negative conditions, and then see a really uncomplicated action at the bottom, like ok we're good, save the thing
That helps avoid some nesting etc too, which is sort of the enemy of readability, especially in a whitespace language like python.
Adapted from Ambition with additions: