Navigation Menu

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

Move remaining "generated by Tendenci" footer text to template. #654

Closed

Conversation

BenSturmfels
Copy link
Contributor

@BenSturmfels BenSturmfels commented Mar 5, 2018

Extending upon 2b2e6f8, this change replacing the remaining hard-coded email footer with templated text. This means that the same email footer is consistently used across the site and enable changes to the footer without forking Tendenci.

Note that the new email_footer.txt template is a text version of the existing email_footer.html, hence I've stuck with that wording rather than the existing wording in add_tendenci_footer.

Extending upon 2b2e6f8, this change replacing the remaining hard-coded email
footer with templated text. This allows changes to the email footer without
forking Tendenci and means the same footer text will be used across the site.
@jennyq
Copy link
Member

jennyq commented Mar 5, 2018

The footer was actually added there intentionally to promote the Tendenci brand. I'm not sure if it is okay to move it to a template. Let me check with @eschipul.

Copy link
Member

@eschipul eschipul left a comment

Choose a reason for hiding this comment

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

I like the abstraction and keeping the HTML and code separate. But given we're a tiny virtual group that has not gone to crowdfunding or asked for donations, we have to pay the bills. Marketing is part of that solution.

What about moving it into a text file that is NOT in the templates folder but still in the repo? That would allow other developers to edit it but prevent our hosted clients who don't pay for email relay through SES from removing it.

Thoughts? Email is a big expense for us that clients don't understand so we do need the branding. And the image is actually supposed to be a beacon like Wordpress does so we can show stats within the sites for super_users.

@BenSturmfels
Copy link
Contributor Author

BenSturmfels commented Mar 6, 2018

Thanks @jennyq and @eschipul, I understand your points and certainly don't want to make it harder to market Tendenci.

I don't know that I have a better answer, but here are a few observations while I think about this further.

  • branding is a very personal thing, and people want control of how they look to their constituents
  • the current email footer + image is possibly a little long
  • this promotion isn't so relevant to a large proportion of email recipients, and can be confusing
  • it's hard to come up with language that works in all cases, eg. "association" may not be an appropriate word and "AMS" is meaningless to most readers
  • most don't begrudge Tendenci this marketing
  • some will still want full control and will need to remove it
  • putting the footer in a template makes it possible to remove the footer, but doesn't mean everyone will

I suspect that if the footer were more modest, like:

<p>Sent from <a href="https://www.tendenci.com">Tendenci</a></p>

this might all be a non-issue, as it's very close to what comes out of many smartphones. The phase "powered by" seems a little cliche. Especially if there was a way to also add some organisational branding.

Thinking out aloud, I wonder whether maybe the system needs both:

  1. An email footer template for the organisation's branding
  2. A modest non-removable "Sent from Tendenci" footer

That's just my thoughts for now. Moving to a text file is certainly a step forward.

@eschipul eschipul added this to Proposed in Tendenci 7 Mar 12, 2018
jennyq and others added 3 commits March 16, 2018 13:24
As discussed by @jennyq and @eschipul in
tendenci#654, Tendenci footers need to be in
code to support ongoing costs of outgoing email for hosted customers as well as
ongoing Tendenci development.
@BenSturmfels
Copy link
Contributor Author

@eschipul I've now moved all the footers to the pair of html_footer() and text_footer() functions for use in Python code and added template tags of the same name for components that insert the footers using templates.

I think this is about as simple as I can get it, but does involve touching a bit of code - this approach is a little more complicated than straight template includes. I've tested each of these changes by hand.

@PaulSD
Copy link
Contributor

PaulSD commented Mar 20, 2018

Looks to me like the real issue here is simply that some things are using the email_footer.html template when they should be using add_tendenci_footer() ...

Rather than adding new functions and templatetags, why not just update everything to use add_tendenci_footer()?

@BenSturmfels
Copy link
Contributor Author

@PaulSD I think that's a good approach so long as we just drop the footer off the three admin-recap templates. There's little value to these particular footers anyway, since admins already know they're running Tendenci. Refactoring these would mean I'd have to touch quite a bit of management command code that will be time consuming to test thoroughly by hand.

Will push up this change in a moment.

@PaulSD
Copy link
Contributor

PaulSD commented Mar 21, 2018

This looks better to me :)

BTW, to squash these multiple attempts into a single commit, you can do:
git reset --soft HEAD~5 # Forget the last 4 commits, but leave the changes in the working directory so you can re-commit
git stash # Temporarily save the working directory changes so you can strip out the changes related to your "merge with master" commit
git pull https://github.com/tendenci/tendenci.git # Rebase on the current master
git stash pop # Bring back your changes, ignoring the "merge with master" stuff which is no longer relevant
git commit -a # Commit your changes
git push --force # Overwrite your old commits with the new commit

@BenSturmfels
Copy link
Contributor Author

@PaulSD I use GitLab a whole lot more that GitHub, but surely GitHub has a comparable "squash and rebase on merge" feature that the project committer can choose to use if that's their preference?

I'm of the philosophy that once a branch has gone public in any way, you don't rewrite it's history, to avoid creating a mess for anyone else following along.

@PaulSD
Copy link
Contributor

PaulSD commented Mar 21, 2018

Hmm... Yup, I guess it does: https://help.github.com/articles/about-pull-request-merges/

@jennyq
Copy link
Member

jennyq commented Mar 22, 2018

The changes look fine except per @eschipul we really want to have the image link and the text in the email footer.

I have an idea: Let's move the hard-coded tendenci footer to a template tendenci_footer.html, but make this file a non-editable file in the theme editor. Basically, hide this file so that it is not accessible from web interface. Of course, you can easily change this file at server level. How does that sound?

This means the logo will actually show up, rather than being blocked as an
external image by email clients.
@BenSturmfels
Copy link
Contributor Author

@jennyq no problem, this sounds close to my original proposal, so I'll revert to that and add the image. I've made it a Base64 embedded image since most sensible email clients block remote image to prevent tracking.

Looks like this:
screenshot_1

I could use your help with making the email_footer.html and email_footer.txt non-editable - I've had a bit of a look through the code and there are a few spots you'd need to block editing. Probably in tendenci/apps/theme_editor/utils.py:get_dir_list(), get_all_files_list() and possibly also views.py:edit_file() also to be sure. That's getting kinda complicated. Any thoughts?

@jennyq
Copy link
Member

jennyq commented Mar 27, 2018

To hide the email_footer.html from theme editor, yes, in the get_all_files_list, I think you can just add this if statement right after the for loop for f in files: https://github.com/tendenci/tendenci/blob/master/tendenci/apps/theme_editor/utils.py#L193

if f not in ['email_footer.html']:

@AdamBark
Copy link
Contributor

Not sure if there's a better place to put this but it's somewhat related. Spamassassin isn't a big fan of html only emails, would you accept patches to send multipart messages? The image can also affect the score but it sounds like you want to keep that in all instances.

@eschipul
Copy link
Member

@AdamBark - I do want to reinstate the option of an html or text email option for the newsletter generator. I think open source is a tough business model so I'd like the default mention of the tendenci brand to remain. (a developer can remove this anyway as they aren't "pixels" and pull from the local site regardless.)

That would require checking the emails object to support text or html. And having a drop down in the newsletter generator to select one. And then generating all of the content.

But I agree with you. Re spam assassin, while this helpfile isn't directly applicable you can see what we have to do just to get spf records to work.
https://www.tendenci.com/help-files/tendenci-forms-email-notifications/

After SPF - I also highly recommend using dnsstuff.com to check your domain's configuration.

@jennyq jennyq force-pushed the master branch 4 times, most recently from b9f4355 to cfa8f55 Compare October 1, 2020 01:45
@BenSturmfels
Copy link
Contributor Author

Closing this PR as it's not currently a priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Tendenci 7
  
Proposed
Development

Successfully merging this pull request may close these issues.

None yet

5 participants