Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent delete/disable CMS pages used in configuration #4239

Merged
merged 10 commits into from
Oct 15, 2024

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Oct 2, 2024

Reproduce

Try to disable/delete configured no-route page.

Related Pull Requests

  1. See Prevent delete/disable CMS pages used as default pages #2979

@github-actions github-actions bot added Component: Cms Relates to Mage_Cms translations Relates to app/locale labels Oct 2, 2024
@empiricompany
Copy link
Contributor

Isn't it better to put it in the controller?

Resource should not do these checks

@sreichel
Copy link
Contributor Author

sreichel commented Oct 2, 2024

There are different checks in resource, e.g. getIsUniquePageToStores.

Having it in model allows to use ... Mage::getModel('cms/page')->load(1)->getUsedInStoreConfigCollection()

@sreichel
Copy link
Contributor Author

sreichel commented Oct 4, 2024

Maybe a unit test for the helper method, but this can be done in another PR.

@addison74
Copy link
Contributor

addison74 commented Oct 4, 2024

I am testing this PR. For the 404 Not Found page (no-route).

  1. If I delete it the message is "Cannot delete page, it is used in "web/default/cms_no_route".

  2. If I disable it I am getting two messages "Cannot disable page, it is used in "web/default/cms_no_route" and "The page has been saved." This can be cofusing.

Another observation changing the URL Key will have a negative impact. For example, from no-route to no-route-2.

@sreichel
Copy link
Contributor Author

sreichel commented Oct 4, 2024

Having two messages is intended.

The page gets saved, but resets "is active" option to yes.

I have not tested changing url-key ... what happens? (I am not at home to test it)

@addison74
Copy link
Contributor

Changing the URL key of that page is similar to actions such as delete or disable, because once the URL is changed that page will no longer be used and therefore it will be a problem. My opinion is that it should not be allowed to change the URL as long as it is used.

url_key

@sreichel
Copy link
Contributor Author

sreichel commented Oct 4, 2024

Good idea.

I think there are other scenarios to cover ...

  • having multiple pages with same identifier for different stores
  • having different pages configured for different stores

@sreichel
Copy link
Contributor Author

sreichel commented Oct 5, 2024

Better leave it for another PR.

@addison74
Copy link
Contributor

Better leave it for another PR.

@sreichel - I agree. What else I would propose for this PR would be a different message, for example

You cannot delete or disable the page nor change the URL Key as long as it is used as a Default Page in the configuration.

One message covers all the situations and makes the things clear. Like a general warning what you cannot do.

@sreichel
Copy link
Contributor Author

sreichel commented Oct 5, 2024

One message covers all the situations

This will not work. There is a difference between them.

When deleting an exception is thrown (red message) and the "delete action" is aborted.

When disabling the "save action" continues, all other changes are applied, but active state is set back and a warning (yellow message) is left.

Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested

@sreichel sreichel merged commit 2111640 into OpenMage:main Oct 15, 2024
18 checks passed
@sreichel sreichel deleted the ref-2979 branch October 15, 2024 22:47
fballiano added a commit to MahoCommerce/maho that referenced this pull request Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Cms Relates to Mage_Cms translations Relates to app/locale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants