Disable Ajax request for GPG status on repository page

added frontend label

added 2 commits

bf3387c6 - Add failing test for #35699 (closed)
47972d4d - Disable Ajax request for GPG status on repository page

assigned to @winh

added Pick into Stable Platform regression repository labels

changed milestone to %9.5

changed the description

@iamphill Thanks to your hint (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13308#note_36921629) I was able to display the badge now. Could you please review?

unmarked as a Work In Progress

assigned to @iamphill

added 31 commits

47972d4d...28299de1 - 29 commits from branch master
51618790 - Add failing test for #35699 (closed)
acb8b5bb - Disable Ajax request for GPG status on repository page

Compare with previous version

There were unrelated failing tests, so I just rebased.

resolved all discussions

approved this merge request

test failures related? There is a lot of them

assigned to @winh

canceled the automatic merge

test failures related? There is a lot of them

Yep, the first ones I saw weren't. But now they are legit.

mentioned in issue #36046

changed the description

added 532 commits

60d075c3...0ec87b3b - 530 commits from branch master
721fbdd8 - Add failing test for #35699 (closed)
f6027e56 - Disable Ajax request for GPG status on repository page

Compare with previous version

added 532 commits

60d075c3...0ec87b3b - 530 commits from branch master
721fbdd8 - Add failing test for #35699 (closed)
f6027e56 - Disable Ajax request for GPG status on repository page

Compare with previous version

resolved all discussions

added 2 commits

573fbc6c - Add failing test for #35699 (closed)
300f3363 - Disable Ajax request for GPG status on repository and search page

Compare with previous version

added 2 commits

335ee51e - Add failing test for #35699 (closed)
1a06f6b8 - Disable Ajax request for GPG status on repository and search page

Compare with previous version

@iamphill sorry about the earlier test failures—they are fixed now and the necessary changes covered by (new) tests. could you have another look please?

I'll leave it to you to decide whether this should also have a backend review.

assigned to @iamphill

here is the EE merge request: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2633

Frontend is good for me

@smcgivern Can you review the Ruby please?

assigned to @smcgivern

@dzaporozhets can you help here? You know more about this than me I thought the whole point of this was to not block the initial page load on GPG status?

assigned to @dzaporozhets

I thought the whole point of this was to not block the initial page load on GPG status?

We still have that on the commits page. Unfortunately, we have no endpoint that provides the signature for a single commit (yet). So I didn't see any way to load the signature asynchronously at other places.

We still have that on the commits page. Unfortunately, we have no endpoint that provides the signature for a single commit (yet)

@winh how about adding commit/:sha/signature? It will speed up page load and allow us to avoid load_signature_async flags everywhere.

how about adding commit/:sha/signature?

@dzaporozhets that was my initial idea (https://gitlab.com/gitlab-org/gitlab-ce/issues/35699#note_36786505) but it would require some backend work and therefore take significantly more time until the regression is fixed. but I agree that this would be the better solution.

@winh Will it take much FE code to make it happen? If not - I rather do it instead of loading signature synchronously. I can add a backend for commit/:sha/signature

Will it take much FE code to make it happen?

@iamphill Are we ok with making one request per badge?

@dzaporozhets In that case I think the necessary frontend changes would be small.

I can add a backend for commit/:sha/signature

Do you want to open a separate merge request and ping me there?

@winh I'm happy with making this all async

Do you want to open a separate merge request and ping me there?

@winh let's continue work here. I will push BE changes right into this branch

Are we ok with making one request per badge?

I'm happy with making this all async

@iamphill sorry, what I meant was: are you happy with one request per badge rather than one request for all badges on the page? (because the latter would be more complicated)

let's continue work here. I will push BE changes right into this branch

@dzaporozhets ok, thank you!

one request per badge rather than one request for all badges on the page?

@winh can you clarify on that? I though we continue to do one request per all commits on page. And one request for cases like last commit widget where it is only one commit.

So on commits page, we do commits/signatures and on Repository/Files we do commit/:sha/signature

Search/commits should also use commits/signatures

I though we continue to do one request per all commits on page.

@dzaporozhets This requires the JavaScript to know whether we are on a page with multiple badges or not (which is not impossible, just slightly more complicated).

Search/commits should also use commits/signatures

Does the signatures endpoint of the commits controller already take search terms into account? I thought that would be the business of the search controller.

If the endpoint just returns an array (even if there is a single badge) then really the JS change is minimal, we would just need to add the GpgBadges.fetch into the right places in the dispatcher

@koffeinfrei we are going to change commits/signatures endpoint here so it accepts an array of sha as an argument. This will allow us to use endpoint for search page where commits are mixed

I have a chat with @winh. Here is our plan:

commits/signatures accepts the argument as an array of sha's and returns appropriate signatures
we show signatures only on pages where we explicitly set so. No spinner in unexpected places
both search/commits and repository/tree will do ajax request to commits/signatures for signature
for search/commits and commits/index pages we do SINGLE ajax call

Just adding that there is the compare branches page and the commits of a merge request which also need to make a single request with possibly multiple SHAs.

I propose we split MR on 2 parts:

hide loading spinner for pages that can’t load signatures: all pages except commits and commit - easyfix to pick into stable
Everything else including BE and FE changes according to the plan

you create 1) separately. we continue with 2) in current MR

