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

Use bug trackers defined in the DB #79

Open
wants to merge 7 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@atodorov
Contributor

atodorov commented Mar 14, 2016

You can define any bugtracker you want using the admin interface but these don't get used because Bugzilla and JIRA are hard-coded into the HTML template. Moreover there are JavaScript functions which explicitly declare trackers with IDs > 2 to be invalid. This all needs to go.

NOTE: this is the bare minimum patch to get a custom bug tracker running. I've seen a few places where I've left visible artefacts from the Bugzilla and JIRA hard-coding but I'll remove them later.

I'll focus first on getting tests working into the local environment and enabling Travis-CI before I can continue any further.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Mar 20, 2016

Member

Hi, thanks for your patch. I'll review this when back from my vocation.

Member

tkdchen commented Mar 20, 2016

Hi, thanks for your patch. I'll review this when back from my vocation.

<div class="bug_id">Bug ID</div>
<input type="text" name="bug_id">
</br>
<input type="checkbox" name="bz_external_track" value="bz_external_track">&nbsp;
<span class="use_bz_external_track">Check to add Test Cases to BZ</span>

This comment has been minimized.

@tkdchen

tkdchen Apr 3, 2016

Member

This checkbox should not be removed. If you do not require this, you can set BUGZILLA_EXTERNAL_TRACKER to False in settings/common.py

@tkdchen

tkdchen Apr 3, 2016

Member

This checkbox should not be removed. If you do not require this, you can set BUGZILLA_EXTERNAL_TRACKER to False in settings/common.py

This comment has been minimized.

@atodorov

atodorov Apr 28, 2017

Contributor

There is a bigger problem with this checkbox than disabling via setting or removing it from the code. Currently it works only for Red Hat Bugzilla, possibly another Bugzilla instance.
Because this PR removes everything that is hard-coded we don't really know how the remote system works anymore.

I imagine it will require a few more settings - the remote URL to receive the request and maybe a callback which knows how to send said request. We need to provide a very flexible way for users to be able to integrate Nitrate with whatever bug tracking system they want to use. To make it easier we can provide some default integrations for Bugzilla, JIRA, etc.

I will open a new issue to track this but will not add the changes to this PR because it is already very big.

@atodorov

atodorov Apr 28, 2017

Contributor

There is a bigger problem with this checkbox than disabling via setting or removing it from the code. Currently it works only for Red Hat Bugzilla, possibly another Bugzilla instance.
Because this PR removes everything that is hard-coded we don't really know how the remote system works anymore.

I imagine it will require a few more settings - the remote URL to receive the request and maybe a callback which knows how to send said request. We need to provide a very flexible way for users to be able to integrate Nitrate with whatever bug tracking system they want to use. To make it easier we can provide some default integrations for Bugzilla, JIRA, etc.

I will open a new issue to track this but will not add the changes to this PR because it is already very big.

This comment has been minimized.

@tkdchen

tkdchen May 7, 2017

Member

Please bring the tabs and checkbox Check to add Test Cases to BZ back.

I agree that the checkbox could be configurable, but at this moment, it should be still usable.

Tabs are also useful when there are a few bug trackers enabled. Different bug trackers can have their own settings and display corresponding controls in the dialog for users, that would be flexible to add more controls in the dialog in the future.

I think you could just fix this commit instead of making another commit.

@tkdchen

tkdchen May 7, 2017

Member

Please bring the tabs and checkbox Check to add Test Cases to BZ back.

I agree that the checkbox could be configurable, but at this moment, it should be still usable.

Tabs are also useful when there are a few bug trackers enabled. Different bug trackers can have their own settings and display corresponding controls in the dialog for users, that would be flexible to add more controls in the dialog in the future.

I think you could just fix this commit instead of making another commit.

This comment has been minimized.

@atodorov

atodorov May 7, 2017

Contributor

Scratch the part about tabs and UI, this is completely different ball game. We're not there yet.

Adding back the checkbox is part of #185. I'm not going to include it in this PR.

