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

Adds code to update delete site explanation when a site has MTM container, #PG-3574 #846

Merged
merged 12 commits into from
Sep 5, 2024

Conversation

AltamashShaikh
Copy link
Contributor

@AltamashShaikh AltamashShaikh commented Aug 7, 2024

Description:

Adds code to update delete site explanation when a site has MTM container
Requires: matomo-org/matomo#22484
Fixes: #PG-3574

Review

Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Looks good overall. I had a couple comments.
When I go to delete a site, it shows the enter password string twice:
image

Edit:
It took rebuilding the Vue files for SitesManager, but I got the view to load as expected. So, functional testing passed 👍

templates/deleteWebsite.twig Show resolved Hide resolved
Copy link
Contributor

@araichyk araichyk left a comment

Choose a reason for hiding this comment

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

Functionality works as expected. It even gracefully handles edge case of 100 containers in the "will be deleted" list. Overall implementation looks good 👍

@AltamashShaikh
Copy link
Contributor Author

@snake14 Did anything changed in SitesManager plugin after vue:build?

It took rebuilding the Vue files for SitesManager, but I got the view to load as expected. So, functional testing passed

@snake14
Copy link
Contributor

snake14 commented Aug 8, 2024

@snake14 Did anything changed in SitesManager plugin after vue:build?

It took rebuilding the Vue files for SitesManager, but I got the view to load as expected. So, functional testing passed

@AltamashShaikh I don't see any change in SitesManager, so it must have just forced the cache even though I already tried clearing the cache via console command 🤔

Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Looks good and functional testing worked as expected 👍

Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Looks good. Confirmed that the message displays as expected while deleting a site from my local instance 👍

lang/en.json Outdated Show resolved Hide resolved
@AltamashShaikh
Copy link
Contributor Author

@snake14 Core PR is merged, can you review the PR once again ?

Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Looks good. I updated to the latest version of Core and this branch. Everything displayed as expected 👍

@AltamashShaikh AltamashShaikh merged commit 5591eac into 5.x-dev Sep 5, 2024
4 checks passed
@AltamashShaikh AltamashShaikh deleted the PG-3574-delete-website-message branch September 5, 2024 03:18
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.

4 participants