cc @winh @iamphill

@iamphill I dont think its regression as you pinted at https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13308#note_36919971 since the feature relates to commits and commit pages and works correctly there. Basically, there should be no spinner in other places as expected behavior for now

However fix proposed https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13308/diffs feels hacky by hiding css. Instead I think we should remove spinner from HTML template and add it only on appropriate pages by JS when AJAX request is actually started

@dzaporozhets So for now we just hide that little box on project overview page?

@iamphill so we do boring thing:

enable feature on pages it supposed to be (commits page, commit page)
prepare BE and FE for more pages support
Enable signatures on more pages like last commit widget or search/commits

basically, instead of rendering it EVERYWHERE where commit/_commit is rendered, we render signature (and load spinner) only where we explicitly say so.

@winh I updated commits/signatures to work with one argument sha which can be single string or array.

curl http://localhost:3000/gitlab-org/gitlab-test/commits/signatures?sha=5937ac0a7beb003549fc5fd26fc247a

{
  "signatures":[
    {
      "commit_sha":"5937ac0a7beb003549fc5fd26fc247adbce4a52e",
      "html":"\u003cbutton class=\"btn status-box gpg-status-box valid\" data-content=\"\u0026lt;div class=\u0026quot;clearfix\u0026quot;\u0026gt;\n\u0026lt;a class=\u0026quot;gpg-popover-user-link\u0026quot; href=\u0026quot;/root\u0026quot;\u0026gt;\u0026lt;div\u0026gt;\n\u0026lt;img class=\u0026quot;avatar has-tooltip s32  lazy\u0026quot; alt=\u0026quot;Administrator\u0026amp;#39;s avatar\u0026quot; title=\u0026quot;Administrator\u0026quot; data-container=\u0026quot;body\u0026quot; data-src=\u0026quot;http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=64\u0026amp;amp;d=identicon\u0026quot; src=\u0026quot;data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==\u0026quot; /\u0026gt;\n\u0026lt;/div\u0026gt;\n\u0026lt;div\u0026gt;\n\u0026lt;strong\u0026gt;Administrator\u0026lt;/strong\u0026gt;\n\u0026lt;div\u0026gt;@root\u0026lt;/div\u0026gt;\n\u0026lt;/div\u0026gt;\n\u0026lt;/a\u0026gt;\n\u0026lt;/div\u0026gt;\nGPG Key ID:\n\u0026lt;span class=\u0026quot;monospace\u0026quot;\u0026gt;627C5F589F467F17\u0026lt;/span\u0026gt;\n\u0026lt;a class=\u0026quot;gpg-popover-help-link\u0026quot; href=\u0026quot;/help/workflow/gpg_signed_commits/index.md\u0026quot;\u0026gt;Learn more about signing commits\u0026lt;/a\u0026gt;\n\" data-html=\"true\" data-placement=\"auto top\" data-title=\"\u0026lt;div class=\u0026quot;gpg-popover-status\u0026quot;\u0026gt;\n\u0026lt;div class=\u0026quot;gpg-popover-icon valid\u0026quot;\u0026gt;\n\u0026lt;svg width=\u0026quot;22px\u0026quot; height=\u0026quot;22px\u0026quot; viewBox=\u0026quot;0 0 22 22\u0026quot; version=\u0026quot;1.1\u0026quot; xmlns=\u0026quot;http://www.w3.org/2000/svg\u0026quot;\u0026gt;\u0026lt;path d=\u0026quot;M11.4583333,12.375 L8.70008808,12.375 C8.45889044,12.375 8.25,12.5826293 8.25,12.8387529 L8.25,14.2029137 C8.25,14.4551799 8.4515113,14.6666667 8.70008808,14.6666667 L12.9619841,14.6666667 C13.3891296,14.6666667 13.75,14.3193051 13.75,13.8908129 L13.75,13.2899463 L13.75,6.42552703 C13.75,6.16226705 13.5423707,5.95833333 13.2862471,5.95833333 L11.9220863,5.95833333 C11.6698201,5.95833333 11.4583333,6.16750307 11.4583333,6.42552703 L11.4583333,12.375 Z\u0026quot; id=\u0026quot;Combined-Shape\u0026quot; transform=\u0026quot;translate(11.000000, 10.312500) rotate(-315.000000) translate(-11.000000, -10.312500) \u0026quot;\u0026gt;\u0026lt;/path\u0026gt;\u0026lt;/svg\u0026gt;\n\n\u0026lt;/div\u0026gt;\n\u0026lt;div\u0026gt;\nThis commit was signed with a \u0026lt;strong\u0026gt;verified\u0026lt;/strong\u0026gt; signature.\n\u0026lt;/div\u0026gt;\n\n\u0026lt;/div\u0026gt;\n\" data-toggle=\"popover\"\u003e\nVerified\n\u003c/button\u003e\n\n\n"
    }
  ]
}