You keep saying we shouldn't break existing functionality but the fact is that it is already broken even without this pull request. The hard-coded Bugzilla and JIRA related code works only for the Bugzilla and JIRA instances that are hard-coded. Given that by default we have bugzilla.redhat.com means nobody else besides Red Hat uses this setting because it is broken for them. More over without custom bug tracker support the entire Add/Remove bug is broken for everyone outside Red Hat.

@atodorov

atodorov May 7, 2017

Contributor

Scratch the part about tabs and UI, this is completely different ball game. We're not there yet.

Adding back the checkbox is part of #185. I'm not going to include it in this PR.

You keep saying we shouldn't break existing functionality but the fact is that it is already broken even without this pull request. The hard-coded Bugzilla and JIRA related code works only for the Bugzilla and JIRA instances that are hard-coded. Given that by default we have bugzilla.redhat.com means nobody else besides Red Hat uses this setting because it is broken for them. More over without custom bug tracker support the entire Add/Remove bug is broken for everyone outside Red Hat.

@@ -861,8 +851,6 @@ def get_context_data(self, **kwargs):
'test_case_run_bugs': bug_ids,
'mode_stats': mode_stats,
'summary_stats': summary_stats,
'jira_url': jira_url,
'bugzilla_url': bugzilla_url

This comment has been minimized.

@tkdchen

tkdchen Apr 3, 2016

Member

Both of these should not be removed. IMO, which bug trackers are displayed depends on what bug trackers are defined in the database. Displaying links linking to bugs in the report page is required and a must-have feature in Nitrate. Instead of deleting jira_url and bugzilla_url, it would be better display them by consulting the database. This is the same as adding a issue ID in a test run page.

@tkdchen

tkdchen Apr 3, 2016

Member

Both of these should not be removed. IMO, which bug trackers are displayed depends on what bug trackers are defined in the database. Displaying links linking to bugs in the report page is required and a must-have feature in Nitrate. Instead of deleting jira_url and bugzilla_url, it would be better display them by consulting the database. This is the same as adding a issue ID in a test run page.

This comment has been minimized.

@atodorov

atodorov Apr 28, 2017

Contributor

These context variables are used in the template to render a single URL which will open all the bugs at once. The previous hard-coded design distinguished between Bugzilla and JIRA only by the presence of the dash symbol ("-") inside the bug ID. This is a terrible design and it doesn't really work with the current changes.

When viewing the test run bug report you can still see bugs for individual test case runs, you also see a list of all unique bugs at the bottom of the screen. However you no longer see the 2 shortcut URLs which will pen the list of all bugs inside a single page in the bug tracker.

This is similar to my previous comment, I will create a new issue to track its progress but will not add any more changes related to this to the current PR because it is already big enough.

@atodorov

atodorov Apr 28, 2017

Contributor

These context variables are used in the template to render a single URL which will open all the bugs at once. The previous hard-coded design distinguished between Bugzilla and JIRA only by the presence of the dash symbol ("-") inside the bug ID. This is a terrible design and it doesn't really work with the current changes.

When viewing the test run bug report you can still see bugs for individual test case runs, you also see a list of all unique bugs at the bottom of the screen. However you no longer see the 2 shortcut URLs which will pen the list of all bugs inside a single page in the bug tracker.

This is similar to my previous comment, I will create a new issue to track its progress but will not add any more changes related to this to the current PR because it is already big enough.

This comment has been minimized.

@tkdchen

tkdchen May 7, 2017

Member

See my above comment, opening bugs or issues should not be removed in this commit.

@tkdchen

tkdchen May 7, 2017

Member

See my above comment, opening bugs or issues should not be removed in this commit.

This comment has been minimized.

@atodorov

atodorov May 7, 2017

Contributor

Well again this is broken by design. I use GitHub and tons of others bug trackers and the result of the above reporting URL is:

https://bugzilla.redhat.com/buglist.cgi?..... listing my GitHub issue numbers as parameters. Hardly useful to anyone outside Red Hat, won't you agree ?

@atodorov

atodorov May 7, 2017

Contributor

Well again this is broken by design. I use GitHub and tons of others bug trackers and the result of the above reporting URL is:

