Refactor get in page_form_view

[JoeyStk]

Short description

After refactoring the post method of the the page_form_view, I decided to also refactor the get method.
Similarly to the previous PR, I also used the cyclomatic complexity as a metric to track progress. This time I was able to reduce the CC by half (from 18 (=C) to 9 (=B)).

Proposed changes

• Refactor get method in page_form_view

Side effects

• I'm not entirely happy with the method yet, but think it's a good step forward. Let me know if there is still something left in the method that you think should be refactored
• I'm not 100% sure if I really caught every test case, so thorough testing is very welcome

Faithfulness to issue description and design

There are no intended deviations from the issue and design.

How to test

Resolved issues

Fixes: /

---

Pull Request Review Guidelines

[MizukiTemma]

Looks good 😸

[jonbulz]

@JoeyStk could you rebase so that we get a test run, now that #4069 is merged?

[JoeyStk]

@jonbulz This PR is ready for review again :)

[jonbulz]

Thank you for your work, I think this makes things a lot cleaner! I approve because I agree with the concepts and separation of logic. However, I have a few comments regarding the naming. But naming is hard and always subjective, so I wouldn't say that my suggestions are better. I just wanted to give some food for thought :)

[jonbulz]

The value that is returned here is never used. If this is just to handle warning messages if the page is archived, I'd prefer renaming the method to something more fitting

[JoeyStk]

Maybe I'm not understand the code correctly, but the way I see it is, that the value is given to the PageForm in line 112 to disable the form :)

[JoeyStk]

Would be check_if_initially_disabled be a better name? :)

[jonbulz]

The disable variable that is declared here is never used and completely overruled by the disabled that is later declared by check_is_disabled
IMO, there are 2 possible conclusions:

1. What we want is actually not what's happening: Say get_initial_disable_state returns True because the page is archived, but check_is_disabled returns False because the user has the appropriate permissions, then disabled will just be False after line 110. If it should still be True because the page is archived, we need some kind of and to ensure this behavior.
2. If what happens currently in the above mentioned case IS what we want, then what is returned by get_initial_disable_state doesn't matter, it does not affect the state of the page form, but instead only adds some extra messages. If that is the case, then we should rename the method accordingly

[JoeyStk]

Ah, now I get it. I'm sorry it took me a minute. I think this should be fixed now :). Could you please have another look at it? :)

[JoeyStk]

@jonbulz do you think you could have another look at this? :)

[jonbulz]

The disable variable that is declared here is never used and completely overruled by the disabled that is later declared by check_is_disabled
IMO, there are 2 possible conclusions:

1. What we want is actually not what's happening: Say get_initial_disable_state returns True because the page is archived, but check_is_disabled returns False because the user has the appropriate permissions, then disabled will just be False after line 110. If it should still be True because the page is archived, we need some kind of and to ensure this behavior.
2. If what happens currently in the above mentioned case IS what we want, then what is returned by get_initial_disable_state doesn't matter, it does not affect the state of the page form, but instead only adds some extra messages. If that is the case, then we should rename the method accordingly