added 1 commit

8b7ece40 - Change commits/signatures method to accept array of sha

Compare with previous version

I'm unlabeling this regression and Pick into Stable and reopen https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13308 with a better fix. I'll talk to @timzallmann about the priority of this one.

removed Pick into Stable regression labels

removed milestone

changed the description

assigned to @winh

mentioned in merge request !13308 (closed)

added 249 commits

8b7ece40...21a6898b - 247 commits from branch master
97f251de - Add failing test for https://gitlab.com/gitlab-org/gitlab-ce/issues/35699
ff398c43 - Display GPG status loading spinner only when Ajax request is made

Compare with previous version

I just realized that I had used the same branch name here and in !13308 (closed) and accidentally pushed to this merge request, too (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13337#note_37490015). The branch is restored now.

added 3 commits

335ee51e - Add failing test for #35699 (closed)
1a06f6b8 - Disable Ajax request for GPG status on repository and search page
8b7ece40 - Change commits/signatures method to accept array of sha

Compare with previous version

mentioned in issue #36047 (closed)

The frontend part is scheduled for the next 3-6 months: https://gitlab.com/gitlab-org/gitlab-ce/issues/36046#note_37525401

So I am unassigning me for now.

removed assignee

Disable Ajax request for GPG status on repository page

What does this MR do?

Screenshots

What are the relevant issue numbers?

Activity

Admin message

Admin message

Disable Ajax request for GPG status on repository page

What does this MR do?

Screenshots

What are the relevant issue numbers?

Merge request reports

Activity