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

Segment deletion validation. #6566

Merged
merged 3 commits into from Mar 4, 2019

Conversation

mitresh95
Copy link
Contributor

Description:

We need to validate if some other entity depends on the segment being deleted. Currently there is no validation which leads to segment processing errors when dependent segments are deleted.

Steps to reproduce the bug:

  1. Apply membership filters on a segment.
  2. Attempt to delete the segment on which the previous segment was dependent. It will be deleted.

Steps to test this PR:

  1. Checkout to the branch in PR.
  2. Follow the same steps as to reproduce the issue and now it would prevent the deletion and provide the flash message.

@npracht
Copy link
Member

npracht commented Sep 14, 2018

Hi @mitresh95 this is an awesome improvement !!
Does it also work if the segment is using in a campaign or a segment email or report filter ?

@npracht npracht added user-interface Anything related to appearance, layout, and interactivity ready-to-test PR's that are ready to test enhancement Any improvement to an existing feature or functionality code-review-needed PR's that require a code review before merging labels Sep 14, 2018
@kuzmany kuzmany self-requested a review September 19, 2018 18:56
@kuzmany kuzmany added this to the 2.14.2 milestone Sep 19, 2018
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Doesn' work for me. Tested with Mautibox https://mautibox.com/6566/s/segments

If I use delete button, segment deleted, but no flash message.
If I use batch delete, there is flash message (translations missing), but segment is delete either.

image

@kuzmany kuzmany added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-test PR's that are ready to test labels Sep 19, 2018
@mitresh95
Copy link
Contributor Author

Hi @npracht Thank you! And sorry for the delayed response. So if you mean to say that if a contact segment is used in the Campaigns or else, then no, right now this feature just checks for segment inter-dependency. So while deletion it will be only checked with other segments weather or not other segment/s are dependent on it.

@mitresh95
Copy link
Contributor Author

@kuzmany This is strange. Checking out the branch in my local environment and testing it gave me proper results. Let me look it up and see what is wrong here. Thanks!

@npracht
Copy link
Member

npracht commented Sep 27, 2018

@mitresh95 it would be awesome that it is checked in any other type of entity (points, campaigns, email, etc.)

@mleffler
Copy link
Contributor

Agree, Norman, but this is a step in the right direction :). Thx for commenting.

@mleffler mleffler removed the code-review-needed PR's that require a code review before merging label Sep 28, 2018
@escopecz escopecz added this to Pending Feedback in Testing 2.14.2 Oct 2, 2018
@escopecz escopecz added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Oct 15, 2018
@escopecz escopecz modified the milestones: 2.14.2, 2.15.0 Oct 16, 2018
@npracht npracht added this to Pending feedback in Testing 2.15.0 Oct 16, 2018
@mleffler
Copy link
Contributor

mleffler commented Nov 9, 2018

@kuzmany let us know if this has resolved in mautibox. We are running it and it is working fine.

@mleffler
Copy link
Contributor

mleffler commented Nov 9, 2018

@mitresh95 Pls fix the conflicts.

@mleffler mleffler moved this from Pending feedback to To do in Testing 2.15.0 Nov 20, 2018
@mleffler mleffler removed has-conflicts Pull requests that cannot be merged until conflicts have been resolved pending-feedback PR's and issues that are awaiting feedback from the author labels Nov 20, 2018
@kuzmany
Copy link
Member

kuzmany commented Nov 22, 2018

@mleffler Maybe I am wrong, but I didn't see translation for mautic.lead.list.error.cannot.delete.batch anyway in this PR or in actual staging. I am not sure If the PR is committed complete

@kuzmany
Copy link
Member

kuzmany commented Nov 22, 2018

Also as I see the code apply just for batchDeleteAction()
https://github.com/mautic/mautic/pull/6566/files#diff-a22caf80768ad2745ddc23b23520ab94L531

Where is the check for deleteAction() ?

@kuzmany kuzmany added the pending-feedback PR's and issues that are awaiting feedback from the author label Nov 22, 2018
@kuzmany
Copy link
Member

kuzmany commented Nov 22, 2018

Also there is no return if toBeDeleted is empty
https://github.com/mautic/mautic/pull/6566/files#diff-a22caf80768ad2745ddc23b23520ab94R539

Code continue to delete entities and info about success. But there is no success in that case

@mitresh95
Copy link
Contributor Author

This will be taken care of for 2.15.1. Sorry for the delay.

@Woeler Woeler modified the milestones: 2.15.0, 2.15.1 Dec 3, 2018
@Woeler Woeler removed this from Pending feedback in Testing 2.15.0 Dec 3, 2018
@heathdutton heathdutton added this to Code Review (2 required) in Mautic 2 Dec 6, 2018
@heathdutton heathdutton moved this from Code Review (2 required) to Changes Requested in Mautic 2 Dec 6, 2018
@alanhartless alanhartless added this to Discussion in 2.15.1 Jan 14, 2019
@alanhartless
Copy link
Contributor

@kuzmany @npracht this is good be tested again! Sorry we left out part :-)

@alanhartless alanhartless added ready-to-test PR's that are ready to test and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Jan 15, 2019
@alanhartless alanhartless moved this from Changes Requested / Review to Ready to Test (first time) in Mautic 2 Jan 15, 2019
@alanhartless alanhartless moved this from Discussion to Needs Testing in 2.15.1 Jan 15, 2019
@npracht
Copy link
Member

npracht commented Jan 17, 2019

This PR must be very helpful to prevent the issue mentioned in #6871. @johbuch can you test that please ?

@alanhartless alanhartless added the pending-test-confirmation PR's that require one test before they can be merged label Jan 30, 2019
@alanhartless alanhartless moved this from Needs Testing to Needs Second Test in 2.15.1 Jan 30, 2019
Copy link
Member

@npracht npracht left a comment

Choose a reason for hiding this comment

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

Just tested and it works. Great way to improve UX and side effect issues.

@npracht npracht added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged ready-to-test PR's that are ready to test labels Feb 6, 2019
@alanhartless alanhartless merged commit 2ddde70 into mautic:staging Mar 4, 2019
Mautic 2 automation moved this from Ready to Test (first time) to Merged Mar 4, 2019
@alanhartless alanhartless moved this from Needs Second Test to Merged in 2.15.1 Mar 4, 2019
@escopecz escopecz removed their assignment Mar 6, 2019
@alanhartless alanhartless deleted the segment-delete-validation branch April 18, 2019 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any improvement to an existing feature or functionality ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged user-interface Anything related to appearance, layout, and interactivity
Projects
No open projects
2.15.1
Merged
Testing 2.14.2
  
Pending Feedback
Development

Successfully merging this pull request may close these issues.

None yet

7 participants