Bugfix v4: Structure mode toggle button disappearing from toolbar #7272
Conversation
… toolbar to the backend
…quest_pass or the toolbar will be wrongly configured
|
Additional fix in
|
| @@ -1070,12 +1068,17 @@ class StructureBoard { | |||
| const placeholderIds = getPlaceholderIds(CMS._plugins).map(id => `placeholders[]=${id}`).join('&'); | |||
|
|
|||
| return $.ajax({ | |||
| url: Helpers.updateUrlWithPath( | |||
| url: ( | |||
There was a problem hiding this comment.
What were you thoughts when removing: Helpers.updateUrlWithPath?
There was a problem hiding this comment.
Helpers.updateUrlWithPath is the root cause of the bug. It replaces query separator& by &s. Since it is used in many places within django CMS, I did not change it. I cannot anticipate the side effects.
In any case: Helpers.updateUrlWithPath('/local_url?x=1&y=2') returns '/local_url?x=1&y=2&cms_path=%2Fadmin%2Fcms%2Fplaceholder%2Fobject%2F10%2F' which is not a meanigful path to get the toolbar for. The fix in the same situation creates a meaningful url: '/local_url?x=1&y=2&cms_path=%2Fadmin%2Fcms%2Fplaceholder%2Fobject%2F10%2F'
Frankly, I still struggle to comprehend the implementation of Helpers.makeUrl which is called by Helpers.updateUrlWithPath:
django-cms/cms/static/cms/js/modules/cms.base.js
Lines 291 to 308 in 914558d
Line 305 is highly questionable if you have more than one get parameter!
There was a problem hiding this comment.
@Aiky30 An alternative way would be to drop the urljs dependence and use the browsers' URLSearchParams interface. This would allow to have Helpers.updateUrlWithPath to be present. It should build correct urls. But it also requires a small change in cms.pagetree.js. Overall this is probably cleaner but a be it small change deep in the code.
If you think that is better, I'll give it a go!
There was a problem hiding this comment.
After more pondering I come to the conclusion to essentially remove lines 303-307 in makeURL: It seems to never have worked right with more than one query parameter. Overall, this deems to be the most systematic solution.
I still have to make the discrimination test for Resolver404 work.
…ne query parameter is given. Needs to work with many
|
@Aiky30 The resolver test ist implemented and works. I have refactored the patch to change the underlying Test action fails when testing the documents build (which incidentally works in the separate automation). The reason seems to be sphinx 1.8 importing incorrectly from jinja2. Certainly nothing to do with this PR. I suggest to remove the fix to Sphinx==1.8.x in the requirements and backport the changes f3285b6 to the documentation ( |
|
@Aiky30 What are your thoughts on getting this merged? |
|
@fsbraun I haven't forgotten about your changes here. The makeUrl function replacing & with & appears to have been a quick and dirty workaround here: #3404. I guess you may already know this. Once I understand the reason for it's existence I'll be able to approve your rework. My fear right now is any regression, especially if this is used in other parts of the CMS / external packages. |
|
The original bug that needs a test case for, this then ensures that we don't reintroduce the same bug for someone to then undo these changes forever going in a cycle: #3434 |
|
A frontend test that replicates this bug: #3434 Then we can see with these changes if that issue has been reintroduced. |
|
I understand your concerns. The "quick and dirty workaround" from 2014 does not work in the first place, it does break every url with more than one parameter as of Django 2.2.19. See nichoski's discussion in #7113. In Django < 2.2.19 things only work accidentally. The underlying jQuery bug has been fixed. I did not research the exact version but tested it with CMS' jQuery verision. To mitigate your concerns please be aware that this has also been corrected and in the field with v3.10 (PR #7114). Finally, also "Add page type" works in 3.10 with the proposed fix. In v4 page types are gone, hence nothing to test there. |
@fsbraun I could see the change was made to django-cms 3.10 and I guess as you mentioned the issue was in an older release of django. PageType is still in v4 but it just hasn't been configured in to use the edit, preview and structure endpoints. |
|
After seeing the various tickets listing bugs and fixes I'm happy to allow this reverse of the fix. |
Reverse formatting change Co-authored-by: Aiky30 <Aiky30@users.noreply.github.com>
Correct formatting Co-authored-by: Aiky30 <Aiky30@users.noreply.github.com>
Comment above not at end of line Co-authored-by: Aiky30 <Aiky30@users.noreply.github.com>
Update method doc
Remove trailing space
There was a problem hiding this comment.
Looks good. Thanks.
We have a release queued for tomorrow morning first thing, that release will be merged back into develop-4 then I can merge your PR in to keep the order of changes correct.
The release queued for tomorrow morning: #7317
Once the above is complete we can merge this PR and the generate a release straight away to test on.
|
Thanks very much for taking the time. The plan sounds good!! Fabian
… Am 18.05.2022 um 12:43 schrieb Aiky30 ***@***.***>:
@Aiky30 approved this pull request.
Looks good. Thanks.
We have a release queued for tomorrow morning first thing, that release will be merged back into develop-4 then I can merge your PR in to keep the order of changes correct.
The release queued for tomorrow morning: #7317 <#7317>
Once the above is complete we can merge this PR and the generate a release straight away to test on.
—
Reply to this email directly, view it on GitHub <#7272 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEA7CHPZNFWVQUA5SKYLU53VKTCUTANCNFSM5ROPE22Q>.
You are receiving this because you were mentioned.
|
Description
When editing with the structure board open any change in the page, e.g. by moving a plugin to a new position, will make the toggle button in the toolbar disappear as @marksweb reported here: https://django-cmsworkspace.slack.com/archives/C01E6NMK540/p1645542305165849
Please note the empty upper right corner.
Related resources
The disappearing of the button happens in two steps:
The toolbar is reloaded after each editing step (I am not sure this is necessary, btw.)
If the page content has changed, the page is reloaded using xhr. The reload includes the toolbar.
The first step failed to send the current placeholder as get parameters since the url was encoded twice. As a result the backend returns the toolbar with just the create button and without edit button. This new toolbar is rendered in the browser window.
When the page reload delivers the correct toolbar, it is used to patch the current toolbar. This leads fails to restore the toggle button since there is no button to patch.
The fix removes the call to
Helpers.updateUrlWithPathwhich encodes the already encoded url a second time replacing the get parameter separator&by&.NOTE:
Helpers.updateUrlWithPathcan also change the url path according to some rules. This is not the case in the call affected (imho).The double xhr call to the backend is unnecessary and the first call can be dropped. This PR does that by rearranging the code. It leads to better responsiveness of the frontend editing experience.
This PR should be ported to v3 also. I expect it to solve issue #7222 .
Checklist
develop-4