Add ability to attach merge request

Note to reviewer

• API calls are very slow, please monitor the network tab to make sure they are still loading (See https://gitlab.com/gitlab-org/gitlab-ce/issues/32042#note_33258115 for more details about API slowness)
• Trello allows you to create views for custom content but for list content, you have to use their API to display them
• Trello's list content API does not have a good debounce setup, so we are not doing search queries on the GitLab API even though that was an option in the beginning
• Project list retrieved from API is sorted based on last updated. Will need to check with @victorwu whether that is desired or not. Our projects are quite active and I have seen different projects jump to the top of the list.

What does this MR do?

• Adds the ability to save your auth token and api url - Closes #22 (closed)
• Automatically add the GitLab logo for attachments that are from gitlab.com - Closes #21 (closed)
• Adds the GitLab card button - Closes #2 (closed)
• Card button has the ability to search for a project and a merge request - Closes #4 (closed), #5 (closed)
• Card button has the ability to attach a merge request to the Trello card - Closes #6 (closed)
• Contains the general UI flow of this trello power up - Closes #1 (closed)

What does not work?

• Attach Issue list item button does not work when you click the GitLab card button #20 (closed)
• Authorize with GitLab.com button does not work at the moment #15
• No error messages/catch blocks at the moment #19 (closed)
• No view for Edit Power-Up Settings #14 (closed)

marked as a Work In Progress

unmarked as a Work In Progress

marked as a Work In Progress

changed the description

unmarked as a Work In Progress

@filipa can you review please?

assigned to @filipa

changed the description

@ClemMakesApps a couple of notes:

• what does t stands for? Can we make it more obvious?
• All the catch are missing, can we fix it in this MR?
• Do we need to use the window object?
• There are some functions being declared inside the .then, why?

How can I test this?

assigned to @ClemMakesApps

resolved all discussions

• what does t stands for? Can we make it more obvious?

It seems like the convention used in https://developers.trello.com/power-ups/client-library, we might be able to get away by renaming it to trelloInstance. Some of the challenges with t is that each t in each callback is a mutated version of the previous t. I had a few cases where I tried to use the previous version of t and it would create an ambiguous error when testing on trello.com

• All the catch are missing, can we fix it in this MR?

Yes, I have listed it as a follow up issue in the MR description

• Do we need to use the window object?

Yes if we don't add any additional libraries. Ideally, I'd like to add webpack to the build process so we can avoid this in a future iteration

• There are some functions being declared inside the .then, why?

Could you re-clarify this question? Not sure what you are asking here

• How can I test this?

There are steps on testing in the readme file

resolved all discussions

assigned to @filipa

assigned to @mikegreiling

Still playing around with this, but here are some initial thoughts.

1. The ID you're using does not appear to correspond to the MR reference id !1234 but rather some internal database id. In the MR below, the ref should be !1 but it shows up as !4056092.

2. When I initially configured the trello powerup, I used gitlab.com/api/v4/ (omitting the https:// part) and this didn't produce any errors until I tried to use the powerup and looked in the dev console to notice 404 responses from https://5f7b388a.ngrok.io/gitlab.com/api/v4/projects?membership=true&private_token=...

• we should validate the URL upon configuration
• we should enforce https and produce an error if http is specified (otherwise your personal access token will be sent in the clear )

3. I think for the GitLab icon in the sidebar, we should try to use the same gray color to allow it to match the other icons above it. Right now the black and white is a bit jarring next to the others. I'm not sure how the other integrations handle this though, so maybe it's not a big deal.

I think for the GitLab icon in the sidebar, we should try to use the same gray color to allow it to match the other icons above it. Right now the black and white is a bit jarring next to the others. I'm not sure how the other integrations handle this though, so maybe it's not a big deal.

Good catch, let me update that to a gray. I just checked the GitHub integration, and they are using gray

4. Can we prevent the same MR from being attached multiple times to the same card? Also, when clicking on a MR to attach it, we should probably close the selector. I got confused the first time and just clicked it a few times, resulting in this:

The ID you're using does not appear to correspond to the MR reference id !1234 but rather some internal database id. In the MR below, the ref should be !1 but it shows up as !4056092.

Good catch on this one, I've pushed a fix for it

When I initially configured the trello powerup, I used gitlab.com/api/v4/ (omitting the https:// part) and this didn't produce any errors until I tried to use the powerup and looked in the dev console to notice 404 responses from https://5f7b388a.ngrok.io/gitlab.com/api/v4/projects?membership=true&private_token=...

• we should validate the URL upon configuration

• we should enforce https and produce an error if http is specified (otherwise your personal access token will be sent in the clear )

I have #13 (closed) as an issue to track that/implement that feature

resolved all discussions

is mapMergeRequest used elsewhere?

If it isn't does it make sense to just move the content to inside the map function?

Not at the moment. I just broke out all the functions separately so that it was easier to determine what was a function and what wasn't since ES5 function callbacks aren't the prettiest.

Breaking it out may make it easier to test in the future, since everything is a function

Can we prevent the same MR from being attached multiple times to the same card? Also, when clicking on a MR to attach it, we should probably close the selector. I got confused the first time and just clicked it a few times, resulting in this:

Based on my code, it should be auto closing.. as seen in https://gitlab.com/gitlab-org/trello-power-up/blob/attach-merge-request/public/scripts/mergeRequest.js#L7-11 which matches up with their sample code in https://glitch.com/edit/#!/trello-power-up?path=public/js/client.js:181:16.

I'll need to reach out to Trello team to figure out whats going on

same as above, is mapProjects used elsewhere?

return response.data.map(function (p) {
  return {
    text: p.name_with_namespace,
    callback: function selectProject(t) {
      return mergeRequest.showProjectMergeRequest(t, p.id, p.name_with_namespace);
    }
  };
});

Same response as https://gitlab.com/gitlab-org/trello-power-up/merge_requests/7#note_33835215

mentioned in issue #21 (closed)

I commented in https://gitlab.com/gitlab-org/trello-power-up/issues/21#note_33835094 regarding the thumbnail icon we're using with the GitLab logo next to the attachments. I think it may be better to use attachment sections with the gitlab logo in the header.

Ups, sorry @mikegreiling did not see this was assigned to you ! Going to leave eheh

@filipa it never hurts to have more eyes on it

I'd prefer if both of you looked at this if y'all are down for it

I commented in https://gitlab.com/gitlab-org/trello-power-up/issues/21#note_33835094 regarding the thumbnail icon we're using with the GitLab logo next to the attachments. I think it may be better to use attachment sections with the gitlab logo in the header.

Yes, I agree. Though I also want to keep the iterations small, so I've left it as a logo for now.

Can we keep #21 (closed) open after this MR then? Or should I put those comments into a separate issue?

Can we keep #21 (closed) open after this MR then? Or should I put those comments into a separate issue?

I created #25 (closed) to track that

The attachment when viewed on the trello card doesn't display the MR title anywhere, just the project and the merge request ID.

The attachment when viewed on the trello card doesn't display the MR title anywhere, just the project and the merge request ID.

I created https://gitlab.com/gitlab-org/trello-power-up/issues/26 to track that. I just followed GitHub's convention for the time being

This LGTM as an iteration. I presume we wouldn't release this until other issues are addressed (the setup/settings process, support for issues, less-slow API response, etc), so I'm fine approving this MR

approved this merge request

Unrelated to this MR, but is there an /approve slash command? If so it didn't seem to work when I tried it just now (I ended up using the MR widget instead)

Yes of course @mikegreiling, still needs a few things before it is ready to be submitted to Trello.

A lot of it is hinging on the API improving as well. If API doesn't improve in 9.4, this won't get released until it gets fixed

Unrelated to this MR, but is there an /approve slash command? If so it didn't seem to work when I tried it just now (I ended up using the MR widget instead)

I don't think so maybe a separate issue for that?

@mikegreiling did you test this locally?

assigned to @filipa

mentioned in issue #27 (closed)

@filipa yes I did. The bugs I noticed all have either been resolved, were already known/mentioned in the description, or been given their own issues.

Going to merge based in your review @mikegreiling! I don't want to delay this more and I think it is safe to merge. I will test the next iteration locally!

Thanks!

Thanks @filipa and @mikegreiling for being diligent!

merged

changed milestone to %1.0