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

Fix deleted blocks (from composer) still visible #1293

Merged
merged 1 commit into from Apr 18, 2021
Merged

Fix deleted blocks (from composer) still visible #1293

merged 1 commit into from Apr 18, 2021

Conversation

gremo
Copy link
Contributor

@gremo gremo commented Apr 8, 2021

Subject

I am targeting this branch, because this is a bugfix.

Closes #1292

Changelog

### Added
- Added `CreateSnapshotAdminExtension::postRemove()` method in order to create a snapshot when a block is deleted.

@haivala
Copy link
Contributor

haivala commented Apr 8, 2021

I think this is not needed. This is how it should work. You should be able choose when to publish the changes manually by doing for example timed publication. If this patch is for direct publication then go for it.

@VincentLanglet VincentLanglet requested a review from a team April 8, 2021 12:01
@gremo
Copy link
Contributor Author

gremo commented Apr 8, 2021

@haivala what?! This is how it should work? So, why it's working when you edit the block?

@haivala
Copy link
Contributor

haivala commented Apr 8, 2021

@haivala what?! This is how it should work? So, why it's working when you edit the block?

When you edited the block you were checking it probably with logged in user or you have direct publication set on? Either way anonymous user should always see only data from publications and the edits should not make publications unless you have direct publication setting on. Am I right?

@gremo
Copy link
Contributor Author

gremo commented Apr 8, 2021

@haivala as you can see here #1292 I'm using anonymous tab, so in the "black" Chrome window user is not logged.

I use direct publication YES - that means that:

  • Adding a block should create a publication - WORKING
  • Editing a block should create a publication - WORKING
  • Deleting a block should create a publication - NOT WORKING <-- this is what this PR is trying to accomplish

If you think that my PR breaks something when direct publication is OFF, let's talk about it:

  • CreateSnapshotAdminExtension is going to publish the "create a snapshot message"
  • When direct_publication is off, sonata.page.admin.extension.snapshot service (that is, the subject of this PR) is removed by the extension, so no message is dispatched, so snapshot is not created

Am I right?

@haivala
Copy link
Contributor

haivala commented Apr 8, 2021

You are right!
Thank you for making this clear!

VincentLanglet
VincentLanglet previously approved these changes Apr 8, 2021
@VincentLanglet VincentLanglet requested a review from a team April 8, 2021 14:17
@gremo
Copy link
Contributor Author

gremo commented Apr 10, 2021

I'ts OK to merge @VincentLanglet ?

@VincentLanglet
Copy link
Member

I don't use this bundle, so I'd like to get another review. Please take a look @sonata-project/contributors

@OskarStark OskarStark requested a review from core23 April 11, 2021 05:35
Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

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

Could we have some tests covering this fix?

@gremo
Copy link
Contributor Author

gremo commented Apr 11, 2021

Could we have some tests covering this fix?

Yes I can try if there is already some kind of testing for the block admin estension.

@franmomu
Copy link
Member

Yes I can try if there is already some kind of testing for the block admin estension.

Actually, there is:
https://github.com/sonata-project/SonataPageBundle/blob/591a224473caee21233562f9935800bd32b0c38a/tests/Admin/Extension/CreateSnapshotAdminExtensionTest.php

@gremo
Copy link
Contributor Author

gremo commented Apr 18, 2021

Added tests, please merge (maybe release?) thank youuuu

jordisala1991
jordisala1991 previously approved these changes Apr 18, 2021
@franmomu franmomu merged commit 9964f8b into sonata-project:3.x Apr 18, 2021
@franmomu
Copy link
Member

Thanks @gremo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting a block from the page composer doesn't update the site
7 participants