Invalid URLs in insight notification emails #1787

Closed
su opened this Issue Jan 1, 2014 · 5 comments

Comments

Projects
None yet
4 participants

su commented Jan 1, 2014

All the URLs I get in this message are in the format eg http:///thinkup/?u=TwitterNickHere&n=twitter&d=2014-01-01&s=favorites_year_ago_flashback

Note:

  • Triple slash on the protocol, though this might actually be due to…
  • Lack of domain name ref(this install is in a /thinkup/ subdir)

Occurs for every URL in the e-mail, just in case they're generated from different places. This includes the link to mail prefs in footer.

Owner

anildash commented Jan 2, 2014

The three email templates for insight emails all reference {$application_url} which is apparently undefined in your install. See:

The controllers for other parts of the app (e.g. ForgotPasswordController, InstallerController, RegisterController) seem to define application_url thus:

$email_view->assign('application_url', Utils::getApplicationURL() );

Curiously, the InsightGeneratorController has no such assignment, which makes me wonder how it ever works in an email, but obviously it works much of the time. Pinging @cdmoyer in case he's got any insights here. Pun!

Contributor

cdmoyer commented Jan 2, 2014

Hmm. It's actually set the same way, but in InsightsGeneratorPlugin during the crawl.

$view->assign('application_url', Utils::getApplicationURL());  

let's see what Utls is doing:

        $server = empty($_SERVER['SERVER_NAME']) ? '' : $_SERVER['SERVER_NAME'];        
        //Try HTTP_HOST                                                                 
        if ($server == '' ) {                                                           
            $server = empty($_SERVER['HTTP_HOST']) ? '' : $_SERVER['HTTP_HOST'];        
        }                                                                               
        //Then fall back to stored application setting set by Installer::storeServerName       
       if ($server == '') {                                                            
            $option_dao = DAOFactory::getDAO('OptionDAO');                              
            $server_app_setting = $option_dao->getOptionByName(OptionDAO::APP_OPTIONS, 'server_name');                   
            if (isset($server_app_setting)) {                                           
                $server = $server_app_setting->option_value;                            
            }                                                                           
        }                                                                               
        if ($replace_localhost_with_ip) {                                               
            $server = ($server == 'localhost')?'127.0.0.1':$server;                     
        }                                                                               
        //domain name is always lowercase                                               
        $server = strtolower($server);      

So, if you aren't running from an HTTP request, (such as via cron) then you had to have set a server_name set during install, it seems. Which looks like it's set automatically in Installer

783     public static function storeServerName() {                                      
784         $server_name = empty($_SERVER['SERVER_NAME']) ? $_SERVER['HTTP_HOST'] : $_SERVER['SERVER_NAME'];  

One option, if running from cron would be to provide a SERVER_NAME on the command line, like:

cd .../ThinkUp/webapp/crawler/; \
  export THINKUP_PASSWORD=yourpassword; \
  export SERVER_NAME=myhostname.com; \
  /usr/bin/php crawl.php you@email.com

su commented Jan 3, 2014

Okay, if I follow what's going on in the Installer class, I should have a server_name row in my options table with a value of either example.com or an IP address. How long has that been around? My fairly old installation has no such row, nor does another that I set up for a client around August 2012. I'll try a totally new install in the next day or so to see what happens.

As for the cron edit, I'll add that and see what happens tomorrow(report's already run for today), but if I'm following correctly there might still be something that needs addressing in the app itself, right? Assuming it's not something weird about my servers, maybe the upgrade/migration process should test the value exists as an integrity check.

su commented Jan 6, 2014

Adding SERVER_NAME to cron worked, for what it's worth. Haven't had a chance to do the clean install yet.

Owner

ginatrapani commented Jan 6, 2014

Okay, if I follow what's going on in the Installer class, I should have a server_name row in my options table with a value of either example.com or an IP address.

That's right.

How long has that been around?

Looking at git blame, that got added in December 2012:

https://github.com/ginatrapani/ThinkUp/blame/master/webapp/_lib/class.Installer.php

It is set up to store that value anew each application upgrade, so in theory you should have it in your options table, @su. If not, something is amiss.

@ginatrapani ginatrapani added a commit that referenced this issue Jan 6, 2014

@ginatrapani ginatrapani Fix insight permalink in HTML notification email
Possibly related to #1787
376b402

@ecucurella ecucurella added a commit to ecucurella/ThinkUp that referenced this issue Jan 14, 2014

@ginatrapani @ecucurella ginatrapani + ecucurella Fix insight permalink in HTML notification email
Possibly related to #1787
2868da2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment