Currently, every AJAX behavior causes the entire form to reload and the entire form to be passed through validation. This causes two problems 1) It is very abrupt for the user because required elements the user hasn't gotten to because they are lower on the page will hold up the ability to interact with elements higher on the page (yuck!), 2) It totally utterly prevents the ability to write simple tests for the form (critical!).

These two patches take an approach to tone down the validation problem.

  • salesforce_mapping-01-extract_default_values.patch: Prepares the ability to make this goal easier by separating the building of the form from the gathering of the data to populate the form. This makes it easier to manipulate the form's structure and behavior without interfering with the logic that populate's its data.
  • salesforce_mapping-02-ajax_validation.patch: Stops the AJAX behavior from refreshing the entire form and targets only specific parts of the form for an AJAX refresh.

This patch requires the following other patches to be applied first:

CommentFileSizeAuthor
#12 salesforce-mapping-fix-1951722.patch2.13 KBceardach
#6 salesforce_mapping-01-extract_default_values-1951722.patch35.21 KBceardach
#6 salesforce_mapping-02-extract_default_values-1951722.patch23.07 KBceardach
#3 salesforce_mapping-01-extract_default_values-1951722.patch34.44 KBceardach
salesforce_mapping-02-ajax_validation.patch22.71 KBceardach
salesforce_mapping-01-extract_default_values.patch34.83 KBceardach

Comments

kostajh’s picture

kostajh commented
Status: Needs review » Needs work

Thanks for this.

#2 seems a lot simpler. Any reason not to go that route?

+++ b/modules/salesforce_mapping/includes/salesforce_mapping.admin.inc
@@ -941,3 +1063,39 @@ function _sfm_get_empty_field_map_row() {
+function _sfm_get_required_mapping_fields(&$form, &$form_state) {

This should be renamed to _salesforce_mapping_get_required_mapping_fields

kostajh’s picture

kostajh commented

Also... any chance you could re-roll this? I am not able to apply. I checked out a commit (65344de) which includes the 5 issues you've mentioned above, but can't get either patch to apply cleanly.

ceardach’s picture

ceardach commented
StatusFileSize
new salesforce_mapping-01-extract_default_values-1951722.patch34.44 KB

The first patch sets up the ability to to do the second patch. Both are required. I have them separated because it is two distinct tasks, IMO.

The _sfm_* functions are helpers and "sfm" (aka: salesforce_mapping) is used for brevity's sake and readability since they are used extensively throughout the file. I can change them all to _salesforce_mapping_* if you would prefer.

A bunch of isset($mapping) got changed to isset($mapping->foo) in the file somewhere along the way causing the conflicts. I've rerolled patch 01 which fixes the problem. The original 02 patch still is fine.

levelos’s picture

levelos commented

@ceardach, the direction of these patches is great, although I can't get either patch to apply cleanly against head. Not a big deal, but I'd also prefer to keep all function names in the _salesforce_mapping_* format to respect the module namespace. Thanks!

kostajh’s picture

kostajh commented

@ceardach If you can re-roll this one more time to address the namespace issues and also to make sure it applies cleanly against the latest 7.x-3.x code, I'd very much appreciate it. Thanks!

ceardach’s picture

ceardach commented
StatusFileSize
new salesforce_mapping-02-extract_default_values-1951722.patch23.07 KB
new salesforce_mapping-01-extract_default_values-1951722.patch35.21 KB

Re-rolled and converted the _sfm_* functions.

kostajh’s picture

kostajh commented
Assigned: Unassigned » tauno
Status: Needs work » Reviewed & tested by the community

This seems okay to me. It is a lot to review! I did some tests with setting up mappings and editing existing ones without finding any problems.

I'd quite like to get this in so we can move on to #1951744: Tests! For just salesforce_mapping's admin form, but it's a start..

@tauno or @levelos can you review?

levelos’s picture

levelos commented
Assigned: tauno » bleedev
Status: Reviewed & tested by the community » Needs review

Assigning to @bleedev for review and hopeful commit this week.

bleedev’s picture

bleedev commented
Status: Needs review » Fixed

Made a few adjustments and committed to 7.x-3.x. Thank you ceardarch.

ceardach’s picture

ceardach commented
Status: Fixed » Needs work

The alterations to the patch introduced a bug that prevent the "direction" radio buttons from having the correct default values when the table is refreshed. I'm going to investigate and give you a patch.

bleedev’s picture

bleedev commented
Status: Needs work » Fixed

@ceardarch That bug was resolved today in commit ee7ee3. However, if you have a better resolution please post it.

ceardach’s picture

ceardach commented
Status: Fixed » Needs review
StatusFileSize
new salesforce-mapping-fix-1951722.patch2.13 KB

The initial problem was caused by adding the "break" after the "direction" case in _salesforce_mapping_get_default_value(). That case needed to inherit the default case which is why there was no break between the two. The change you had made only recalled the last saved value, not the inputted value, nor would give a default value. The attached patch restores the form to its original state.

Also, I moved the setting of the key value up to the section for fields without deltas since it is a non-delta field.

bleedev’s picture

bleedev commented
Status: Needs review » Fixed

@ceardarch I see what you did there. I added a small comment for future devs so they won't make the same mistake I did in assuming that the break was missing. Also made a few changes to handle the Key. Thank you for the update, it has been committed to 7.x-3.x, commit 17cc53d

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

  • Commit e020ef1 on 7.x-3.x, 8.x-3.x, process-deleted-refactor by bleedev: 
    Issue #1951722 by ceardach: Updates mapping admin form ajax callbacks...
  • Commit 17cc53d on 7.x-3.x, 8.x-3.x, process-deleted-refactor authored by ceardach, committed by bleedev:
    Issue #1951722 by ceardach, bleedev: Salesfore mapping admin page...

  • Commit e020ef1 on 7.x-3.x, 8.x-3.x, mapped-object-ui by bleedev: 
    Issue #1951722 by ceardach: Updates mapping admin form ajax callbacks...
  • Commit 17cc53d on 7.x-3.x, 8.x-3.x, mapped-object-ui authored by ceardach, committed by bleedev:
    Issue #1951722 by ceardach, bleedev: Salesfore mapping admin page...