#4674 add input fields to set bugzilla overrides
Closed 4 years ago by pingou. Opened 4 years ago by karsten.
karsten/pagure bugzilla_overrides  into  master

@@ -144,10 +144,40 @@ 

      </div>

    </div>

    {% endif %}

+   <p><br></p>

+   Bugzilla Overrides:

+   <div class="row">

+     {% if g.authenticated and repo.user.user == g.fas_user.username or pagure_admin %}

+     <div class="col">

+       <span id="assignee_inputs">

+       <form action="" id="change_assignees">

+           <label for="fedora_assignee">Fedora Assignee</label>

+           <input title="Empty input resets assignee" class="form-control" name="fedora_assignee" id="fedora_assignee" value="{{

+                     repo.bzoverride.fedora_assignee if repo.bzoverride.fedora_assignee else repo.user.user }}" size=8/>

+ 	  <br>

+           <label for="epel_assignee">EPEL Assignee</label>

+           <input title="Empty input resets assignee" class="form-control" name="epel_assignee" id="epel_assignee"value="{{

+                     repo.bzoverride.epel_assignee if repo.bzoverride.epel_assignee else repo.user.user }}" 

+                  description="Default assignee for EPEL bugzillas"/>

+         <button class="btn btn-primary" type="submit" title="Update bugzilla overrides" id="update_assignees">

+            Update

+         </button>

+       </form>

+       </span>

+       <span id="assignee_spinner"></span>

+     </div>

+     {% else %}

+     <div class="col">

+       Fedora Assignee:<br>

+       {{ repo.bzoverride.fedora_assignee if repo.bzoverride.fedora_assignee else repo.user.user }}

+       <br><br>

+       EPEL Assignee:<br>

+       {{ repo.bzoverride.epel_assignee if repo.bzoverride.epel_assignee else repo.user.user }}

+     {% endif %}

+   </div>

  

    <script type="text/javascript" nonce="{{ g.nonce }}">

      window.addEventListener('load', function() {

- 

        set_up_monitoring = function(status){

          var _label = "Disabled"

          if (status === "monitoring") {
@@ -174,6 +204,36 @@ 

          }

        });

  

+       $("#update_assignees").click(function(){

+         $("#assignee_inputs").hide();

+         var _s = $("#assignee_spinner");

+         _s.html(

+           "<img id='assigneespinner' src='{{ url_for('static', filename='images/spinner.gif') }}?version={{ g.version}}'>"

+         )

+         _s.show();

+          var _epel_assignee = $('#epel_assignee').val().trim();

+          var _fedora_assignee = $('#fedora_assignee').val().trim();

+          var data = { epel_assignee: _epel_assignee, fedora_assignee: _fedora_assignee };

+          var url = "{{ url_for('distgit_ns.bzoverride_patch_endpoint', repo=repo.name, namespace=repo.namespace) }}";

+          $.ajax({

+              'url': url,

+              'method': 'POST',

+              'dataType': 'json',

+              'data': data,

+              success: function(res) {

+ 	        _s.hide();

+                 $("#assignee_inputs").show();

+                 alert("Successfully changed the bugzilla assignees");

+              },

+              error: function(res) {

+ 	        _s.hide();

+                 $("#assignee_inputs").show();

+                 alert("Failed to change the bugzilla assignees");

+                 console.log("Failed to change the bugzilla assignees");

+              }

+          })

+       });

+ 

        {% if g.authenticated %}

        $(".monitoring-menu a").click(function(){

          var selectedValue = $(this).attr('id');

Signed-off-by: Karsten Hopp karsten@redhat.com

This adds a button on the left side right below the anitya monitoring button to become or reset the EPEL default assignee.
Pls wait with merging until we have the currently WIP pagure-dist-git changes for the bugzilla overrides in place.

let's use jquery here as we do in the other places :)

What about the use case where I want to set someone else (for example of bot account) as point of contact?

var _label=foo
var _assignee = _label

And then i don't see any more uses of _label. Could we just assign _assignee?

What about the use case where I want to set someone else (for example of bot account) as point of contact?

Easy to do with the API, let me think about a solution for the web interface

Are there any tests that need to be added around this? What documentation needs to be updated as part of this?

Are there any tests that need to be added around this? What documentation needs to be updated as part of this?

The API part could be tested on pagure-dist-git's test suite. For the theme part, it would be great if we could enroll pagure-dist-gist on @fbo 's zuul integration and be able to run tests between pagure's #4674 and pagure-dist-git's 'https://pagure.io/pagure-dist-git/pull-request/82 :)

+1 I think the more resiliency we have here from a testing perspective the better

I agree with jlanda here, testing needs to be done as part of the pagure-dist-git plugin. There's already a test script that needs some additions for this. I'll be working on this today

I agree with jlanda here, testing needs to be done as part of the pagure-dist-git plugin. There's already a test script that needs some additions for this. I'll be working on this today

https://pagure.io/pagure-dist-git/pull-request/87# could help setting up the testing environment

2 new commits added

  • use jquery
  • drop unused _label variable
4 years ago

rebased onto c19aa182aead722228716bbe901d923c56e85bd7

4 years ago

This is waiting for the pagure-dist-git changes before it can get merged, right ?

Nope, it needs more changes. We need this to accept any string (could be an user or a @group) and that string should only be changeable by the main admin or infra/releng (ie pagure admins) as discussed in the thread you started on the devel list.

We've talked about this during our meetings and I'll implement that change soon

1 new commit added

  • Replace assignee button with two StringFields
4 years ago

I've replaced the assignee button with two input fields as discussed

I figure that this is a vertical spacing thing. I'm not a big fan of using html tags for styling, I would prefer some kind of css margin between sections on the sidebar. But probably this is out of scope ans we should fix that for the complete sidebar on a different commit

Is unassigning possible? Would be great to have a "leave it empty to unassign" tip in that case

If there is no overrides, shouldn't give the default value?

pretty please pagure-ci rebuild

4 years ago

I figure that this is a vertical spacing thing. I'm not a big fan of using html tags for styling, I would prefer some kind of css margin between sections on the sidebar. But probably this is out of scope ans we should fix that for the complete sidebar on a different commit

I agree, this looks somewhat strange and needs a workover. +1 for a different commit, though

2 new commits added

  • Allow empty input to reset assignees
  • Use repo admin if assignee is unset
4 years ago

I've dropped the fields, allow empty input now to reset the assignee, added a title to the input fields and show the repo admin as assignee if there are no overrides

What's the status of this, is this waiting for the other PR in pagure-dist-git to get merged ?

rebased onto 2e00afe710a10638f311a1f07ddec0a123fbc00c

4 years ago

What's the status of this, is this waiting for the other PR in pagure-dist-git to get merged ?

Yes

rebased onto 5622eca900468eb50c48b6ef5c23870c951e2541

4 years ago

rebased onto 7dc0560bcd4610c847524ced80f1aeedaad20164

4 years ago

For some reason pagure/themes/srcfpo/templates/repo_master_sidebar.html always ends up in the error callback of $("#update_assignees").click(function()
The URL and the data look sane when I log them on the console. What's going wrong, any idea ?

For some reason pagure/themes/srcfpo/templates/repo_master_sidebar.html always ends up in the error callback of $("#update_assignees").click(function()
The URL and the data look sane when I log them on the console. What's going wrong, any idea ?

try without stringify, json is native for javascript.

Something like...

data: {
  epel_assignee: _epel_assignee,
  fedora_assignee: _fedora_assignee
}

And I think that you want trim the input boxes' val() and convert '' to null before assigning it, so before calling jquery's ajax you should create _epel_assignee and _fedora_assignee after sanitizing the input box values

we can avoid a lot of wrong assignees just triming whitespaces, it's quite usual to copy&paste with extra whitespaces around the fas username|groupname

rebased onto 941bd33465278e6ca9a88ab68ca125810d94ea8b

4 years ago

rebased onto 269335c25103a04c55595bda25c45225a8eaefe8

4 years ago

I've removed the stringify, added trim() and log the values of the variables that I use instead of putting them together in the log.
It didn't make any difference on the the error, though

Took me a long time to figure out that firefox-71 misbehaves. The same code works with iridium (Chrome)

Considering Firefox is the default browser in Fedora, I do not think encouraging another browser is an acceptable solution.

Thanks to jlanda. This wasn't firefox but the uMatrix extension that did interfere even though I've told it not to.

I believe break rows should be in a paragraph (in other words: I don't think this is valid html)

This sounds like debugging/development lines, do you intent to keep them in this PR?

rebased onto c4211bfbf6a3958b948c965271b97701e25a6836

4 years ago

Those console log were indeed for debugging, I've removed them and moved the break row

I miss some feedback, a js alert would be enough.
We could rollback to the previous state too

How can I difference between an in progress ajax call from a commited one? I can't . It would be great to have some feedback once it's successfully changed or an spinner while in-progress or something similar to be able to difference between "my assignee changes are processing" and "my assignee changes are processed"

sure thing, I can use alerts instead. But I don't think we need a spinner, this POST will take a millisecond, there's nothing to wait for.

rebased onto a54c12e79b533cde0c80a344e5a3ba843243884a

4 years ago

sure thing, I can use alerts instead. But I don't think we need a spinner, this POST will take a millisecond, there's nothing to wait for.

Don't forget there is a difference between your laptop and a busy production
server reached with a slow internet connection.

rebased onto b324ef8

4 years ago

The latest change has a spinner that gets displayed while the assignees get processed

I've spent a bit of time on the UI of this PR and here is my proposal:

diff --git a/ pagure/themes/srcfpo/templates/repo_master_sidebar.html b/ pagure/themes/srcfpo/templates/repo_master_sidebar.html
index f780ae7e0..d9f95c358 100644
--- a/ pagure/themes/srcfpo/templates/repo_master_sidebar.html  
+++ b/ pagure/themes/srcfpo/templates/repo_master_sidebar.html  
@@ -144,36 +144,60 @@
     </div>
   </div>
   {% endif %}
-  <p></p>
-  <br>
-  <br>
-  Bugzilla Overrides:
-  <br>
-  <div class="row">
-    {% if g.authenticated and repo.user.user == g.fas_user.username or pagure_admin %}
-    <div class="col">
-      <form action="" id="change_assignees">
-          <label for="fedora_assignee">Fedora Assignee</label>
-          <input title="Empty input resets assignee" class="form-control" name="fedora_assignee" id="fedora_assignee" value="{{
-                    repo.bzoverride.fedora_assignee if repo.bzoverride.fedora_assignee else repo.user.user }}" size=8/>
-         <br>
-          <label for="epel_assignee">EPEL Assignee</label>
-          <input title="Empty input resets assignee" class="form-control" name="epel_assignee" id="epel_assignee"value="{{
-                    repo.bzoverride.epel_assignee if repo.bzoverride.epel_assignee else repo.user.user }}" 
-                 description="Default assignee for EPEL bugzillas"/>
-        <button class="btn btn-primary" type="submit" title="Update bugzilla overrides" id="update_assignees">
-           Update
-        </button>
-      </form>
+
+  <div>
+    <div class="col-xs-2 line-height-1">
+    <p>Bugzilla Assignee:</p>
+      <ul>
+        <li><span class="bold">Fedora: </span><span id="fedora_assignee_txt">
+        {{ repo.bzoverride.fedora_assignee if repo.bzoverride.fedora_assignee else repo.user.user }}
+        </span>
+        </li>
+        <li><span class="bold">EPEL: </span><span id="epel_assignee_txt">
+        {{ repo.bzoverride.epel_assignee if repo.bzoverride.epel_assignee else repo.user.user }}
+        </span>
+        </li>
+      </ul>
+      {% if g.authenticated and repo.user.user == g.fas_user.username or pagure_admin %}
+        <div class="btn-group">
+          <a href="#" title="Update the bugzill assignee(s)"
+              class="btn btn-sm btn-outline-primary"
+              data-toggle="modal" data-target="#modal_assignee">
+            <i id="update-bz-icon" class="fa fa-fw fa-refresh"></i>
+            Update
+          </a>
+        </div>
+      {% endif %}
+    </div>
+  </div>
+
+  <div class="modal fade" id="modal_assignee" tabindex="-1"
+          role="dialog" aria-labelledby="Bugzilla assignee" aria-hidden="true">
+    <div class="modal-dialog" role="document">
+      <div class="modal-content">
+        <div class="modal-header">
+          <h4 class="modal-title">Bugzilla Assignee</h4>
+          <button type="button" class="close" data-dismiss="modal" aria-label="Close">
+            <span aria-hidden="true">&times;</span>
+            <span class="sr-only">Close</span>
+          </button>
+        </div>
+        <div class="modal-body">
+         <form id="change_assignees">
+            <label for="fedora_assignee">Fedora</label>
+            <input title="Empty input resets assignee" class="form-control" name="fedora_assignee" id="fedora_assignee" value="{{
+                      repo.bzoverride.fedora_assignee if repo.bzoverride.fedora_assignee else repo.user.user }}"/>
+            <label for="epel_assignee">EPEL</label>
+            <input title="Empty input resets assignee" class="form-control" name="epel_assignee" id="epel_assignee" value="{{
+                      repo.bzoverride.epel_assignee if repo.bzoverride.epel_assignee else repo.user.user }}"
+                   description="Default assignee for EPEL bugzillas" />
+            <button class="btn btn-primary" type="submit" title="Update bugzilla overrides" id="update_assignees">
+               Update
+            </button>
+          </form>
+        </div>
+      </div>
     </div>
-    {% else %}
-    <div class="col">
-      Fedora Assignee:<br>
-      {{ repo.bzoverride.fedora_assignee if repo.bzoverride.fedora_assignee else repo.user.user }}
-      <br><br>
-      EPEL Assignee:<br>
-      {{ repo.bzoverride.epel_assignee if repo.bzoverride.epel_assignee else repo.user.user }}
-    {% endif %}
   </div>

   <script type="text/javascript" nonce="{{ g.nonce }}">
@@ -204,28 +228,6 @@
         }
       });

-      $("#update_assignees").click(function(){
-              $.ajax({
-                      url: "{{ url_for('distgit_ns.bzoverride_patch_endpoint', repo=repo.name, namespace=repo.namespace) }}",
-                      type: 'post',
-                      dataType: 'json',
-                      contentType: 'application/json',
-                      data: JSON.stringify({
-                         'epel_assignee': $('#epel_assignee').val(),
-                         'fedora_assignee': $('#fedora_assignee').val()
-                      }),
-                      success: function(res) {
-                              console.log("Successfully changed the bugzilla assignees")
-                      },
-                      error: function(res) {
-                              console.log(JSON.stringify({ 'epel_assignee': $('#epel_assignee').val(), 'fedora_assignee': $('#fedora_assignee').val() }))
-                              console.log("{{ url_for('distgit_ns.bzoverride_patch_endpoint', repo=repo.name, namespace=repo.namespace) }}")
-                             console.log(res.responseText)
-                             console.log(res.getAllResponseHeaders())
-                      }
-              })
-      });
-
       {% if g.authenticated %}
       $(".monitoring-menu a").click(function(){
         var selectedValue = $(this).attr('id');
@@ -253,6 +255,34 @@
       });
       {% endif %}

+      $("#change_assignees").on('submit',  function(){
+        $('html').css('cursor', 'progress');
+        $.ajax({
+          url: "{{ url_for('distgit_ns.bzoverride_patch_endpoint', repo=repo.name, namespace=repo.namespace) }}",
+          type: 'POST',
+          dataType: 'json',
+          contentType: 'application/json',
+          data: JSON.stringify({
+             'epel_assignee': $('#epel_assignee').val(),
+             'fedora_assignee': $('#fedora_assignee').val()
+          }),
+          success: function(res) {
+            console.log("Successfully changed the bugzilla assignees");
+            $("#fedora_assignee_txt").text(res.fedora_assignee);
+            $("#epel_assignee_txt").text(res.epel_assignee);
+            $('#modal_assignee').modal('hide')
+            $('html').css('cursor', 'default');
+            return false;
+          },
+          error: function(res) {
+            alert("Unable to update the bugzilla assignee(s)");
+            $('html').css('cursor', 'default');
+            return false;
+          }
+        })
+        return false;
+      });
+
       {% if g.authenticated and repo.user.user == "orphan" %}
       $("#take-orphan-button").click(function(){

I'll turn it into a patch an potentially a PR tomorrow so it's easier to review/comment on.

The modal has plenty of space to add documentation about how it works.

Looks good and somehow 'modern' to me. One typo in a title, but that's all

I'm going to close this PR as the commit (and functionality) were merged as part of: https://pagure.io/pagure/pull-request/4709

Pull-Request has been closed by pingou

4 years ago