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

Check SMS is enabled before showing texting opts #803

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Conversation

BryceStevenWilley
Copy link
Contributor

  • is_sms_enabled(), sees if the twilio config in the settings are non-empty (doesn't check that they work)
  • Turns off mentions of texting in the "Share" page
  • Turns off option to get texts from the reminder system
  • Turns off options to text the link to yourself when signing

The only change I could see being suggested is to move is_sms_enabled() (and is_phone_or_email(), even thought it's been in AL for a while) to the Toolbox. IMO, it's not worth splitting highly coupled functions between two different packages, but will happily do it if suggested.

Fixes #411 for texting, not for email, but as mentioned in the meeting this morning, most of our users have some sort of email enabled on their server.

* `is_sms_enabled()`, sees if the `twilio` config in the settings are non-empty
  (doesn't check that they work)
* Turns off mentions of texting in the "Share" page
* Turns off option to get texts from the reminder system
* Turns off options to text the link to yourself when signing
@@ -311,7 +317,7 @@ template: al_share_answers_message_template
subject: |
${ AL_ORGANIZATION_TITLE } form from ${ al_share_form_from_name }
content: |
${ share_interview_answers_message }
${ share_interview_answers_message.replace("\n", "<br>") }
Copy link
Member

Choose a reason for hiding this comment

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

Are we always sending HTML email?

Copy link
Contributor Author

@BryceStevenWilley BryceStevenWilley Dec 11, 2023

Choose a reason for hiding this comment

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

Yeah, docassemble always sends an HTML version. Relevant code is: https://github.com/jhpyle/docassemble/blob/70d7dbfd7db5075f1e51d2b64906739a8f86f679/docassemble_base/docassemble/base/util.py#L8366-L8371

Specifically, when you pass in a template, it gets the content of that template in HTML. BeautifulSoup strips out the tags for the plaintext version if the client doesn't support HTML (which most email clients support, and default to), but both the HTML and non-HTML version are provided in the Message variable. Haven't gone further, but we can't know which clients will and won't be using HTML.

Copy link
Member

@nonprofittechy nonprofittechy left a comment

Choose a reason for hiding this comment

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

Looking good--I really like this change. Can't merge until we handle language properly. And maybe a clarification about the sending of HTML email. If we do make that change, we should use proper syntax - <br/> not <br>

@BryceStevenWilley
Copy link
Contributor Author

If we do make that change, we should use proper syntax - <br/> not <br>

I'm fine with changing it, but from what I understand, <br> is fine, <br/> is only necessary for XHTML

@BryceStevenWilley BryceStevenWilley merged commit 7726cbe into main Dec 12, 2023
5 checks passed
@BryceStevenWilley BryceStevenWilley deleted the no_sms branch December 12, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make some features (signature, sharing) that depend on multi_user responsive to whether it's turned on or off
2 participants