{"action":"created","issue":{"url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/1965","repository_url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector","labels_url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/1965/labels{/name}","comments_url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/1965/comments","events_url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/1965/events","html_url":"https://github.com/Charcoal-SE/SmokeDetector/issues/1965","id":311829576,"number":1965,"title":"Allow users who manually report posts to specify an optional reason to be posted in Smokey's message","user":{"login":"gparyani","id":6440487,"avatar_url":"https://avatars0.githubusercontent.com/u/6440487?v=4","gravatar_id":"","url":"https://api.github.com/users/gparyani","html_url":"https://github.com/gparyani","followers_url":"https://api.github.com/users/gparyani/followers","following_url":"https://api.github.com/users/gparyani/following{/other_user}","gists_url":"https://api.github.com/users/gparyani/gists{/gist_id}","starred_url":"https://api.github.com/users/gparyani/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gparyani/subscriptions","organizations_url":"https://api.github.com/users/gparyani/orgs","repos_url":"https://api.github.com/users/gparyani/repos","events_url":"https://api.github.com/users/gparyani/events{/privacy}","received_events_url":"https://api.github.com/users/gparyani/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-04-06T02:28:43Z","updated_at":"2018-04-06T21:13:59Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"I'd like to propose a new feature for SmokeDetector, to allow users who file manual reports to specify an optional reason when doing so. Manual reports are usually filed because the post is still spam, but evaded Smokey's spam heuristics. Occasionally, it may also not be obvious to other humans that it's in fact spam. For example, [this report](https://chat.stackexchange.com/transcript/message/43834039#43834039) for a spam seed, which other users giving feedback in other chat rooms may not know is a spam seed unless they bothered to go into Charcoal HQ and look it up.\r\n\r\nImplementing this would allow the reporter to supply a reason indicating that the post is a spam seed, which would be shown in the metasmoke page for the manual report, and in Smokey's messages in all chat rooms.\r\n\r\nIn my mind, this would look like this:\r\n\r\n    !!/report [link] \"[reason]\" [link] \"[reason]\"...\r\n\r\nAn example report would be:\r\n\r\n    !!/report https://meta.stackexchange.com/q/3122 \"PST repair spam\""},"comment":{"url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/comments/379381477","html_url":"https://github.com/Charcoal-SE/SmokeDetector/issues/1965#issuecomment-379381477","issue_url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/1965","id":379381477,"user":{"login":"teward","id":327952,"avatar_url":"https://avatars3.githubusercontent.com/u/327952?v=4","gravatar_id":"","url":"https://api.github.com/users/teward","html_url":"https://github.com/teward","followers_url":"https://api.github.com/users/teward/followers","following_url":"https://api.github.com/users/teward/following{/other_user}","gists_url":"https://api.github.com/users/teward/gists{/gist_id}","starred_url":"https://api.github.com/users/teward/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/teward/subscriptions","organizations_url":"https://api.github.com/users/teward/orgs","repos_url":"https://api.github.com/users/teward/repos","events_url":"https://api.github.com/users/teward/events{/privacy}","received_events_url":"https://api.github.com/users/teward/received_events","type":"User","site_admin":false},"created_at":"2018-04-06T21:13:59Z","updated_at":"2018-04-06T21:13:59Z","author_association":"MEMBER","body":"## Summary:\r\n\r\nI think this is doable, but the amount of complexity we'll have to have in the processing logic for this will make the code coverage/complexity *much* higher than we would probably like.\r\n\r\nTechnical details on the implementation are below, *but they do not take into consideration whether MetaSmoke will have any issues with this implementation*.\r\n\r\n------\r\n\r\n------\r\n\r\n## Implementation Data\r\n\r\nIn `chatcommands.py`, the Report command (currently on line #1057) has a function declaration like this:\r\n\r\n    def report(msg, urls):\r\n\r\n... from which we then create a list of URLs inside (on line 1073) with:\r\n\r\n    urls = list(set(urls.split()))\r\n\r\nThis set of URLs, then, is iterated over via a `for` loop as:\r\n\r\n    for index, url in enumerate(urls, start=1)\r\n\r\n... of which each tuple that is returned by `enumerate` is processed via the single index value for the loop and the URL for report processing.\r\n\r\nTherefore, the way this is passed in is that we explicitly take *all* the arguments sent via `!!/report` and then use that to populate the `urls` argument that is passed during the call of the `report` command by splitting on space-delimitation (i.e. \"foo bar baz\" being the arguments run through a `.split()` will return the list of `[\"foo\", \"bar\", \"baz\"]` with the way we're doing the current URL list population).\r\n\r\nFrom SmokeDetector's point of view, this is easily doable, at least for splitting these cases down.  Now, there are four specific cases I look at and build upon, and we're assuming that `msgargs` is going to have all the arguments that were passed to the `report` function as part of its declaration, and we'll build 'reports' instead of URLs:\r\n\r\n------\r\n\r\n**Case 1: Multiple URLs being reported, each with its own defined reason**: *Doable, but not nice*\r\n\r\nLet's take a look at your first case, operating under the assumption that puts a specific reason for each report.  A call of `!!/report foo.com/fudge \"bar baz\" foo.com/yay \"foo bar\"` would end up with `msgargs` passed into such redefined function as this string: `foo.com/fudge \"bar baz\" foo.com/yay \"foo bar\"`\r\n\r\nLet's take that and preprocess it (assume that `msgargs` is the variable in the function definition):\r\n\r\n    reports = []\r\n    reportraw = msgargs.split('\" ')\r\n    for report in reportraw:\r\n        reportdata = report.split(' \"')\r\n        reports.append((reportdata[0],reportdata[1]))\r\n\r\n... will have a `reports` object containing tuples, which ends up like this *the 'pretty print' is my doing):\r\n\r\n    [('foo.com/fudge', 'bar baz'),\r\n     ('foo.com/yay', 'foo bar\"')]\r\n\r\n... whereby each tuple can be handled properly.  This is able to be *handled*, but not very kindly in loops (the handling of which is irrelevant here for now).\r\n\r\n------\r\n\r\n**Case 2: A single URL is reported without a reason.**\r\n\r\nNow, let's look at your second case.  The above code works, but we need to adjust it little for a bare `!!/report foo.com/fudge` call to handle there not being a quote character in it:\r\n\r\n    reports = []\r\n    reportraw = msgargs.split('\" ')\r\n    for report in reportraw:\r\n        if '\"' in report:\r\n            reportdata = report.split(' \"')\r\n            reportdata[1] = reportdata[1].replace('\"', '')\r\n            reports.append((reportdata[0],reportdata[1]))\r\n        else:\r\n            reports.append((report, None))\r\n\r\n------\r\n\r\n**Case 3: Multiple URLs with a single report reason at the end**:\r\n\r\nThis would happen if `!!/report url1 url2 url3 \"global report reason for all\"` happens, as the `reportraw` value would be `['foo.com/one foo.com/fudge \"bar baz\"']`.\r\n\r\nNow, *this* we can do, but the above code wouldn't work for it.  *For just this case*, the split code is:\r\n\r\n    reports = []\r\n    reportraw = msgargs.split('\" ')\r\n    for report in reportraw:\r\n        reportdata = report.split(' \"')\r\n        reportdata[1] = reportdata[1].replace('\"', '')\r\n        if ' ' in reportdata[0]:\r\n            reporturls = reportdata[0].split()\r\n            for url in reporturls:\r\n                reports.append((url, reportdata[1].replace('\"','')))\r\n        else:\r\n            reports.append((reportdata[0], reportdata[1].replace('\"','')))\r\n\r\n... and the resultant `reports` are:\r\n\r\n    [('foo.com/one', 'bar baz'),\r\n     ('foo.com/fudge', 'bar baz')]\r\n\r\nAnd this is usable and doable.\r\n\r\n------\r\n\r\n**Case 4: Multiple URLs, but each URL does *not* have an explicit reason set next to it**\r\n\r\nNow, this is the truly evil case.  We basically have to hybridize all the cases together and have a really *evil* bit of code.\r\n\r\nAssume someone does this with `!!/report foo.com/one foo.com/fudge \"bar baz\" foo.com/yay \"foo bar\"`, which is entirely possible.\r\n\r\n`reportraw` would end up being this:\r\n\r\n    ['foo.com/one foo.com/fudge \"bar baz', 'foo.com/yay \"foo bar\"']\r\n\r\n... and then when we try and split this down further using the rest of the script, `reports` ends up as:\r\n\r\n    [('foo.com/one foo.com/fudge', 'bar baz'),\r\n     ('foo.com/yay', 'foo bar\"')]\r\n\r\nSo now, we have to do an iterative version of the processing that Case 3 has:\r\n\r\n    reports = []\r\n    reportraw = msgargs.split('\" ')\r\n    for report in reportraw:\r\n        if '\"' in report:\r\n            reportdata = report.split(' \"')\r\n            reportdata[1] = reportdata[1].replace('\"', '')\r\n            if ' ' in reportdata[0]:\r\n                reporturls = reportdata[0].split()\r\n                for url in reporturls:\r\n                    reports.append((url, reportdata[1]))\r\n            else:\r\n                reports.append((reportdata[0], reportdata[1]))\r\n        else:\r\n            reports.append((report, None))\r\n\r\nJust keep in mind how complicated this logic got.  And how quickly, nonetheless.\r\n\r\n------\r\n\r\nAssuming we have the code from the last test-case, then *in theory* instead of looping over the `for url in enumerate(urls, start=1):` that we had for processing, we'd instead have to do `for report in enumerate(reports, start=1):`, and then **change the behavior for this loop**, such that the loop at line 1081 becomes:\r\n\r\n    for index, report in enumerate(reports, start=1):\r\n        url = rebuild_url(report[0])\r\n        post = api_get_post(url)\r\n        reason = report[1]\r\n        \r\n        ...  # The rest of the code is otherwise the same until we get to line 1119 in the existing code...\r\n        \r\n        if reason:\r\n            why_info = u\"Post manually reported by user *{}* in room *{}* with reason *'{}'*.\\n\".format(msg.owner.name, msg.room.name, reason)\r\n        else:\r\n            why_info = u\"Post manually reported by user *{}* in room *{}*.\\n\".format(msg.owner.name, msg.room.name)\r\n        \r\n        ... # The rest of the code for the function is the same.\r\n\r\nThis is doable.  But this doesn't take into consideration how MetaSmoke handles this report."}}
{"actor":{"display_login":"teward"}}