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

CRM-21419 Prevent users from accidentally creating a single activity when they want multiple #11264

Merged
merged 1 commit into from
Nov 14, 2017

Conversation

seancolsen
Copy link
Contributor

@seancolsen seancolsen commented Nov 9, 2017

Overview

When creating a new activity with multiple contacts, CiviCRM offers a checkbox to "Create a separate activity for each contact". This is good.

But the "separate activity" use case is common enough, that missing the checkbox can lead to users accidentally creating combined activities. This mistake is bad because it prevents users from being able to make independent edits to those activities for different contacts.

Before

  • Users commonly want this box checked, but it's easy to forget to check it.

    before

After

  • "Activity Separation" is a required field, which prompts the user to make an intentional choice.

    after

  • "Activity Separation" only appears when multiple contacts are chosen (just like the checkbox did).

  • If you submit the form with multiple contacts but nothing selected for "Activity Separation", then the form does not validate.

    error

    • And if you don't choose multiple contacts, then the form won't complain.

Instead of a checkbox for the user to choose to create separate
activities for each contact, use a set of radio options in order to
prompt the user to make a decision. This helps prevent users from
accidentally choosing to create combined activities.
@totten
Copy link
Member

totten commented Nov 9, 2017

(CiviCRM Review Template WORD-1.0)

  • (r-jira) Pass
  • (r-test) Pass
  • (r-code) Pass(*): ...but I only skimmed...
  • (r-doc) Pass: Did a quick grep of the user+dev guides and couldn't find any stale references to the old option. The feature is probably small enough that it doesn't need a detailed explanation.
  • (r-maint) Pass
  • (r-run) Pass(*):
    • I tried creating activities using "Contact => New Activity". Used three configurations: (a) with a single person, (b) with multiple people and combined activity, (c) with multiple people and separate activities. Tried leaving fields blank (and observed expected validation messages) - and then created the activities successfully.
    • Then ran a report and saw the activities were as expected screen shot 2017-11-09 at 12 15 22 pm
    • I did not test any other activity-creation screens (like the ones embedded in "View Contact" or "Manage Case"). That might be a smart thing to do...
  • (r-user) Discuss: On one hand, it's clearer than before. On the other hand, if I were in the habit of regularly creating multi-contact activities, the extra click might feel a little grating. It would be tempting to add a system setting that determines the default value (no-default/combined/separate). But this is also very subjective. Might want to get another opinion on r-user.
  • (r-tech) Pass: Tweaks the UI/appearance without changing data-model or API model.

@AnneDru
Copy link

AnneDru commented Nov 9, 2017

HI, User Anne here! I've had a look at the before/after screenshots and explanation. If I understand it all correctly - and if you are seeking my 'user' perspective, then here it is:- I really like the option to HAVE TO CLICK to choose either option and the 'error message if I forget to check the box. I often create multiple activities/emails and often I want to personalise one or more of them. This would seem to give me the ideal solution. Currently, for those I wish to individualise, I have to create separate activities/emails, so this would be a great help if most of the info in the activity is common. The ERROR message will prevent me forging on and accidentally sending to multiple contacts by mistake. So, I like this idea very much from a USER perspective. I really don't mind the extra step and we soon get used to small changes like this (if we see the error message often enough!

@joannechester
Copy link
Contributor

As always, how people use the system will influence what they think of this PR. Personally the change Sean has proposed fits in nicely with how we use CiviCRM .
We mail out hard copy magazine and journal. If you print a PDF letter you can get an activity created automatically, but when you create mailing labels or export contact info that isn't an option. So I often want to create an activity for each of the 10,000 contacts I have exported onto a list to send to the mail house for sending out our magazine, or the 600 contacts I have just created labels for to send out our journal. I want separate activities, but I sometimes forget to tick the box. Sean's PR would mean I couldn't forget.

I would like to note though that the consequences of creating one activity with 10,000 contacts no longer has the diabolical performance hit it used to have on the loading of the activities screen.

@seancolsen
Copy link
Contributor Author

seancolsen commented Nov 13, 2017

Thanks @AnneDru and @joannechester 🙂 Now that we've had two user opinions on r-user expressing personal support for this PR, I think it's worth getting attention from a merger like @totten @eileenmcnaughton or @colemanw to see if you're comfortable merging based on Tim's review + user feedback.

@colemanw
Copy link
Member

I've looked it over and agree with this change & Tim's review.

@colemanw colemanw merged commit 3162c83 into civicrm:master Nov 14, 2017
@jitendrapurohit
Copy link
Contributor

jitendrapurohit commented Nov 16, 2017

seems a regression has been introduced with this change. We cannot add multiple target contacts to case activities. No radio button is shown and formrule error gets displayed on Save(screenshot from dmaster). @seanmadsen is it possible for you to take a quick look at it?

image

@seancolsen
Copy link
Contributor Author

seancolsen commented Nov 16, 2017

😲 I will look at this today. Thanks for raising this here, @jitendrapurohit

@totten
Copy link
Member

totten commented Dec 5, 2017

Just a little meta/retrospective on this one -- in the review, I had tested a few screens/pageflows, and I wanted to say, "This works in all the screens that I've tested, but there are others that should be tested." I had commented:

  • (r-run) Pass(*):
    • I tried creating activities using "Contact => New Activity". Used three configurations: (a) with a single person, (b) with multiple people and combined activity, (c) with multiple people and separate activities. Tried leaving fields blank (and observed expected validation messages) - and then created the activities successfully.
    • Then ran a report and saw the activities were as expected screen shot 2017-11-09 at 12 15 22 pm
    • I did not test any other activity-creation screens (like the ones embedded in "View Contact" or "Manage Case"). That might be a smart thing to do...

Of course, the last bullet wound up being significant, and I feel bad having labeled it Pass(*). Two things seem significant:

  • The asterisk is too subtle. It allows subsequent reviewers/mergers to skip over it.
  • The visual size and placement made the third bullet seem insignificant -- when, in fact, it was really an outstanding task.

I do think it's important and legitimate to quickly communicate when you've "tested X but not Y". The simplest change would have been to summarize as r-run: Incomplete or r-run: Partial instead of r-run: Pass(*).

@AnneDru
Copy link

AnneDru commented Dec 6, 2017 via email

sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
…eparation

CRM-21419 Prevent users from accidentally creating a single activity when they want multiple
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.

7 participants