Skip to content
This repository has been archived by the owner on Nov 6, 2023. It is now read-only.

Update Ars-Technica.xml #4201

Closed
wants to merge 1 commit into from
Closed

Update Ars-Technica.xml #4201

wants to merge 1 commit into from

Conversation

terrorist96
Copy link
Contributor

Addresses #4196

@terrorist96
Copy link
Contributor Author

@J0WI what does needs test coverage update mean?

@J0WI
Copy link
Contributor

J0WI commented Mar 13, 2016

This will fail ruleset tests, see https://github.com/EFForg/https-everywhere/blob/master/ruleset-testing.md

@terrorist96
Copy link
Contributor Author

Please tell me what to add/remove to fix it. Thanks.

@J0WI
Copy link
Contributor

J0WI commented Mar 16, 2016

You need to add test urls for every possible expression that matches the rule and at least three examples for wildcards.

@terrorist96
Copy link
Contributor Author

Even for all the existing rules? How can I find test URLs? Something like this: https://arstechnica.com/civis/viewtopic.php?f=2&t=1308189&p=30695775#p30695775?
And I don't know what you mean by wildcards.

@J0WI
Copy link
Contributor

J0WI commented Mar 17, 2016

Exactly. Note that the test urls should be as generic as possible and should not contain any errors (e.g. 404).

There is a wildcard target host on https://github.com/EFForg/https-everywhere/pull/4201/files#diff-aa9c3be14dc8df076fc11fd18d4adeabR72
You could also replace this with a list of subdomains as target hosts.

@terrorist96
Copy link
Contributor Author

Here's what I'm trying to accomplish: if I comment on an article from arstechnica.com and someone comments after me, I'll get an email with a link like this
http://arstechnica.com/civis/viewtopic.php?f=2&t=1310747&p=30849471&e=30849471
If I'm not already logged in, this will take me to a page to log in. Https isn't forced for this page and it should be. The default login page (accessed by clicking the login button from the site) ends in civis/ucp.php, which is already covered by the rule, but the link to login that redirects to a specific thread (as obtained from an email notification) ends in civis/viewtopic.php, which doesn't have https forced. So I can't give a generic version of the link; it has to point to a specific thread, like the example I just pasted for the article about the Tesla Model X. If you strip the stuff after viewtopic.php, it will load a 404 page.

@terrorist96
Copy link
Contributor Author

Actually, the generic http://arstechnica.com/civis/ needs https forced too.

@terrorist96
Copy link
Contributor Author

@fuglede I need help getting this merged.

@terrorist96
Copy link
Contributor Author

Not fixed in (Version: 2016.5.10)

@J0WI
Copy link
Contributor

J0WI commented May 14, 2016

Please read our guides carefully:
https://github.com/EFForg/https-everywhere/blob/master/ruleset-style.md
https://github.com/EFForg/https-everywhere/blob/master/ruleset-testing.md

This ruleset does not comply with our current standards, so it needs to be rewritten.
This means:

  • get rid of unneeded wildcards
  • remove non-capturing groups
  • test coverage
  • update documentation

@jeremyn
Copy link
Contributor

jeremyn commented Sep 9, 2016

@J0WI I get what you're saying but this is literally a one-line change, and Ars-Technica.xml is already in the whitelist. I think this can be merged after the whitelist hash is updated, although @terrorist96 has deleted their branch so we might need a new pull request. What do you think?

@Hainish
Copy link
Member

Hainish commented Dec 9, 2016

source repo deleted

@Hainish Hainish closed this Dec 9, 2016
@jeremyn jeremyn mentioned this pull request Dec 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants