Update page link before submitting page form

[charludo]

Short description

I had a bit of trouble reproducing the issue. The only way I could trigger it was by entering Firefox's Responsive Design Mode and enabling Touch Simulation (though I suspect this might also happen on real touch devices).

I don't think this is worth an entry in the change log, this really is an edge case.

Proposed changes

• if the content form is submitted while we're still waiting on a response for slugify, prevent the submission, then trigger it again once the previous call completed.

Side effects

• hopefully not

Resolved issues

Fixes: #2624

---

Pull Request Review Guidelines

[codeclimate]

Code Climate has analyzed commit 4113830 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 92.8% (50% is the threshold).

This pull request will bring the total coverage in the repository to 82.7% (0.0% change).

View more on Code Climate.

[seluianova]

though I suspect this might also happen on real touch devices

Right, I was able to reproduce the issue using the touch screen.

As I can see now, the proposed changes fix that! 🚀
I think this can be applied to events and locations as well.

UPD.
And the same problem occurs when saving to draft or submitting for approval.

[charludo]

I think this can be applied to events and locations as well.

UPD. And the same problem occurs when saving to draft or submitting for approval.

Right, thanks for catching that!

I've added a more general solution.

[seluianova]

Looks good to me, thanks!

[seluianova]

Opps, sorry, I actually found one issue:
"Submit for approval" action has been broken for Observer

It can be reproduced, if Observer changes the page title.

[charludo]

Opps, sorry, I actually found one issue: "Submit for approval" action has been broken for Observer
[...]
It can be reproduced, if Observer changes the page title.

Sorry for taking so long to get back to you.

Looks like this also happens on the develop branch, so it's unrelated to the changes. However, I think it might be intentional - @osmers, should Observers be able to change titles of pages they can edit?

If yes, I can open a new issue;
if no, I'd use this PR to set the field disabled for Observers, if that's all right with both of you.

[seluianova]

Looks like this also happens on the develop branch, so it's unrelated to the changes

I have re-checked:
The error on the screen is reproduced on develop, so it's really unrelated.
But the action itself works on develop - I can submit a title change for approval now.

But right, if the title field should be disabled, it will solve the problem anyway :)

[charludo]

But the action itself works on develop - I can submit a title change for approval now.

Ah, right. Then I guess it's not unrelated anymore, I'll look into it :D

[charludo]

Looks like this also happens on the develop branch, so it's unrelated to the changes

I have re-checked: The error on the screen is reproduced on develop, so it's really unrelated. But the action itself works on develop - I can submit a title change for approval now.

But right, if the title field should be disabled, it will solve the problem anyway :)

AH, I mistakenly thought Observers could not change the title, but in fact it's just that they can't directly change the link (so the automatic link update is, of course, not an issue at all here.

I've removed the submission prevention for "submit for approval".

[osmers]

Looks like this also happens on the develop branch, so it's unrelated to the changes

I have re-checked: The error on the screen is reproduced on develop, so it's really unrelated. But the action itself works on develop - I can submit a title change for approval now.
But right, if the title field should be disabled, it will solve the problem anyway :)

AH, I mistakenly thought Observers could not change the title, but in fact it's just that they can't directly change the link (so the automatic link update is, of course, not an issue at all here.

I've removed the submission prevention for "submit for approval".

I think it makes sense that observers can change the title of the page they are allowed to edit. Either they were given the rights to publish right away which means they are trusted to only do what's expected or they can only submit for review, in which case the changes to the title could be rejected if not wanted, right?

[seluianova]

Observer can now submit a title change for approval (the same behavior as in develop) ✅
But the original problem remains for the "Submit for approval" action:

• login as Author (the problem is that not only Observers submit changes for approval)
• edit some page title
• click 'Submit for approval' using the touch screen
  --> the page is saved with a new title, but the link is not updated

I mistakenly thought Observers could not change the title, but in fact it's just that they can't directly change the link

Actually it seems they can change a link directly (submit a link change for approval), I have just checked in develop.

[charludo]

I mistakenly thought Observers could not change the title, but in fact it's just that they can't directly change the link

Actually it seems they can change a link directly (submit a link change for approval), I have just checked in develop.

Alright. I think everythin is working now the same as on develop, plus touch screens work. However, I have not fixed the underlying bug (which is that the slugify_ajax endpoint raises PermissionDenied for observers because they are not allowed to edit objects), but instead just moved the release of the submission lock to a finally() block. This way the fetch still throws an unhandled error, just like on develop.

I think we should either

• keep the PermissionDenied but handle it "properly" (aka hide it / prevent logging to console), or
• change the required permission in slugify_ajax.py to view_page etc.

I like the second option better, but wasn't sure if there's some unintended consequences.

[JoeyStk]

Thank you very much! Looks really good! I really would like to have a test for this, but since we don't have JS tests yet, I think we can give it a pass

[charludo]

Thank you very much! Looks really good! I really would like to have a test for this, but since we don't have JS tests yet, I think we can give it a pass

Yes, fair 😅 Any suggestions on how to handle the PermissionDenied?

[JoeyStk]

Oh sorry, I forgot to mention that. I also tend towards the second option. I think the PermissionDenied indicates that somewhere we have a logical thinking error in our permissions. I think changing the permission should be the right thing 🤔

[seluianova]

change the required permission in slugify_ajax.py to view_page etc.

Sounds better to me, because if Observer has the right to modify the link directly, then modifying the link should also work for Observer through ajax. I think that would be more consistent.

[charludo]

Alright, I've lowered the required permissions.

"Worst case" someone can generate a slug for a content objects they shouldn't - but since the function only returns the generated slug, not actually saves it, I think that's OK.

I have also let the client-side error handling in, just in case anything else occurs server-side we still release the submission lock.

[seluianova]

"Worst case" someone can generate a slug for a content objects they shouldn't

This could be eliminated if we check for "cms.change_page_object" permission instead of "is user.role == Observer".
This will also make the check more accurate, because the slug can actually be edited when the user has permissions to a particular page and being an Observer is not enough.
But it will require some re-work of slugify_ajax() function. Not sure it's worth it, given the criticality of the original issue.

Smthg like:

request.user.has_perm("cms.change_page_object", object_instance.page)

But I don't insist here, let me know if you would prefer to keep the current implementation.

[charludo]

"Worst case" someone can generate a slug for a content objects they shouldn't

This could be eliminated if we check for "cms.change_page_object" permission instead of "is user.role == Observer". This will also make the check more accurate, because the slug can actually be edited when the user has permissions to a particular page and being an Observer is not enough. But it will require some re-work of slugify_ajax() function. Not sure it's worth it, given the criticality of the original issue.

Smthg like:

request.user.has_perm("cms.change_page_object", object_instance.page)

But I don't insist here, let me know if you would prefer to keep the current implementation.

You're absolutely right, that makes a lot more sense. Tbh I completely forgot that checking permissions for a specific object is an option... 😅

[seluianova]

I've re-tested everything again and found no new problems except the one caused by removing the if-condition in the templates.
As soon as the if-s are returned, I think it's good to go 🤞
Thanks for tackling this tricky issue!

[charludo]

I've re-tested everything again and found no new problems except the one caused by removing the if-condition in the templates. As soon as the if-s are returned, I think it's good to go 🤞 Thanks for tackling this tricky issue!

No, thank YOU for the thorough reviews and attention to detail!!

Seriously, without you, this PR might have solved the original issue, but would also have introduced multiple new bugs. Thank you 💛