removed data science bugzilla stuff with jira #2316
Conversation
tiftran
force-pushed
the
2303
branch
2 times, most recently
from
cec60e7 to
488b74a
Compare
There was a problem hiding this comment.
Very nice @tiftran 🙆
Buncha nitty changes but overall looks great thank you! 🐻
.env.sample
Outdated
| @@ -7,6 +7,7 @@ DB_PASS=postgres | |||
| DB_USER=postgres | |||
| DEBUG=True | |||
| DELIVERY_CONSOLE_HOST= | |||
| DS_ISSUE_HOST= | |||
There was a problem hiding this comment.
Is there a dev/staging jira instance we can put in here?
There was a problem hiding this comment.
From lurking in the jira-support channel, I don't think there's a dev instance, there's https://jira.allizom.org which they call staging, but it seems to be more like a space for people to set up jira projects before they get migrated to production
There was a problem hiding this comment.
Okay then just stick https://jira.example.com/ in there, we'll need something in there for integration tests to pass.
| @@ -40,5 +40,5 @@ def fill_overview(selenium, base_url): | |||
| experiment.short_description = "Testing in here" | |||
| experiment.public_name = "Public Name" | |||
| experiment.public_description = "Public Description" | |||
| experiment.bugzilla_url = "http://bugzilla.com/show_bug.cgi?id=1234" | |||
| experiment.bugzilla_url = "https://jira.mozilla.com/browse/DS-123" | |||
There was a problem hiding this comment.
Hrmmmm I'll need to update my integration test PR for this
| @@ -14,7 +14,7 @@ def test_add_branch(base_url, selenium): | |||
| experiment.short_description = "Testing in here" | |||
| experiment.public_name = "Public Name" | |||
| experiment.public_description = "Public Description" | |||
| experiment.bugzilla_url = "http://bugzilla.com/show_bug.cgi?id=1234" | |||
| experiment.bugzilla_url = "https://jira.mozilla.com/browse/DS-123" | |||
There was a problem hiding this comment.
Leave these like this and we'll land this first and I'll fix it in my PR
| DS_root = urljoin(settings.DS_ISSUE_HOST, "DS-") | ||
| DO_root = urljoin(settings.DS_ISSUE_HOST, "DO-") | ||
|
|
||
| if ( | ||
| DS_root not in cleaned_value and DO_root not in cleaned_value | ||
| ) or self.get_ds_issue_id(cleaned_value) is None: | ||
|
|
||
| raise forms.ValidationError(err_str.format(ds_url=settings.DS_ISSUE_HOST)) | ||
| return cleaned_value | ||
|
|
||
| def get_ds_issue_id(self, bug_url): | ||
| ds = re.match(re.escape(settings.DS_ISSUE_HOST) + r"(DS|DO)-(\w+.*)", bug_url) | ||
|
|
||
| return ds.group(1) |
There was a problem hiding this comment.
Wait what am I thinking, couldn't you do this whole thing with just the regex match looking for the ID and if match is None then we know it's invalid?
There was a problem hiding this comment.
being the regex newbie, what do you mean by regex match looking for the ID, like if the url ends in numbers?
There was a problem hiding this comment.
I mean like if ds is None, then the regex failed to find the ID at the end of the URL because it didn't match the regex, so then we know whatever URL was provided isn't a 'valid' jira URL, which accomplishes the same thing as the two not in checks above it, so you should be able to remove them and just do the regex check?
.env.sample
Outdated
| @@ -7,6 +7,7 @@ DB_PASS=postgres | |||
| DB_USER=postgres | |||
| DEBUG=True | |||
| DELIVERY_CONSOLE_HOST= | |||
| DS_ISSUE_HOST= | |||
There was a problem hiding this comment.
Okay then just stick https://jira.example.com/ in there, we'll need something in there for integration tests to pass.
There was a problem hiding this comment.
@tiftran Just change those two little things and this is good to go 👍 Thanks tif!!
.env.sample
Outdated
| @@ -7,6 +7,7 @@ DB_PASS=postgres | |||
| DB_USER=postgres | |||
| DEBUG=True | |||
| DELIVERY_CONSOLE_HOST= | |||
| DS_ISSUE_HOST=https://jira.mozilla.com | |||
There was a problem hiding this comment.
Nit: https://jira.example.com/browse/
| def validate_ds_issue_url(self, bug_url): | ||
| ds = re.match(re.escape(settings.DS_ISSUE_HOST) + r"(DS|DO)-(\w+.*)", bug_url) | ||
|
|
||
| return ds |
There was a problem hiding this comment.
Just inline this above, doesn't need to be a method
No description provided.