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

Minor a_b/b_a mixup on case type edit screen #15412

Merged
merged 1 commit into from
Oct 7, 2019

Conversation

demeritcowboy
Copy link
Contributor

Overview

This fixes a minor a_b/b_a mixup that came up during this testing.

To reproduce:

  1. Create/edit a case type.
  2. On the roles tab in the dropdown to add a role start typing something that isn't in the list, like "Spacefleet Commander is".
  3. It will suggest (new) beside your typing. Click it.
  4. It will open a box to create a new relationship type on the fly. Fill it out.
  5. When you save the new relationship type it will add the A to B direction to the table. So far everything is fine.
  6. Now try to add the B to A direction to the table. It won't let you because while the table is correct, the dropdown has them mixed up and thinks you're trying to add the same thing that's already in the table.
  7. Similarly try to add the A to B direction to the table again. It will make a duplicate, except it's not actually a duplicate it has added the opposite direction now. You can see this if you save and then re-edit the case type.

The affected code is only ever used in this scenario, before you close the case type edit screen, and only for the newly added relationship type. Before the patch the relationship type itself is fine, the rest of the dropdown is fine, and once you close and re-edit the case type the dropdown is also fine including the relationship type you just added earlier. So it's just this one thing in this one place in this one workflow.

Before

Confusing/can add wrong direction

After

Better

Technical Details

Comments

Also adds test which I couldn't add yet during #15378

@alifrumin @agh1

@civibot
Copy link

civibot bot commented Oct 7, 2019

(Standard links)

@civibot civibot bot added the master label Oct 7, 2019
@alifrumin
Copy link
Contributor

  • General standards

    • Explain (r-explain) - PASS : The goal/problem/solution have been adequately explained in the PR.
    • User impact (r-user) - PASS: The change would be intuitive for a majority of users who work with this feature.
    • Documentation (r-doc) - PASS: There are relevant updates for the documentation, or the changes do not require documentation.
    • Run it (r-run) - PASS: I tested by following the instructions of how to reproduce in the "Overview" section. Specifically, I logged into the jenkins build and tested by creating a new case type. Adding a new Relationship Type from the Case Roles options list. Selecting the "Relationship Type Label B to A" instead of A to B and saving. It saved no problem 👍
  • Developer standards

    • Technical impact (r-tech) - PASS: The change preserves compatibility with existing callers/code/downstream.
    • Code quality (r-code) - PASS: The functionality, purpose, and style of the code seems clear+sensible.
    • Maintainability (r-maint) - PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests. -PASS: The change adds a test
    • Test results (r-test) - PASS: The test results are all-clear.

This looks ready to be merged to me 👍

@seamuslee001 seamuslee001 merged commit 7646662 into civicrm:master Oct 7, 2019
@seamuslee001
Copy link
Contributor

thanks @alifrumin @demeritcowboy

@alifrumin
Copy link
Contributor

that was speedy! Thank you @seamuslee001

@demeritcowboy
Copy link
Contributor Author

Thanks @alifrumin @seamuslee001

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants