jetpack icon indicating copy to clipboard operation
jetpack copied to clipboard

CRM: Ensuring the assigned owner of a contact is not removed upon editing the contact

Open coder-karen opened this issue 1 year ago • 1 comments

Fixes https://github.com/Automattic/zero-bs-crm/issues/3438

Proposed changes:

  • This PR makes sure a value is added to the assigned owner meta box when a user has been assigned, so that upon saving after editing the contact that value is not replaced.
  • This prevents the issue of assigned users being unassigned from contacts after editing the contact (if 'Assign Ownership' is unchecked in the CRM general settings page).

Other information:

  • [ ] Have you written new tests for your changes, if applicable?
  • [ ] Have you checked the E2E test CI results, and verified that your changes do not break them?
  • [ ] Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

https://github.com/Automattic/zero-bs-crm/issues/3438

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

To replicate the issue:

  • Create a couple of test contacts
  • Create a CRM Contact Manager user
  • Assign a contact to that user
  • Log in as that user, and attempting to make any change to that contact (eg. status).
  • The change should stick, but the user is now unassigned from that contact

To test the fix, with this PR applied:

  • With the same test contacts and user assignments as above, ensure you can edit a contact for which you have been assigned, and that your assignment isn't removed when doing so
  • Make sure you can't edit a contact for which you haven't been assigned (unless it is unassigned).
  • Also, make sure you cannot select any other user to be the owner of that contact.
  • To make sure there are no regressions:
  • Change the settings to make sure 'Assign Ownership' is checked, and ensure you can edit any contact, regardless of whether you are assigned or someone else is. Make sure you can also change the owner.

coder-karen avatar Mar 06 '24 12:03 coder-karen

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • :white_check_mark: Include a description of your PR changes.
  • :white_check_mark: Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • :white_check_mark: Add testing instructions.
  • :white_check_mark: Specify whether this PR includes any changes to data or privacy.
  • :white_check_mark: Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation :robot:


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged. If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

github-actions[bot] avatar Mar 06 '24 12:03 github-actions[bot]

Good point, thank you.

It seems that was also possible before this solution was implemented as well (albeit not as simply as just swapping a value number, but instead adding a select and option value via the inspector).

I've added some checks within ZeroBSCRM.MetaBoxes3.Contacts.php in the save_data function. Ironically there was an existing comment in the code there posing a question about whether or not we should have permissions checks there. Adding these checks too means the original code change wasn't needed now, so the name can display as it was.

coder-karen avatar Mar 08 '24 08:03 coder-karen