Skip to content

Conversation

@codygordon
Copy link
Collaborator

@codygordon codygordon commented Mar 23, 2021

Description

We need to insert custom texter profile fields into scripts which resulted some significant changes to the existing profile_fields implementation. cc @jeffm2001

  • Pull in this setting from getConfig so we can use an instance-wide variable

  • For consistency in variable naming, changed it to TEXTER_PROFILE_FIELDS, however using profile_fields setting var name will still work for backwards compatability

  • Added an isRequired key to the profile settings objects – this is really necessary for WFP so users can optionally enter some customized data. For backward compatibility, if legacy extra profile fields are unchanged and isRequired is not defined, those fields will be treated as if they had "isRequired": true so expected behavior of existing fields doesn't change. For new fields added, "isRequired": false should be added if it is optional.

  • Added a conditional env var called SCRIPTS_USE_TEXTER_PROFILE_FIELDS that will make any extra texter profile fields defined in TEXTER_PROFILE_FIELDS populate in the script editor. These will show up in the script editor with a prefix of texter_

Example of working isRequired vs not fields:
Screen Shot 2021-03-27 at 10 31 44 AM

Showing up in script editor:
Screen Shot 2021-03-27 at 10 46 01 AM

Checklist:

  • I have manually tested my changes on desktop and mobile
  • The test suite passes locally with my changes
  • If my change is a UI change, I have attached a screenshot to the description section of this pull request
  • My change is 300 lines of code or less, or has a documented reason in the description why it’s longer
  • I have made any necessary changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • My PR is labeled [WIP] if it is in progress

@codygordon codygordon requested a review from schuyler1d March 23, 2021 18:57
@codygordon codygordon force-pushed the texter-script-fields branch 2 times, most recently from ffb97cf to 1a1a3c2 Compare March 23, 2021 19:51
@codygordon codygordon force-pushed the texter-script-fields branch from 1a1a3c2 to 60dac53 Compare March 27, 2021 17:38
@codygordon codygordon requested a review from jeffm2001 March 27, 2021 17:59
@Frydafly Frydafly added A-contact loaders Area: Related to ingesting and importing contacts using the contact loaders framework S-waiting on review Status: Awaiting review from the assignee but also interested parties labels Mar 27, 2021
@jeffm2001
Copy link
Collaborator

I don't have time right now to test this thoroughly, but glancing through the code, it seems fine to me. My one question is do we actually need the SCRIPTS_USE_TEXTER_PROFILE_FIELDS env var? Wondering if we should just always make the tester profile fields available in the script. I'm trying to think of a scenario where that would be a problem, and not coming up with one.

@codygordon
Copy link
Collaborator Author

codygordon commented Mar 29, 2021

@jeffm2001 I can't either but implemented it this way with the spirit of disrupting others as little as possible, so for most folks they wouldn't see any change unless they choose to (of course this time other than the breaking change regarding var name / isRequired I mentioned).

I was also considering adding another key to the fields themselves, like useInScripts or the like.

Copy link
Collaborator

@schuyler1d schuyler1d left a comment

Choose a reason for hiding this comment

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

++ let's remove the environmental variable -- I appreciate the care to avoid disrupting folks, but this has no backwards-incompatible implications so we don't need one.

We only need new variables if it would break previous things (e.g. if you were changing or removing the availability of those script choices).

@schuyler1d schuyler1d added S-waiting on author Status: PR is awaiting some action (such as code changes) from the PR author and removed S-waiting on review Status: Awaiting review from the assignee but also interested parties labels Mar 30, 2021
@codygordon codygordon requested a review from schuyler1d May 11, 2021 16:10
Copy link
Collaborator

@schuyler1d schuyler1d left a comment

Choose a reason for hiding this comment

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

👍

@schuyler1d schuyler1d added S-ready for stage-main (qa) Status (ADMINS ONLY): PR label for those ready to be added for stage: Approved, tests, etc and removed S-waiting on author Status: PR is awaiting some action (such as code changes) from the PR author labels Sep 9, 2021
Copy link
Collaborator

@schuyler1d schuyler1d left a comment

Choose a reason for hiding this comment

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

I didn't realize this was backwards incompatible -- I would like to avoid that.

Further with a bit extra scrutiny I don't think the cacheable campaign thing needs to be included.

customFields = Object.keys(JSON.parse(firstContact.custom_fields));
}
return [];

Copy link
Collaborator

Choose a reason for hiding this comment

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

is this part necessary? This is caching the field list to be sent about the contact -- I don't think we need this at all as these are texter fields, not contact fields.

Copy link
Collaborator Author

@codygordon codygordon Dec 2, 2021

Choose a reason for hiding this comment

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

@crayolakat to answer @schuyler1d's question here, this is necessary because it's grabbing the existing contact custom fields to be merged with the texter fields to be cached on the campaign, allowing them to be used in scripts.

see this line:
https://github.com/MoveOnOrg/Spoke/pull/1961/files#diff-612db26be6b7f5a0cdd54b604ffa5d5afb3bd8208271fea120755796f1803972R77

@schuyler1d schuyler1d added S-waiting on author Status: PR is awaiting some action (such as code changes) from the PR author and removed S-ready for stage-main (qa) Status (ADMINS ONLY): PR label for those ready to be added for stage: Approved, tests, etc labels Sep 10, 2021
@crayolakat
Copy link
Collaborator

Hi @codygordon , while testing, I am seeing the following error message
image

That message appears when I click into a required field, and then click out without typing anything. Is the error message supposed to say that, or should it say "Zip Code is a required field" similar to what's in the screenshot provided in the description of your PR?

@codygordon codygordon removed the request for review from jeffm2001 January 7, 2022 17:15
@codygordon
Copy link
Collaborator Author

@crayolakat sorry this took so long, I totally missed this and thought it had already been deployed!

I fixed the weird validation message, as well as a couple bugs I noticed while re-reviewing.

Copy link
Collaborator

@crayolakat crayolakat left a comment

Choose a reason for hiding this comment

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

Thanks @codygordon !

Another small thing I noticed:

  1. Create a new user and join an organization. You'll be taken to a page asking you to complete your profile
  2. Don't fill out any additional fields and click "SAVE"
  3. Issue: No validation errors appear telling the user they need to fill out required fields

Can we add validation errors at that step?

image

@codygordon
Copy link
Collaborator Author

@crayolakat good flag, this wasn't working properly when extra was null or undefined. Pushed a fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-contact loaders Area: Related to ingesting and importing contacts using the contact loaders framework S-waiting on author Status: PR is awaiting some action (such as code changes) from the PR author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants