BDI/BGE: Update FLS to check only populated fields for DI & target objects #4155

psadie-sfdo · 2019-04-01T20:05:22Z

This updates the way we handle FLS during the mapping phase of processing/dry-running. Previously we checked FLS on all the mapped fields. Now we'll run FLS only on fields that are actually being populated/otherwise being updated in the DI records. If any DI record in a Batch has a failing FLS check, then the entire batch is failed.

Critical Changes

Changes

Batch Gift Entry Enhancements

We updated how Batch Gift Entry and Data Import handle field level security (FLS). Before this release, a user would receive an FLS permissions error when creating a new batch in Batch Gift Entry when they didn't have edit access on all NPSP Data Import fields. Now, Batch Gift Entry only displays NPSP Data Import fields in the batch setup screen when a user has edit permissions on those fields.
In addition, we made it possible for an intern or a user with fewer security permissions on Opportunities or Payments to prepare a batch for processing by a gift entry manager or another user with more permissions. A user entering gifts must have access to the NPSP Data Import fields included in the batch. To process a batch, a user must have access to the fields on the NPSP Data Import object as well as the corresponding fields on Opportunities or Payments. For example, if your batch includes Donation Date (on the NPSP Data Import object) and that field maps to Opportunity Close Date and Payment Date, then the user must have Edit access to all three fields.

Issues Closed

New Metadata

Deleted Metadata

…actively selected fields

ptewson-sfdo · 2019-04-02T01:07:31Z

src/classes/BGE_BatchGiftEntry_UTIL.cls

