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.

Sign up for GitHub

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

Jump to bottom

Fixes #36577 - Filter gets applied to all the repository upon removal #10640

Merged
merged 2 commits into from
Jul 19, 2023
Merged

Fixes #36577 - Filter gets applied to all the repository upon removal #10640

merged 2 commits into from
Jul 19, 2023

Conversation

sjha4
Copy link
Member

@sjha4 sjha4 commented Jul 11, 2023

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

  1. Create a couple of repos and add them to filters.
  2. Go to repo > Remove repository
  3. You should see a section in the deletion confirmation modal if the repo is the last repo that a filter is applied to.
  4. You should be able to choose to delete the filter/or make it apply to all repositories in the CV depending on your selection.
  5. This section shouldn't appear for repos that are not the last affected repo in any filters.

@theforeman-bot
Copy link

Issues: #36577

@sjha4 sjha4 force-pushed the remove_filters_repo_deletion branch from 5ff63ce to 1b60b4d Compare July 11, 2023 19:55
jeremylenz
jeremylenz reviewed Jul 12, 2023
View reviewed changes
Copy link
Member

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

just a few wording suggestions :)

app/controllers/katello/api/v2/repositories_controller.rb Outdated Show resolved Hide resolved
app/lib/actions/katello/repository/destroy.rb Outdated Show resolved Hide resolved
...ascripts/bastion_katello/products/details/repositories/details/views/repository-details.html Outdated Show resolved Hide resolved
@sjha4 sjha4 force-pushed the remove_filters_repo_deletion branch from 1b60b4d to aa9c1a8 Compare July 12, 2023 14:45
@sjha4 sjha4 force-pushed the remove_filters_repo_deletion branch from aa9c1a8 to dc20aaa Compare July 12, 2023 17:03
@sjha4 sjha4 force-pushed the remove_filters_repo_deletion branch from dc20aaa to 1f4455a Compare July 12, 2023 18:43
@sjha4
Fixes #36577 - Filter gets applied to all the repository upon removal…
5c99a80 
… of repository for which the filter was created.
@sjha4 sjha4 force-pushed the remove_filters_repo_deletion branch from 1f4455a to 5c99a80 Compare July 12, 2023 18:48
jeremylenz
jeremylenz reviewed Jul 12, 2023
View reviewed changes
Copy link
Member

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

When clicking Remove Repository from the repo details screen / Select Action,
✔️ I see the section added to the modal when it's the last repo
✔️ I don't see the section added to the modal when it's not the last repo

One question.. When selecting a repo from the repo list screen and clicking Remove Repositories, I don't see anything special. Is this intended?

image

...ion_katello/test/products/details/repositories/details/repository-details.controller.test.js Outdated Show resolved Hide resolved
@sjha4
Copy link
Member Author

sjha4 commented Jul 13, 2023

Good point! For bulk actions, we don't check if individual selected repos are part of published versions or this use case..For CV versions that is fine because we fail the deletion for that repo. For filters we would default to deleting the filter.

@jeremylenz
Copy link
Member

For filters we would default to deleting the filter.

Have we always deleted the filter in this case? Or this PR changes that behavior?

If it's a change, should probably add some notice to that modal.

@sjha4
Copy link
Member Author

sjha4 commented Jul 13, 2023

For filters we would default to deleting the filter.

Have we always deleted the filter in this case? Or this PR changes that behavior?

If it's a change, should probably add some notice to that modal.

We do change the behavior. Earlier it would make the filter apply to all repos..

@sjha4 sjha4 force-pushed the remove_filters_repo_deletion branch from f31c229 to fb8a914 Compare July 13, 2023 18:24
@sjha4 sjha4 force-pushed the remove_filters_repo_deletion branch from fb8a914 to d820de9 Compare July 13, 2023 19:44
@sjha4 sjha4 force-pushed the remove_filters_repo_deletion branch from d820de9 to b2de292 Compare July 14, 2023 13:48
@sjha4 sjha4 requested a review from jeremylenz July 14, 2023 17:19
jeremylenz
jeremylenz reviewed Jul 17, 2023
View reviewed changes
Copy link
Member

@jeremylenz jeremylenz left a comment •
edited

Choose a reason for hiding this comment

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

I was expecting a different message on both the bulk product delete confirmation and the bulk repository delete confirmation modals, but I don't see them.

See my question/suggestion below about auto-deleting filters during bulk delete. Maybe we should change the behavior as per that, then update these modals to describe it. Thoughts?

...ts/javascripts/bastion_katello/products/details/repositories/views/product-repositories.html Outdated Show resolved Hide resolved
...n_katello/app/assets/javascripts/bastion_katello/products/details/views/product-details.html Outdated Show resolved Hide resolved
app/lib/actions/katello/product/destroy.rb Show resolved Hide resolved
@sjha4 sjha4 force-pushed the remove_filters_repo_deletion branch from b2de292 to 51f542d Compare July 17, 2023 17:12
@sjha4 sjha4 force-pushed the remove_filters_repo_deletion branch from 51f542d to 5d3cbac Compare July 17, 2023 19:46
jeremylenz
jeremylenz approved these changes Jul 18, 2023
View reviewed changes
Copy link
Member

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

thanks @sjha4!

ACK 👍

@sjha4 sjha4 merged commit 49a55b1 into Katello:master Jul 19, 2023
4 of 5 checks passed
wbclark pushed a commit to wbclark/katello that referenced this pull request Sep 7, 2023
@sjha4 @wbclark
Fixes #36577 - Filter gets applied to all the repository upon removal (…
52146d1 
…Katello#10640)

* Fixes #36577 - Filter gets applied to all the repository upon removal of repository for which the filter was created.

* Fixes #36577 - Handle bulk repo and product deletion

(cherry picked from commit 49a55b1)
wbclark pushed a commit that referenced this pull request Sep 27, 2023
@sjha4 @wbclark
Fixes #36577 - Filter gets applied to all the repository upon removal (…
2573d09 
…#10640)

* Fixes #36577 - Filter gets applied to all the repository upon removal of repository for which the filter was created.

* Fixes #36577 - Handle bulk repo and product deletion

(cherry picked from commit 49a55b1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremylenz jeremylenz jeremylenz approved these changes

Assignees
No one assigned
Labels
Not yet reviewed Waiting on contributor
Projects
None yet
Milestone
No milestone
3 participants
@sjha4 @theforeman-bot @jeremylenz