removed data science bugzilla stuff with jira#2316
Conversation
tiftran
force-pushed
the
2303
branch
2 times, most recently
from
cec60e7 to
488b74a
Compare
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Very nice @tiftran 🙆
Buncha nitty changes but overall looks great thank you! 🐻
| 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.
| 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
| 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
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Thanks for the changes tif! Just a couple last things.
| 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?
| 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.
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
@tiftran Just change those two little things and this is good to go 👍 Thanks tif!!
There was a problem hiding this comment.
Nit: https://jira.example.com/browse/
There was a problem hiding this comment.
Just inline this above, doesn't need to be a method
2 participants
No description provided.