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

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

Merged
merged 6 commits into from
Sep 20, 2024
Merged

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

merged 6 commits into from
Sep 20, 2024

Conversation

Aaryanpal
Copy link
Contributor

@Aaryanpal Aaryanpal commented Sep 12, 2024 •
edited
Loading

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
cielf
cielf reviewed Sep 12, 2024
View reviewed changes
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

dorner
dorner requested changes Sep 13, 2024
View reviewed changes
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
dorner
dorner requested changes Sep 15, 2024
View reviewed changes
spec/requests/partner_group_spec.rb
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
cielf
cielf reviewed Sep 19, 2024
View reviewed changes
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
@github-actions GitHub Actions
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
@Aaryanpal @jlandiseigsti
4634 - Add the Delete button on the Partner Group Page (rubyforgood#4649
724bc01 
)

* 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
Reviewers

@cielf cielf cielf left review comments

@dorner dorner dorner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ability to remove (empty) partner groups
3 participants
@Aaryanpal @dorner @cielf