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

(WIP) CRM-14097 - Scheduled reminders UI problems #7877

Closed
wants to merge 2 commits into from

Conversation

rajithnishanth82
Copy link

Change the label of is_active field. Added new field for "Send Email"
Hide Email fields if "Send Email" is unchecked
Added required field validation for email subject
Moved "Record activity for automated email" to email section


Change the label of is_active field. Added new field for "Send Email"
Hide Email fields if "Send Email" is unchecked
Added required field validation for email subject
Moved "Record activity for automated email" to email section
Change the label of is_active field. Added new field for "Send Email"
Hide Email fields if "Send Email" is unchecked
Added required field validation for email subject
Moved "Record activity for automated email" to email section
@civicrm-builder
Copy link

Can one of the admins verify this patch?


//CRM-14097 - When un-checking the "send email" box, the email fields do not get hidden
cj(function ( $ ) {
$(document).ready(function(){
Copy link
Member

Choose a reason for hiding this comment

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

These two wrappers are not necessary - this code is already wrapped by CRM.$(function($) { on line 178.

@totten
Copy link
Member

totten commented Mar 28, 2016

jenkins, ok to test

@agh1
Copy link
Contributor

agh1 commented May 12, 2016

Jenkins, retest this please

@agh1
Copy link
Contributor

agh1 commented May 12, 2016

@rajithnishanth82 I'm going through a number of open PRs that may be considered for 4.7.8, and to be considered, this will need the tests to be passing. It looks like you've got a bad indent on line 285 of ScheduleReminders.php (see the test results).

If you can resolve that and the comments that @colemanw made, and commit in the next couple of days with the tests passing, we'll be able to consider it for merging this month.

@AkA84
Copy link

AkA84 commented May 16, 2016

I found 3 errors related to the "send email" field

1) The toggle of the "Email screen" fieldset doesn't work properly

When first loading the modal the toggle logic is inverted, the second time the modal is opened it doesn't work at all, the third time it's inverted, and so on

error-1

Providing a flag to the .toggle() method based on the status of the checkbox ensures that the visibility of the fieldset is tied to the checkbox value, and providing a more specific #email.crm-collapsible selector makes sure you're targeting the correct collapsible element

$('body').on('change', '.crm-scheduleReminder-form-block-active #send_email', function () { 
    $('#email.crm-collapsible').toggle(this.checked);
});

There is still the question of what the initial status would be for the checkbox. If unchecked, you also need to hide by default the fieldset

<fieldset id="email" class="crm-collapsible" style="display: none;">

2) The Subject field is still mandatory even when send_email is false

If you deselect the send email checkbox, the form will fail validation on the "subject" field, even if its section was removed as it wasn't of interest for the user (note that in the gif the bug described in issue no.1 is present)

error-2

3) send_email value is not saved

The send_email value will always be false even for reminders where it was set to true at the moment of submitting the form

error-3

@totten
Copy link
Member

totten commented May 21, 2016

Given the issues reported above, I'll edit the subject-line to say "WIP" (work-in-progress). When they're resolved, please remove "WIP".

@totten totten changed the title CRM-14097 - Scheduled reminders UI problems (WIP) CRM-14097 - Scheduled reminders UI problems May 21, 2016
@JoeMurray
Copy link
Contributor

Heh @rajithnishanth82 as Release Manager this month, I'm trying to recruit people to help pare down the backlog of almost 100 PRs, some going back to last summer. I'm wondering if you would be able to help QA another PR if I got someone to QA this PR?

@eileenmcnaughton
Copy link
Contributor

I'm closing this PR as I believe #7924 supercedes it -please re-open if I am wrong

@civicrm-builder
Copy link

Can one of the admins verify this patch?

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.

8 participants