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

Add UTRSBot notification scripts #69

Merged
merged 87 commits into from May 12, 2016

Conversation

Projects
None yet
2 participants
@dqwiki
Member

dqwiki commented May 6, 2016

@TParis Could you write a description for the change log and for this issue?

I'll try and do a code review ASAP.

@dqwiki dqwiki self-assigned this May 6, 2016

@dqwiki dqwiki added the feature label May 6, 2016

@TParis

This comment has been minimized.

Show comment
Hide comment
@TParis

TParis May 6, 2016

Contributor

New feature: Applies a notification to a blocked user's talk page upon email verification
New feature: New "Blocking Admin" button automatically notifies blocking admins upon a request
New feature: New "WMF Staff" button emails community outreach and places ticket in HOLD queue
New feature: For IP appeals only, will request open proxy check when placed in OPP queue
Bug fix: Repaired "Autorized" language in ACP
Bug fix: Repaired missing [$lang] when moving ticket to OPP queue

Contributor

TParis commented May 6, 2016

New feature: Applies a notification to a blocked user's talk page upon email verification
New feature: New "Blocking Admin" button automatically notifies blocking admins upon a request
New feature: New "WMF Staff" button emails community outreach and places ticket in HOLD queue
New feature: For IP appeals only, will request open proxy check when placed in OPP queue
Bug fix: Repaired "Autorized" language in ACP
Bug fix: Repaired missing [$lang] when moving ticket to OPP queue

Show outdated Hide outdated public_html/appeal.php Outdated
@dqwiki

This comment has been minimized.

Show comment
Hide comment
@dqwiki

dqwiki May 7, 2016

Member

Ok, so several questions:

  • 1) Is the omission of subst: for the templates on purpose?
  • 2) Could we perhaps rename the onwiki templates to a uniform style vs:

private $userTemplate = "Unblock-utrs";
private $adminTemplate = "Unblock-UTRS-AdminNotify";
private $oppTemplate = "UTRS-OPP";

to maybe something like UTRS-unblock, UTRS-AdminNotify, UTRS-OPP?

  • 3) With how unblock-utrs is worded and coloured (like the normal awaiting review of an admin, and open) would it not be prudent to change it to the reviewed color and mark it as declined or accepted like normal unblock requests?
  • 4) L269 of appeal.php seems to have the wrong if statement (comment made direct in diff)
  • 5) We never should be throwing the direct IP from the "CU data" in the DB on to wiki. I know it's "protected" by other if statements, but routed through a function such as getCommonName() so we can ensure continual integrity of private data
  • 6) In OPP deferral $appeal->getCommonName() duplicates $appeal->getIP() in theoretical functionality
  • 7) When deferring to OPP fails because hasAccount is true, a message should be in place explaining why a CU needs to review this instead of a general error message about posting manually. The error message still could be used as a general failure
  • 8) I don't follow what is happening or being checked here: 45f3f8e#diff-b7f273a2c2eb9a486a8c5a03b50c115fR663
  • 9) In reviewing the following theoretical case:

    Does reset to new allow posting of a hashed IP, revealing publicly a hash that could be used to confirm if it's an old IP or not, then violating data retention?

It would not violate data retention, as the data was not collected by us - but provided - and that is a non-issue here. But posting hashes of IPs on closed appeals...should be prohibited as they serve no cause. Maybe throw a quick check?

Awesome work so far though :D

Member

dqwiki commented May 7, 2016

Ok, so several questions:

  • 1) Is the omission of subst: for the templates on purpose?
  • 2) Could we perhaps rename the onwiki templates to a uniform style vs:

private $userTemplate = "Unblock-utrs";
private $adminTemplate = "Unblock-UTRS-AdminNotify";
private $oppTemplate = "UTRS-OPP";

to maybe something like UTRS-unblock, UTRS-AdminNotify, UTRS-OPP?

  • 3) With how unblock-utrs is worded and coloured (like the normal awaiting review of an admin, and open) would it not be prudent to change it to the reviewed color and mark it as declined or accepted like normal unblock requests?
  • 4) L269 of appeal.php seems to have the wrong if statement (comment made direct in diff)
  • 5) We never should be throwing the direct IP from the "CU data" in the DB on to wiki. I know it's "protected" by other if statements, but routed through a function such as getCommonName() so we can ensure continual integrity of private data
  • 6) In OPP deferral $appeal->getCommonName() duplicates $appeal->getIP() in theoretical functionality
  • 7) When deferring to OPP fails because hasAccount is true, a message should be in place explaining why a CU needs to review this instead of a general error message about posting manually. The error message still could be used as a general failure
  • 8) I don't follow what is happening or being checked here: 45f3f8e#diff-b7f273a2c2eb9a486a8c5a03b50c115fR663
  • 9) In reviewing the following theoretical case:

    Does reset to new allow posting of a hashed IP, revealing publicly a hash that could be used to confirm if it's an old IP or not, then violating data retention?

It would not violate data retention, as the data was not collected by us - but provided - and that is a non-issue here. But posting hashes of IPs on closed appeals...should be prohibited as they serve no cause. Maybe throw a quick check?

Awesome work so far though :D

@dqwiki

This comment has been minimized.

Show comment
Hide comment
@dqwiki

dqwiki May 11, 2016

Member

@TParis after fixing several issues, I have tested:

  1. Account, account block
  2. Account, IP block
  3. IP block

Is there anything else testing needed?
Also please review and sign off on the latest changes (https://github.com/UTRS/utrs/pull/69/files/b66d667cc535e1174f3c901e594fbc31102acaab..563d042999ff312d21034815829e233cefa44ef5)

Member

dqwiki commented May 11, 2016

@TParis after fixing several issues, I have tested:

  1. Account, account block
  2. Account, IP block
  3. IP block

Is there anything else testing needed?
Also please review and sign off on the latest changes (https://github.com/UTRS/utrs/pull/69/files/b66d667cc535e1174f3c901e594fbc31102acaab..563d042999ff312d21034815829e233cefa44ef5)

@dqwiki

This comment has been minimized.

Show comment
Hide comment
@dqwiki

dqwiki May 12, 2016

Member

Since the changes were approved (I think) and the rest were bug fixes, some very critical, i'm pushing this to live.

Member

dqwiki commented May 12, 2016

Since the changes were approved (I think) and the rest were bug fixes, some very critical, i'm pushing this to live.

@dqwiki dqwiki merged commit 312cb74 into live May 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment