search

smart-facility / cognicity-reports-powertrack

cognicity-reports: NodeJS app - Twitter & GNIP PowerTrack support for the CogniCity framework
5 stars 4 forks source link

Incorrect tweet length checking #15

Closed matthewberryman closed 8 years ago

matthewberryman commented 8 years ago

The code in areTweetMessageLengthsOk CognicityReportsPowertrack#L645 in master incorrectly counts the length of a full URL instead of the 22 characters for the shortened URL. Rather than try and replicate the regexp that Twitter uses for URLs to find the URL, and only count 22 characters for the URL, it might be better to simply rely on detecting http error 403 paired with error 354 in returned json and fail based on that. Note that this bug is in master and 2.0.x and resolution in 2.0.x requires issue #14 to be resolved first.

benatwork99 commented 8 years ago

Sounds like a good approach. It's a shame we can't get it to check these at startup, because you may not find out there's a problem for some time, but agree re-implementing the URL parsing doesn't sound like something we want to do.

tomasholderness commented 8 years ago

Temporary hack pushed to cognicity-reports 2.0.x branch to account for short urls and new report ID linking. This will need to be cleaned and moved down into this repo (powertrack) as per this bug and #14 https://github.com/smart-facility/cognicity-reports/commit/fbab09cecd0fcd05c3a6c51d654c3e6b94e0e672

matthewberryman commented 8 years ago

I have an idea for how to rewrite sensibly using a regular expression, however I think we had better leave the timestamp in even if the URLs are unique because:

I will get to work on this but probably not until this afternoon.

matthewberryman commented 8 years ago

I have (tentatively) fixed it with https://github.com/smart-facility/cognicity-reports/commit/73449ce8bf0e09a86dd4335742faf5d99938ea15 .. https://github.com/smart-facility/cognicity-reports/commit/c97f999b563b1742f2603963ca2ddb0339ba71f4 (my use case for testing that directly at a node.js console didn't cover the no-urls case hence the extra fix, sorry). This now passes the unit tests that @benatwork99 wrote, see https://travis-ci.org/smart-facility/cognicity-reports/builds/93063735 although none of those test for cases with and without urls (including more than one) present. @benatwork99 can you please work on that.

matthewberryman commented 8 years ago

@benatwork99 before you write the tests I think we need to undo https://github.com/smart-facility/cognicity-reports/commit/da780d824f8a79038d50e757392378287679ee9c in addition to some of the other commits Tom's made. The tests lengths that @talltom put in with that commit aren't appropriate because the tests as run don't include nor have urls added, and we now have code for URLs which will need separate testing as mentioned. Also @talltom hardcoded in the wrong constant. I'd like to just double check a few things with Tom first and then I'm happy to undo his changes and then you can proceed, so please wait.

tomasholderness commented 8 years ago

Hi @matthewberryman thanks for this, happy for you guys to move ahead with this when @benatwork99 finds some time. Changes I made were definitely just a temporary fix.

matthewberryman commented 8 years ago

@benatwork99 after closing https://github.com/smart-facility/cognicity-reports-powertrack/issues/14 can you write the tests to: Test the checking code without urls but with varying the config.twitter.addTimestamp option. Test the checking code with config.twitter.addTimestame=false but with 0, 1, and 2 urls. I'm happy to test these independently as the code is independent and I want to avoid a combinatorial explosion. In all options check with two cases that should return false and true from the check.

benatwork99 commented 8 years ago

@matthewberryman FYI, https://dev.twitter.com/overview/t.co says the length can change and you can query the Twitter API to get the current length. Not something I'd like to do in test really, but this is probably what we should be doing.

Re. tests - we already have tests for with/without timestamp. Agree on the additional URL tests.

matthewberryman commented 8 years ago

I suspect it's very slowly growing. One option might be to make it a variable that I can periodically update. I will have a think over the weekend (working early shift today).

Noted re tests - sorry I didn't have a closer look.

benatwork99 commented 8 years ago

Yes I imagine very slow. Even if we use that for the main app (and may be good enough to have a config variable as you say) I will mock it out in test as I wouldn't like our unit test to be dependent on a network service.

matthewberryman commented 8 years ago

I've made url length a config option with https://github.com/smart-facility/cognicity-reports-powertrack/commit/427c837482d6a00fcf09ba79cabc46d17d57c387 and https://github.com/smart-facility/cognicity-reports-powertrack/commit/5243b3f307b39f4307c2b7d16ee60c3e66142301 and #14 is closed, so just up to @benatwork99 to do the additional url tests.

benatwork99 commented 8 years ago

This code is currently not run until https://github.com/smart-facility/cognicity-reports/issues/8 is fixed.

We can still write the unit tests at any point.