Refactor page form view

[JoeyStk]

Short description

This PR refactors the post method of page_form_view a little bit by adding some functionality to sub methods.

Proposed changes

• Move some functionality of the post method into sub methods

Side effects

• We decided to keep the conditional statement in the main method, because we think it's better to read this way. However with this in mind, we were only able to reduce the cyclomatic complexity marginally from 28 to 26.

Faithfulness to issue description and design

There are no intended deviations from the issue and design.

How to test

Resolved issues

Fixes: None
Hopefully this helps us a little bit with #3837

---

Pull Request Review Guidelines

[jonbulz]

Thanks, this is definitely taking us in the right direction! A few suggestions from my side:

• integreat-cms/integreat_cms/cms/views/pages/page_form_view.py

  Lines 321 to 341 in 2856953

  	if not page_form.is_valid() or not page_translation_form.is_valid():
  	# Add error messages
  	page_form.add_error_messages(request)
  	page_translation_form.add_error_messages(request)
  	elif not request.user.has_perm(
  	"cms.publish_page_object",
  	page_form.instance,
  	) and (
  	page_translation_form.cleaned_data.get("status")
  	in [status.DRAFT, status.PUBLIC]
  	):
  	# Raise PermissionDenied if user wants to publish page but doesn't have the permission
  	raise PermissionDenied(
  	f"{request.user!r} does not have the permission to publish {page_form.instance!r}",
  	)
  	elif (
  	page_translation_form.instance.status == status.AUTO_SAVE
  	and not page_form.has_changed()
  	and not page_translation_form.has_changed()
  	):
  	messages.info(request, _("No changes detected, autosave skipped"))

  
  could maybe be refactored to a method like check_page_save_valid or something similar? Then what is now in the else block would be a bit more explicit like:
  if self.check_page_save_valid(): Perhaps there is a better name, but you get what I mean.

• integreat-cms/integreat_cms/cms/views/pages/page_form_view.py

  Lines 430 to 452 in 2856953

  	# Add the success message and redirect to the edit page
  	if not page_instance:
  	if page_translation_form.instance.status == status.REVIEW:
  	messages.success(
  	request,
  	_(
  	'Page "{}" was successfully created and submitted for approval',
  	).format(page_translation_form.instance.title),
  	)
  	else:
  	messages.success(
  	request,
  	_('Page "{}" was successfully created').format(
  	page_translation_form.instance.title,
  	),
  	)
  	elif (
  	not page_form.has_changed() and not page_translation_form.has_changed()
  	):
  	messages.info(request, _("No changes detected, but date refreshed"))
  	else:
  	# Add the success message
  	page_translation_form.add_success_message(request)

  
  could perhaps be refactored to self.handle_messages()?

That are the things that came to my mind, but I haven't completely thought them through, so feel free to disagree. :)

[JoeyStk]

@jonbulz I really liked your suggestions! I especially loved the first one! It was something that had bothered me too, but I didn't know how it could be different :)

Please find my commits and feel free to re-review! :)

[MizukiTemma]

@JoeyStk
Thank you for tackling this problem 💪

I'm not against the current implementation, but would prefer an approach like #4045 . Is there a reason you went for another way? (Just asking, as I don't know whether @svenseeberg and you had any exchange about this topic)

[svenseeberg]

but would prefer an approach like #4045

I suppose you meant to refer to another PR? #4045 is exactly this one.

as I don't know whether @svenseeberg and you had any exchange about this topic

None specifically related to this PR that I can remember.

[JoeyStk]

I suppose you meant to refer to another PR? #4045 is exactly this one.

Yes, I think #4046 was meant :)

I'm not against the current implementation, but would prefer an approach like #4045 . Is there a reason you went for another way?

So this PR and #4046 are doing different things. In the end we also want an approach like #4046 for pages, but first Peter and I wanted to refactor pages, so it becomes easier for us to implement the new approach like in #4046. So this is one step before what is done in #4046 :) I hope that clarifies it :)

[MizukiTemma]

@JoeyStk @svenseeberg

I suppose you meant to refer to another PR? #4045 is exactly this one.

Yes, I think #4046 was meant :)

Ups, yes, I meant #4046 🙈

So this PR and #4046 are doing different things. In the end we also want an approach like #4046 for pages, but first Peter and I wanted to refactor pages, so it becomes easier for us to implement the new approach like in #4046. So this is one step before what is done in #4046 :) I hope that clarifies it :)

Thank you for explanation 👍

[MizukiTemma]

Very cool refactoring 😻 Thank you so much 🚀

[jonbulz]

Looks great, thank you very much!