Skip to content
This repository has been archived by the owner on Jan 9, 2025. It is now read-only.

Phrases must not exceed 60chrs each #335

Conversation

xmacex
Copy link
Contributor

@xmacex xmacex commented Oct 2, 2018

I believe this fixes #334. In general I do not feel very confident about this PR, but I want to contribute 😄 . I hope someone more familiar with the TCAT codebase reviews this PR. Comments are of course welcome, and I'll do my best to incorporate them.

I provide a couple of tests for this under a new top-level directory tests. There is an element of danger here, if the tests are run on a live TCAT installation where test data might trickle down to the database. Some PHPUnit database setup and fixtures ought to used to insulate these tests from live data.

Sorry I could not provide tests for the UI check and in Javascript function validateQuery(query,type) For my own purposes, I isolated the function into validateQuery.js and my test suite with Jest is the following package.json

{
    "dependencies": {
	"jquery": "^3.3.1"
    },
    "scripts": {
	"test": "jest"
    },
    "jest": {
    }
}

this jest.config.js

module.exports = {
    setupFiles: ["<rootDir>/test-env.js"],
    testEnvironmentOptions: {
	beforeParse (window) {
	    console.log('------------------ window.alerts')
	    window.alert = (msg) => {console.log(msg); };
	}
    }
}

and the following validateQuery.test.js

const validateQuery = require('./tcat-validateQuery');

test('Perfectly fine, straightforwards track query should validate', () => {
    expect(validateQuery('kittens,lizards,aardvark', 'track')).toBe(true);
});

describe('Check commas in places', () => {
    test('Track query beginning with a comma should validate', () => {
	expect(validateQuery(',kitten,lizard,aardvark', 'track')).toBe(true);
    });

    test('Track query ending in a comma should validate', () => {
	expect(validateQuery('kitten,lizard,aardvark,', 'track')).toBe(true);
    });

    test('Track query with repeated commas should validate', () => {
	expect(validateQuery('kitten,,,,lizard,aardvark', 'track')).toBe(true);
    });

    test('Track query with commas inside literals should not validate', () => {
	expect(validateQuery("kitten,'lizard,queen',aardvark", 'track')).toBe(false);
    });
});

describe('Check query phrase lenghts', () => {
    test('Empty track query should not validate', () => {
	expect(validateQuery('', 'track')).toBe(false);
    });

    test('Long total query with short enough phrases should validate', () => {
	expect(validateQuery('kitten,lizard,aardvark,doggo,anteater,snake,wombat,vampire bat,zebra,moose,ruutana', 'track')).toBe(true);
    });
    
    test('Track query with a phrase 60 chrs long phrase should validate', () => {
	expect(validateQuery('kitten,lizard,aarvark,012345678901234567890123456789012345678901234567890123456789', 'track')).toBe(true);
    });

    test('Track query phrase over 60 chrs long phrase should not validate', () => {
	expect(validateQuery('kitten,lizard,aarvark,012345678901234567890123456789012345678901234567890123456789aaaaaaaaaaaaaaaaaaaaa', 'track')).toBe(false);
    });
});

And then npm test yields

PASS ./tcat-validateQuery.test.js
✓ Perfectly fine, straightforwards track query should validate (7ms)
Check commas in places
✓ Track query beginning with a comma should validate (1ms)
✓ Track query ending in a comma should validate
✓ Track query with repeated commas should validate
✓ Track query with commas inside literals should not validate (1ms)
Check query phrase lenghts
✓ Empty track query should not validate
✓ Long total query with short enough phrases should validate (1ms)
✓ Track query with a phrase 60 chrs long phrase should validate
✓ Track query phrase over 60 chrs long phrase should not validate

------------------ window.alerts
Imported jQuery version 3.3.1 in test-env.js
Single quotes can only be used to specify a literal query without comma's. Please change your query.
You should specify at least one query
Query phrases should not exceed 60 characters each. Please shorten your query phrases.
Test Suites: 1 passed, 1 total
Tests: 9 passed, 9 total
Snapshots: 0 total
Time: 6.33s
Ran all test suites.

xmacex added 7 commits October 2, 2018 01:36
…new bin, both at the web UI and further down the stack.
… to throw an exception in addition to an error message for humans. This is a risky move up the stack I might think, if it is not caught under normal operations.
… to throw an exception in addition to an error message for humans. This is a risky move up the stack I might think, if it is not caught under normal operations.
… procrastination commit under cognitive stress of getting all the tests in place. Please bear with me.
…ctionality in modify_bin() too. On the UI side the same validateQuery() is used as when making a new bin, so I that should be fine.
@xmacex
Copy link
Contributor Author

xmacex commented Nov 10, 2018

242532961023202

Any change of someone reviewing this, and maybe either merging it, advising what I ought to do to help improve it, or rejecting it? :)

@xmacex
Copy link
Contributor Author

xmacex commented Nov 13, 2018

I've implemented checks against the 60 character limit per query in common/functions.php function validate_capture_phrases(), as requested in #334 (comment). Now that I think about it, I was making some assumptions where it will be employed in the application... 🤔. I better check those assumptions...

@xmacex
Copy link
Contributor Author

xmacex commented Nov 13, 2018

Ok phew my assumptions were not ludicrous.

howdonamesevenwork:dmi-tcat $ find . -name '*.php' -exec egrep -l 'validate_capture_phrases' \{\} \;;
./capture/search/search.php
./tests/common/functionsTest.php
./common/functions.php
howdonamesevenwork:dmi-tcat $

Of which the last one only has its definition, the second one is in the test suite. As things currently stand, the only occurrence of calling it is

if (!validate_capture_phrases($keywords))

…e format familiar from the Twitter UI with disjunctions marked by ' OR ' instead of the comma ',' as the query bin manager thing on TCAT. Updated tests pass (hmm how can I have a test suite that I can commit to, but that does not get pushed to the repo? Maybe a separate project outside of the dmi-tcat top directory or something. Anyway...).
@xmacex
Copy link
Contributor Author

xmacex commented Sep 4, 2019

Whoops I notice a merge conflict has appeared 👀. Was it always there?

@niczem
Copy link
Contributor

niczem commented Feb 23, 2020

"Whoops I notice a merge conflict has appeared 👀. Was it always there?"

... the trim function was replaced by the sanitise function in between. I resolved the conflict.

Sorry for the long silence, we are currently cleaning up the repo and hope to merge open pull requests as soon as possible :)

@niczem niczem added the staged staged for merging label Jul 3, 2020
@niczem niczem changed the base branch from master to develop July 8, 2020 05:24
@niczem niczem merged commit 41204fd into digitalmethodsinitiative:develop Jul 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
staged staged for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long conjunctive query phrases make TCAT not start
2 participants