+    */
+    private static List<String> getCoreDataImportFields(Boolean includeRelationshipFields) {
+        List<String> fields = new List<String> {
+            'Id',


Is there any reason that Donation_Amount__c, Donation_Date__c, and NPSP_Data_Import_Batch__c aren't included in this list?

Yeah, these get added in by the getActiveFieldNamesFromBatch method as these are always required and are also considered "selected" fields in the BGE.

nm let's leave this alone

ptewson-sfdo · 2019-04-02T01:12:32Z

src/classes/BDI_DataImportService.cls

+            DataImportBatch__c currentBatch = dataImportBatches[0];
+            if (!String.isBlank(currentBatch.Active_Fields__c)) {
+                relevantDataImportFields = BGE_BatchGiftEntry_UTIL.getDataImportFields(batchId, false);
+                batchFromBGE = true;


Want to avoid references to "BGE" if we can describe this property more generically ... "batchDefinesActiveFieldList" or somethign

ptewson-sfdo · 2019-04-02T01:14:49Z

src/classes/BDI_DataImportService.cls

-            return dataImportBatches[0];
+            DataImportBatch__c currentBatch = dataImportBatches[0];
+            if (!String.isBlank(currentBatch.Active_Fields__c)) {
+                relevantDataImportFields = BGE_BatchGiftEntry_UTIL.getDataImportFields(batchId, false);


This was the thing that made me the most uncomfortable about the original PR, but I am thinking we may be able to avoid it - applications calling BDI (like BGE) should ideally not be defining BDI behaviors, else we could build up all these conditionals and exceptions built in for each application that's calling into it. HOWEVER, I think we may be able to avoid this

@ptewson-sfdo - Would moving this logic (along with other the field collection/map building) from the BGE util to the BDI_DataImportService class make sense? The one thing that immediately sticks out to me is that the getActiveFieldNamesFromBatch method from BGE_BatchGiftEntry_UTIL relies on the BGE_ConfigurationWizard_CTRL.BGEField class. If the above is acceptable, would moving BGEField into its own file make sense so we don't rely on the config wizard controller in BDI here as well?

We believe we can pull the call to BGE_BatchGiftEntry_UTIL.getDataImportFields(batchId, false) entirely - these fields would only be used for FLS when the help text maps to a target object field and then the check would be on the target field. None of these fields have default mappings or map logically to target fields and would not be checked anyway, and we can remove an unneeded dependency on BGE by pulling this out.

ptewson-sfdo · 2019-04-02T20:30:27Z

src/classes/BDI_DataImportService.cls

+    /*******************************************************************************************************
+    * @description flag to let us know if we came from BGE
+    */
+    global static Boolean batchFromBGE { get; set; }


avoid global!

ptewson-sfdo · 2019-04-02T20:30:34Z

src/classes/BDI_DataImportService.cls

+    * This list gets set in the loadBatch method.
+    * If no batch is loaded, we use the full list of all DI fields listStrDataImportFields
+    */
+    global static List<String> relevantDataImportFields {


avoid global!

…e util

…forceFoundation/Cumulus into feature/prince-bge-bdi-fls-bug-fix

ptewson-sfdo · 2019-04-04T23:36:09Z

src/classes/BGE_ConfigurationWizard_CTRL.cls

                && allowedDataTypes.contains(dfr.type)
                && !bannedFields.contains(dataImportFieldApiName)
                && dfr.getInlineHelpText() != null)
-                || allowedFields.contains(dataImportFieldApiName);
+                || allowedFields.contains(dataImportFieldApiName))
+            );


Is the call to "BGE_BatchGiftEntry_UTIL.checkFieldPermissions(filteredDFRMap.values());" below on line 223 redundant now, since the field has to be updatable to get in the map in the first place?

Yeah, it is redundant as all the fields passed in will always pass. What do you think about the following scenario?

If a user without perms for a field (Campaign Source) accesses a DI Batch that has Campaign Source in its Opportunity Selected Fields... with this update they'll receive an error on the DI Batch page but still be able to click "Edit" and open the Config Wizard. The page will tell them they don't have access to field Campaign Source, the page will not render the New Gift form and not render the Gifts table. They'll still be able to open the Config Wizard, but Campaign Source will not be visible for them in the Opportunity Selected Fields.

This is what we discussed earlier today (Monday 4/8) right? I think this is ok, we may look to providing a more informative blocking message when trying to edit a batch with an inaccessible field, but this is great of this story.

And I believe we should still remove the redundant call to "BGE_BatchGiftEntry_UTIL.checkFieldPermissions(filteredDFRMap.values());" on line 223-ish?

Yup, exactly. Completely agree, I'll go ahead and remove the call to the BGE util

…vice

…ess and is populated

ptewson-sfdo · 2019-04-08T21:45:59Z

src/classes/BDI_DataImportService.cls

@@ -812,8 +826,8 @@ global with sharing class BDI_DataImportService {

                        if (failFieldLevelSecurity ||
                            fieldDescribe == null ||
-                            !fieldDescribe.isCreateable() ||
-                            (!isAuditField && !fieldDescribe.isUpdateable())) {
+                            (!fieldDescribe.isCreateable() && populatedDataImportFields.contains(dataImportField)) ||


format this block's linebreaks a little more cleanly?

…r mapping method

ptewson-sfdo

Looks good!

ptewson-sfdo · 2019-04-09T00:36:57Z

**lurch: attach W-030471

LurchTheButler · 2019-04-09T00:40:01Z

Tracking W-030471

psadie-sfdo added 3 commits March 29, 2019 14:18

BDI only runs FLS on populated fields. BGE only runs FLS on core and …

0bbec29

…actively selected fields

Add flag when batch is from BGE

74ea7de

Fix populated field collection

eefb9c7

psadie-sfdo self-assigned this Apr 1, 2019

psadie-sfdo requested a review from ptewson-sfdo April 1, 2019 20:05

psadie-sfdo changed the title ~~Feature/prince bge bdi fls bug fix~~ [do not merge] Feature/prince bge bdi fls bug fix Apr 1, 2019

Merge b421aed into feature/prince-bge-bdi-fls-bug-fix

88e7121

ptewson-sfdo reviewed Apr 2, 2019

View reviewed changes

mrbelvedere added 4 commits April 2, 2019 10:25

Merge 30eeb7d into feature/prince-bge-bdi-fls-bug-fix

c9e0ae3

Merge b379f89 into feature/prince-bge-bdi-fls-bug-fix

5e6c546

Merge ca4a469 into feature/prince-bge-bdi-fls-bug-fix

280831c

Merge 7f23e7a into feature/prince-bge-bdi-fls-bug-fix

955dd72

ptewson-sfdo reviewed Apr 2, 2019

View reviewed changes

mrbelvedere and others added 6 commits April 2, 2019 16:58

Merge 5484454 into feature/prince-bge-bdi-fls-bug-fix

e0c0021

Fix BGE calls in BDI and add check for relationship fields in describ…

271efbc

…e util

Merge branch 'feature/prince-bge-bdi-fls-bug-fix' of github.com:Sales…

cd1348b

…forceFoundation/Cumulus into feature/prince-bge-bdi-fls-bug-fix

Merge 91b0a81 into feature/prince-bge-bdi-fls-bug-fix

80d28e7

Fix permission error on BGE config wizard

6faa986

Remove fields user doesn't have permissions to access in Config Wizard

b9d0732

ptewson-sfdo reviewed Apr 4, 2019

View reviewed changes

moved relationship type check from UTIL_Describe to BDI_DataImportSer…

eef472b

…vice

psadie-sfdo changed the title ~~[do not merge] Feature/prince bge bdi fls bug fix~~ feature/prince bge bdi fls bug fix Apr 5, 2019

psadie-sfdo added 2 commits April 5, 2019 17:13

Fix reference in loop

2b6048f

Add required fields to relevant data import fields

ab143c8

psadie-sfdo marked this pull request as ready for review April 8, 2019 12:54

psadie-sfdo and others added 2 commits April 8, 2019 12:08

Update FLS check, throw error only on DI fields where user has no acc…

019123b

…ess and is populated

Merge 4f450d0 into feature/prince-bge-bdi-fls-bug-fix

35312f8

Remove redundant FLS call

66b6569

ptewson-sfdo reviewed Apr 8, 2019

View reviewed changes

Update populated list string case and update condition readability fo…

b3e05db

…r mapping method

ptewson-sfdo approved these changes Apr 9, 2019

View reviewed changes

mrbelvedere added 2 commits April 10, 2019 12:25

Merge a6a0756 into feature/prince-bge-bdi-fls-bug-fix

7d5d2a1

Merge f796c33 into feature/prince-bge-bdi-fls-bug-fix

15fd82e

psadie-sfdo changed the title ~~feature/prince bge bdi fls bug fix~~ BDI/BGE: Update FLS to check only populated fields for DI process/dry-run Apr 10, 2019

ptewson-sfdo merged commit f791fc0 into master Apr 12, 2019

ptewson-sfdo deleted the feature/prince-bge-bdi-fls-bug-fix branch April 12, 2019 16:10

melissabarber changed the title ~~BDI/BGE: Update FLS to check only populated fields for DI process/dry-run~~ BDI/BGE: Update FLS to check only populated fields for DI & target objects Apr 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BDI/BGE: Update FLS to check only populated fields for DI & target objects #4155

BDI/BGE: Update FLS to check only populated fields for DI & target objects #4155

psadie-sfdo commented Apr 1, 2019 •

edited by melissabarber

Loading

ptewson-sfdo Apr 2, 2019

psadie-sfdo Apr 2, 2019 •

edited

Loading

ptewson-sfdo Apr 2, 2019

ptewson-sfdo Apr 2, 2019

ptewson-sfdo Apr 2, 2019

psadie-sfdo Apr 2, 2019

ptewson-sfdo Apr 2, 2019

ptewson-sfdo Apr 2, 2019

ptewson-sfdo Apr 2, 2019

ptewson-sfdo Apr 4, 2019

psadie-sfdo Apr 5, 2019

ptewson-sfdo Apr 8, 2019

ptewson-sfdo Apr 8, 2019

psadie-sfdo Apr 8, 2019

ptewson-sfdo Apr 8, 2019

ptewson-sfdo left a comment

ptewson-sfdo commented Apr 9, 2019

LurchTheButler commented Apr 9, 2019

BDI/BGE: Update FLS to check only populated fields for DI & target objects #4155

BDI/BGE: Update FLS to check only populated fields for DI & target objects #4155

Conversation

psadie-sfdo commented Apr 1, 2019 • edited by melissabarber Loading

Critical Changes

Changes

Batch Gift Entry Enhancements

Issues Closed

New Metadata

Deleted Metadata

Choose a reason for hiding this comment

psadie-sfdo Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptewson-sfdo left a comment

Choose a reason for hiding this comment

ptewson-sfdo commented Apr 9, 2019

LurchTheButler commented Apr 9, 2019

psadie-sfdo commented Apr 1, 2019 •

edited by melissabarber

Loading

psadie-sfdo Apr 2, 2019 •

edited

Loading