https://bugzilla.redhat.com/buglist.cgi?..... listing my GitHub issue numbers as parameters. Hardly useful to anyone outside Red Hat, won't you agree ?

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 28, 2017

Coverage Status

Coverage increased (+0.02%) to 45.39% when pulling 901f223 on MrSenko:remove_bugzilla into a2363f8 on Nitrate:develop.

coveralls commented Apr 28, 2017

Coverage Status

Coverage increased (+0.02%) to 45.39% when pulling 901f223 on MrSenko:remove_bugzilla into a2363f8 on Nitrate:develop.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Apr 28, 2017

Contributor

@tkdchen it's been a while since I had the opportunity to work on this but it is ready for merge.

There are a few setbacks, see my comments above, but those are really not such a big deal and I will fix them in a separate PR.

The updated version is rebased to the latest develop branch and properly handles client side validation of bug IDs as you previously requested. I have also fixed a few minor bugs and made some improvements, see the commit log messages.

Waiting for review. You can also ping me on IRC.

Contributor

atodorov commented Apr 28, 2017

@tkdchen it's been a while since I had the opportunity to work on this but it is ready for merge.

There are a few setbacks, see my comments above, but those are really not such a big deal and I will fix them in a separate PR.

The updated version is rebased to the latest develop branch and properly handles client side validation of bug IDs as you previously requested. I have also fixed a few minor bugs and made some improvements, see the commit log messages.

Waiting for review. You can also ping me on IRC.

Improve admin display for bug tracker objects
by making it more clear what are the expected values.
We also give examples in the help text and update the
model verbose name so that it is easily found in Admin.

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 28, 2017

Coverage Status

Coverage increased (+0.05%) to 45.413% when pulling 9016038 on MrSenko:remove_bugzilla into a2363f8 on Nitrate:develop.

coveralls commented Apr 28, 2017

Coverage Status

Coverage increased (+0.05%) to 45.413% when pulling 9016038 on MrSenko:remove_bugzilla into a2363f8 on Nitrate:develop.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen May 1, 2017

Member

As mentioned in #185, IssueTracker should be the new name of this integration with Bugzilla, JIRA or any other things that are able to manage issues. So, "bug tracker" should be replaced through the code and documentation properly.

I'll use a separate branch to merge issue tracker related code, and then merge it into master once the improvement is completed and good enough.

BTW, currently, master branch should contain code that belong to milestone 4.0.

Member

tkdchen commented May 1, 2017

As mentioned in #185, IssueTracker should be the new name of this integration with Bugzilla, JIRA or any other things that are able to manage issues. So, "bug tracker" should be replaced through the code and documentation properly.

I'll use a separate branch to merge issue tracker related code, and then merge it into master once the improvement is completed and good enough.

BTW, currently, master branch should contain code that belong to milestone 4.0.

Bug tracker validation regexp doesn't have to be escaped
This produces incorrect regexp which fails validation on the
client side. Never before noticed because we've had this hard-coded
on the JS side before.

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 1, 2017

Coverage Status

Coverage increased (+0.05%) to 45.413% when pulling 25b44b3 on MrSenko:remove_bugzilla into a2363f8 on Nitrate:develop.

coveralls commented May 1, 2017

Coverage Status

Coverage increased (+0.05%) to 45.413% when pulling 25b44b3 on MrSenko:remove_bugzilla into a2363f8 on Nitrate:develop.

