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

Connection Owner Deletion Notice: Fix display bug and sanitize $_REQUEST #14082

Merged
merged 1 commit into from
Nov 21, 2019

Conversation

kbrown9
Copy link
Member

@kbrown9 kbrown9 commented Nov 20, 2019

The Connection Owner Deletion notice should display when the connection owner is
about to be deleted, but it doesn't.

The notice is shown when the connection owner's id is found in the array of user ids that are about to be deleted. However, the arguments to in_array() are an integer and an array of strings, and the in_array() call uses strict matching. Since the connection owner's id is never found in the array of ids that are about to be deleted, the notice doesn't display. Fix this by converting the user ids in the array from strings to integers.

Changes proposed in this Pull Request:

  • Convert the elements of the $user_ids_to_delete array to ints.
  • Santize and unslash the $_REQUEST variable before using it.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This fixes an existing feature.

Testing instructions:

  1. Install, activate, and connect Jetpack. The first connected user is the connection owner.

  2. Create some users. At least one of these users must be an admin. So you'll have at least two admins: the connection owner and at least one secondary admin.

  3. Log in as a secondary admin user.

  4. Navigate to the Users page.

  5. Verify that the Connection Owner Deletion notice displays when you attempt to delete the connection owner in different ways:
    a) Hover over the user name and click Delete. Verify that the notice displays and go back to the Users page.

    b) Check the box next to the connection owner’s user name, select the Delete action in the action dropdown box, and click Apply. Verify that the notice displays and go back to the Users page.

    c) Select multiple users, including the connection owner. Select the Delete action, and click Apply. Verify that the notice displays and go back to the Users page.

  6. The warning notice should not display if you aren’t deleting the connection owner, so test these cases with a user that isn't the connection owner:
    a) Hover over the user name and click Delete. Verify that the notice does not display and go back to the Users page.

    b) Check the box next to a user name, select the Delete action in the action dropdown box, and click Apply. Verify that the notice does not display and go back to the Users page.

    c) Select multiple users (do not include the connection owner), select the Delete action, and click Apply. Verify that the notice does not display.

Proposed changelog entry for your changes:

  • Fix bug that prevents the Connection Owner Deletion notice from displaying

The 'Delete Connection Owner' Notice doesn't display when the connection owner is
about to be deleted. This happens because the arguments to in_array() are an int
and an array of strings. Fix this by converting the elements of the $user_ids_to_delete
array to ints.

Also, santize and unslash the $_REQUEST variable before using it.
@kbrown9 kbrown9 added [Type] Bug When a feature is broken and / or not performing as intended General [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 20, 2019
@kbrown9 kbrown9 requested a review from a team November 20, 2019 18:23
@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: December 3, 2019.
Scheduled code freeze: November 26, 2019

Generated by 🚫 dangerJS against 3760617

@kbrown9 kbrown9 self-assigned this Nov 20, 2019
@jeherve jeherve added this to the 8.0 milestone Nov 21, 2019
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! Connect Flow Connection banners, buttons, ... and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 21, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This works well in my tests. Merging.

@jeherve jeherve merged commit 3ce846f into master Nov 21, 2019
@jeherve jeherve deleted the fix/deleting_connection_owner_notice branch November 21, 2019 09:35
@matticbot matticbot added [Status] Needs Changelog [Status] Needs Package Release This PR made changes to a package. Let's update that package now. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 21, 2019
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [Status] Needs Changelog labels Nov 25, 2019
jeherve added a commit that referenced this pull request Nov 25, 2019
jeherve added a commit that referenced this pull request Nov 25, 2019
* 8.0 Release: running changelog

* Changelog: add #13921

* Changelog: add #13980

* Changelog: add #13905

* Changelog: add #13971

* Changelog: add #13984

* Changelog: add #14009

* Changelog: add #13620

* Remove things that will ship in 7.9.1

* Changelog: add 7.9.1 release (#14044)

* Changelog: add base for 7.9.1 release

* Update release date and post link

* Changelog: add #14066

* Update changelog for 7.9.1

* Changelog: add #13405

* Changelog: add #13841

* Changelog: add #13924

* Changelog: add #13986

* Changelog: add #14010, #14028, #14053, #14055.

* Changelog: add #14054

* Changelog: add #14031

* Changelog: add #14039

* Changelog: add #14050

* Changelog: add #14070

* Changelog: add #14082

* Changelog: add #14084

* Changelog: add #14111

* Changelog: add #13961

* Changelog: add #14047

* Changelog: add #14091

* Changelog: add #14108

* Changelog: add #14121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Connect Flow Connection banners, buttons, ... General [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants