PR#871 Merged Remove reply button for non-authn users

Proposed 2 years ago by pfrields
Modified 2 years ago
From forks/pfrields/pagure issue839  into pagure master
file changed

@@ -138,12 +138,13 @@ 

        {% endif %}

        <div class="issue_action icon pull-xs-right p-b-1">

          <div class="btn-group" role="group" aria-label="Basic example">

-           <a class="reply btn btn-secondary btn-sm" data-toggle="tooltip" title="Reply to this comment - loose formating">

-             <span class="oi" data-glyph="share-boxed"></span>

-           </a>

+           {% if id != 0 and g.fas_user and comment.parent.status in [True, 'Open'] %}

+             <a class="reply btn btn-secondary btn-sm" data-toggle="tooltip" title="Reply to this comment - loose formating">

+               <span class="oi" data-glyph="share-boxed"></span>

+             </a>

+           {% endif %}

            {% if id != 0 and g.fas_user and (

-               (comment.parent.status in [True, 'Open'] and g.fas_user.username == comment.user.username)

-               or repo_admin) %}

+               comment.parent.status in [True, 'Open'] and g.fas_user.username == comment.user.username) %}

            <a class="btn btn-secondary btn-sm edit_btn" href="{{

              '%s/comment/%s/edit' % (request.base_url, comment.id) }}"

              data-comment="{{ comment.id }}" data-objid="{{ issueid }}">

@@ -151,8 +152,7 @@

            </a>

            {% endif %}

            {% if id != 0 and g.fas_user and (

-               (comment.parent.status in [True, 'Open'] and g.fas_user.username == comment.user.username)

-               or repo_admin) %}

+               comment.parent.status in [True, 'Open'] and g.fas_user.username == comment.user.username) %}

            <button class="btn btn-secondary btn-sm" type="submit" name="drop_comment" value="{{ comment.id }}"

              onclick="return confirm('Do you really want to remove this comment?');"

              title="Remove comment">

@@ -176,10 +176,12 @@

    </section>

    <div class="issue_action icon pull-xs-right">

      <div class="btn-group" role="group">

-       <a class="btn btn-secondary btn-sm reply"

-         title="Reply to the initial comment - loose formatting">

-         <span class="oi" data-glyph="share-boxed"></span>

-       </a>

+       {% if g.fas_user %}

+         <a class="btn btn-secondary btn-sm reply"

+           title="Reply to the initial comment - loose formatting">

+           <span class="oi" data-glyph="share-boxed"></span>

+         </a>

+       {% endif %}

        {% if repo_admin or (

          g.fas_user and g.fas_user.username == comment.user.username) %}

        <a class="btn btn-primary btn-sm" href="{{

Fixes https://pagure.io/pagure/issue/839 -- now with indentation (and rebased)

:thumbsup:
Please note you can update the PR in place by force-pushing to the PR branch, and the web interface will automatically update.

for the record: this was previously PR #870

pingou commented on line 24 of pagure/templates/_formhelper.html.
2 years ago
(Hide)
One sudden thought, do we need ``repo_admin`` here? Since it can be true only if ``g.fas_user`` is not None, no?
pingou commented on line 8 of pagure/templates/_formhelper.html.
2 years ago
(Hide)
Same question for ``repo_admin`` here as below

Good questions on repo_admin, pingou... I didn't know whether pagure allowed some out-of-band users that would make g.fas_user == None. It sounds like that's not the case?

We always check for g.fas_user != None to check if an user is authenticated or not, and there isn't really any special-case users that would be using the UI.

Would g.fas_user be used (ie. different of None) by instance of pagure not using FAS for the user authentication ???

Would g.fas_user be used (ie. different of None) by instance of pagure not using FAS for the user authentication ???

Yes, however unfortunate the variable name is in this case, if you use openid or the local auth in pagure, in both cases the user's info are stored in g.fas_user.

2 years ago

Pull-Request has been rebased

2 years ago

Pull-Request has been updated

2 years ago

Pull-Request has been updated

I also removed other unnecessary "if repo_admin" statements in this PR in a separate commit.

We should rework the length of some of these lines but that's for another time.

Thanks for fixing this @pfrields! Merging :)

2 years ago

Pull-Request has been merged by pingou

Changes summary
+13 -11
file changed