return result;
function validateIssueID(bugRegExp, bugId) {
// if bugRegExp is empty string then all input is valid!
if (!bugRegExp) {

This comment has been minimized.

@tkdchen

tkdchen May 7, 2017

Member

If bugRegExp is empty, Nitrate should refuse to add this bug ID.

Checking whether bugRegExp is empty string explicitly is a better way than only using !.

if (bugRegExp === '') {
     return false;
}
@tkdchen

tkdchen May 7, 2017

Member

If bugRegExp is empty, Nitrate should refuse to add this bug ID.

Checking whether bugRegExp is empty string explicitly is a better way than only using !.

if (bugRegExp === '') {
     return false;
}

This comment has been minimized.

@atodorov

atodorov May 7, 2017

Contributor

I don't mind changing it but can you explain to me what is the difference?

@atodorov

atodorov May 7, 2017

Contributor

I don't mind changing it but can you explain to me what is the difference?

var bug_re = new RegExp(bugRegExp);
// catch syntax errors in regular expressions
} catch(err) {
window.alert(err.name + ': ' + err.message);

This comment has been minimized.

@tkdchen

tkdchen May 7, 2017

Member

In whatever the case, it is not a good idea to display such underlying error message to user in client side. Instead, a descriptive message should be displayed.

@tkdchen

tkdchen May 7, 2017

Member

In whatever the case, it is not a good idea to display such underlying error message to user in client side. Instead, a descriptive message should be displayed.

This comment has been minimized.

@atodorov

atodorov May 7, 2017

Contributor

Hmmm, in this case we have a problem with the regexp compiling. Isn't JavScript going to provide the most descriptive error message, as to why the regex won't compile? Sure 'Your bug validation regexp is not correct' looks better but it doesn't help the user figure out what's wrong. Regular expressions are notoriously hard to debug. Can you tell me which one is better:
SyntaxError: Invalid regular expression: /$?/: Nothing to repeat or a generic message?

@atodorov

atodorov May 7, 2017

Contributor

Hmmm, in this case we have a problem with the regexp compiling. Isn't JavScript going to provide the most descriptive error message, as to why the regex won't compile? Sure 'Your bug validation regexp is not correct' looks better but it doesn't help the user figure out what's wrong. Regular expressions are notoriously hard to debug. Can you tell me which one is better:
SyntaxError: Invalid regular expression: /$?/: Nothing to repeat or a generic message?

<tr>
<td>View all bugs(JIRA)</td>
<td><a href="{{ jira_url }}" target="_blank">{{ jira_url }}</a></td>
</tr>

This comment has been minimized.

@tkdchen

tkdchen May 7, 2017

Member

Well, this commit aims to use bug trackers defined in database, then all hardcoded bug tracker settings should be read from database.

Both bugzilla_url and jira_url should not be removed, but be defined in and read from db along with Check to add Test Cases to BZ.

Potential improvements of issue trackers could be done in the future, but at current stage, "Use bug trackers defined in the DB" should not break existing features and original experience to users.

@tkdchen

tkdchen May 7, 2017

Member

Well, this commit aims to use bug trackers defined in database, then all hardcoded bug tracker settings should be read from database.

Both bugzilla_url and jira_url should not be removed, but be defined in and read from db along with Check to add Test Cases to BZ.

Potential improvements of issue trackers could be done in the future, but at current stage, "Use bug trackers defined in the DB" should not break existing features and original experience to users.

This comment has been minimized.

@atodorov

atodorov May 7, 2017

Contributor

This is part of #185. Given that I seem to be the most active user of Nitrate currently I think we can live this this for the next week or so until I implement it.

@atodorov

atodorov May 7, 2017

Contributor

This is part of #185. Given that I seem to be the most active user of Nitrate currently I think we can live this this for the next week or so until I implement it.

atodorov added some commits Apr 28, 2017

Use bug trackers defined in the DB
- remove hardcoded Bugzilla and JIRA trackers and let AddIssueDialog
  use bugtrackers which are defined in the database!
- use the validation regexp stored in the DB to actually validate
  if the bug ID conforms to the expected format
- render the bug addition HTML only once when the TestRun view is
  loaded, not on every TestCaseRun detail get!

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Update bug removal in JavaScript so that it works again
- add missing parameter to invocation of removeCaseRunBug()
- refactor AddIssueDialog so we can use it both for adding and
  removing of bugs
- update mass removal of bugs to use the refactored AddIssueDialog
  and actually perform bug ID validation before removal
- delete unused functions and values

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Use bugtrackers defined in DB for TestCase -> Bugs tab
- make sure to remove hard coded bug trackers
- this will utilize existing JavaScript validations
- update the HTML layout so that the butracker form
  isn't rendered every time the bug list is refreshed via AJAX.
  This is achieved by splitting the form into a separate template
  and including it under the bug list. The form is always static!

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Allow bug deletion only if user has permission to do so
Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Document bug tracker administration
Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment