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

4634 - Add the Delete button on the Partner Group Page #4649

Merged

Conversation

Aaryanpal
Copy link
Contributor

@Aaryanpal Aaryanpal commented Sep 12, 2024

Resolves #4634

Description

Add the Delete Button Functionality in User group that doesn't have partners in it

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Created the New Parent-Group and Deleted the User Group by clicking on the Alert window
  • On Pre-Existing Parent-Group that has partners Delete button is disable
  • Added Automated Test To Support it

ScreenCast

Screencast.from.2024-09-13.00-04-23.webm

@cielf cielf self-requested a review September 12, 2024 19:07
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Other than the one wording change ("destroyed" to "deleted") mentioned in an earlier comment, it looks good functionally.

@cielf cielf requested a review from dorner September 12, 2024 19:24
@Aaryanpal Aaryanpal force-pushed the 4634_add_ability_to_remove_partner_group branch from a88cf00 to bccadb5 Compare September 13, 2024 08:13
@Aaryanpal
Copy link
Contributor Author

Other than the one wording change ("destroyed" to "deleted") mentioned in an earlier comment, it looks good functionally.

Donw With changes @cielf

Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

app/controllers/partner_groups_controller.rb Outdated Show resolved Hide resolved
app/views/partners/_partner_groups_table.html.erb Outdated Show resolved Hide resolved
spec/system/partner_system_spec.rb Outdated Show resolved Hide resolved
spec/system/partner_system_spec.rb Outdated Show resolved Hide resolved
@Aaryanpal Aaryanpal force-pushed the 4634_add_ability_to_remove_partner_group branch from 069ee84 to 81655f3 Compare September 14, 2024 00:59
sign_in(user)
end

describe "DELETE #destroy" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a spec that checks that the delete button is only shown if there are no partners. This can still be a request spec since you can check the response HTML.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also - please don't force push your branch once a review has started - it becomes harder to see what has changed since the last review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hii @dorner,
I've updated the spec based on your suggestion. I had to force-push the changes since I couldn't see my updates in the branch. Please ignore this for now. I'll work on improving it in future PRs

@Aaryanpal Aaryanpal force-pushed the 4634_add_ability_to_remove_partner_group branch from 8fdb6d5 to 2e6f4a3 Compare September 19, 2024 14:05
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

ok by me,

@dorner
Copy link
Collaborator

dorner commented Sep 20, 2024

Looks good on my end!

@dorner dorner merged commit fe27da4 into rubyforgood:main Sep 20, 2024
11 checks passed
@Aaryanpal Aaryanpal deleted the 4634_add_ability_to_remove_partner_group branch September 22, 2024 11:29
Copy link
Contributor

@Aaryanpal: Your PR 4634 - Add the Delete button on the Partner Group Page is part of today's Human Essentials production release: 2024.09.22.
Thank you very much for your contribution!

jlandiseigsti pushed a commit to jlandiseigsti/human-essentials that referenced this pull request Oct 2, 2024
)

* Add the Delete button on the Partner Group Page

* Replace Destroyed with Deleted

* Resolve Comments

* Added Request Spec for PartnerGroup and remove corresponding system-spec

* Fix lint

* fix the request-spec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to remove (empty) partner groups
3 participants