Commenting and declining support for pull requests

dploeger commented 6 years ago

Fixes #67

dploeger commented 6 years ago

@seletskiy This would be my first idea about this. Please check it out.

dploeger commented 5 years ago

Okay, I've just read about the event listeners. I will switch storing, if a comment has already been added to a PR, to ActiveObjects and write event listeners on PR, repo and project delete events to remove old data.

seletskiy commented 5 years ago

@dploeger: great, looking forward to it

dploeger commented 5 years ago

@seletskiy I've added that now. I use an ActiveObjects entity to store wether a check was already carried out for the current version of the PR (new pushes increase that version) and wether comments were added.

I implemented that check before running the external scripts, too, so scripts aren't run twice for the same PR version (speeds up the process).

Also, I added event listeners, so if a PR or a repository is removed, the corresponding entries in the ActiveObjects entity is removed as well.

To implement this all, I needed to add the spring-scanner based dependency injection patterns, that newer plugins use (the use of in atlassian-plugin.xml is deprecated). I'd recommend, that you update the plugin to the more current design patterns to be safe with newer versions of bitbucket.

dploeger commented 5 years ago

@seletskiy Do you want to merge this PR? Is something holding you back?

seletskiy commented 5 years ago

@dploeger: Hi, sorry, yes. Just didn't have time to test it, will do soon.

dploeger commented 5 years ago

Okay, added the requested changes.

seletskiy commented 5 years ago

@dploeger: Hey! Thanks for fixes. I've tested it again and it seems that new fixes introduced error:

[INFO] 2018-09-25 12:07:25,755 WARN  [http-nio-7990-exec-4] admin @13G9KQ0x727x231x0 5g316v fe80:0:0:0:3613:e8ff:fe5a:f37%13 "GET /rest/api/latest/projects/PROJECT_1/repos/rep_1/pull-requests/5/merge HTTP/1.1" c.a.s.i.h.r.DefaultRepositoryHookService [PROJECT_1/rep_1[1]] Error calling com.ngs.stash.externalhooks.hook.ExternalMergeCheckHook.preUpdate (com.ngs.stash.externalhooks.external-hooks:external-merge-check-hook)
[INFO] java.lang.UnsupportedOperationException: null
[INFO]  at com.google.common.collect.ImmutableCollection.removeAll(ImmutableCollection.java:132)
[INFO]  at com.ngs.stash.externalhooks.hook.ExternalMergeCheckHook.preUpdate(ExternalMergeCheckHook.java:246)
[INFO]  at com.ngs.stash.externalhooks.hook.ExternalMergeCheckHook.preUpdate(ExternalMergeCheckHook.java:58)
[INFO]  at com.atlassian.stash.internal.hook.repository.DefaultRepositoryHookService.preUpdate(DefaultRepositoryHookService.java:860)
[INFO]  at com.atlassian.stash.internal.hook.repository.DefaultRepositoryHookService.lambda$preUpdate$4(DefaultRepositoryHookService.java:458)
[INFO]  at org.springframework.transaction.support.TransactionTemplate.execute(TransactionTemplate.java:133)
[INFO]  at com.atlassian.stash.internal.hook.repository.DefaultRepositoryHookService.preUpdate(DefaultRepositoryHookService.java:437)
[INFO]  at com.atlassian.stash.internal.pull.DefaultMergeRequestCheckService.doCheck(DefaultMergeRequestCheckService.java:64)
[INFO]  at com.atlassian.stash.internal.pull.DefaultMergeRequestCheckService.checkMergeability(DefaultMergeRequestCheckService.java:48)
[INFO]  at com.atlassian.stash.internal.pull.DefaultPullRequestService.canMerge(DefaultPullRequestService.java:251)
[INFO]  at com.atlassian.plugin.util.ContextClassLoaderSettingInvocationHandler.invoke(ContextClassLoaderSettingInvocationHandler.java:26)
[INFO]  at org.eclipse.gemini.blueprint.service.importer.support.internal.aop.ServiceInvoker.doInvoke(ServiceInvoker.java:56)
[INFO]  at org.eclipse.gemini.blueprint.service.importer.support.internal.aop.ServiceInvoker.invoke(ServiceInvoker.java:60)
[INFO]  at org.eclipse.gemini.blueprint.service.util.internal.aop.ServiceTCCLInterceptor.invokeUnprivileged(ServiceTCCLInterceptor.java:70)
[INFO]  at org.eclipse.gemini.blueprint.service.util.internal.aop.ServiceTCCLInterceptor.invoke(ServiceTCCLInterceptor.java:53)
[INFO]  at org.eclipse.gemini.blueprint.service.importer.support.LocalBundleContextAdvice.invoke(LocalBundleContextAdvice.java:57)
[INFO]  at com.atlassian.stash.internal.rest.pull.PullRequestResource.canMerge(PullRequestResource.java:480)
[INFO]  at com.atlassian.applinks.core.rest.context.ContextFilter.doFilter(ContextFilter.java:24)
[INFO]  at com.atlassian.applinks.core.rest.context.ContextFilter.doFilter(ContextFilter.java:24)
[INFO]  at com.atlassian.applinks.core.rest.context.ContextFilter.doFilter(ContextFilter.java:24)
[INFO]  at com.atlassian.applinks.core.rest.context.ContextFilter.doFilter(ContextFilter.java:24)
[INFO]  at com.atlassian.applinks.core.rest.context.ContextFilter.doFilter(ContextFilter.java:24)
[INFO]  at com.atlassian.analytics.client.filter.UniversalAnalyticsFilter.doFilter(UniversalAnalyticsFilter.java:92)
[INFO]  at com.atlassian.analytics.client.filter.AbstractHttpFilter.doFilter(AbstractHttpFilter.java:39)
[INFO]  at com.atlassian.labs.httpservice.resource.ResourceFilter.doFilter(ResourceFilter.java:59)
[INFO]  at com.atlassian.stash.internal.spring.lifecycle.LifecycleJohnsonServletFilterModuleContainerFilter.doFilter(LifecycleJohnsonServletFilterModuleContainerFilter.java:42)
[INFO]  at com.atlassian.plugin.connect.plugin.auth.scope.ApiScopingFilter.doFilter(ApiScopingFilter.java:81)
[INFO]  at com.atlassian.stash.internal.spring.lifecycle.LifecycleJohnsonServletFilterModuleContainerFilter.doFilter(LifecycleJohnsonServletFilterModuleContainerFilter.java:42)
[INFO]  at com.atlassian.stash.internal.spring.security.StashAuthenticationFilter.doFilter(StashAuthenticationFilter.java:85)
[INFO]  at com.atlassian.stash.internal.web.auth.BeforeLoginPluginAuthenticationFilter.doInsideSpringSecurityChain(BeforeLoginPluginAuthenticationFilter.java:112)
[INFO]  at com.atlassian.stash.internal.web.auth.BeforeLoginPluginAuthenticationFilter.doFilter(BeforeLoginPluginAuthenticationFilter.java:75)
[INFO]  at com.atlassian.security.auth.trustedapps.filter.TrustedApplicationsFilter.doFilter(TrustedApplicationsFilter.java:94)
[INFO]  at com.atlassian.oauth.serviceprovider.internal.servlet.OAuthFilter.doFilter(OAuthFilter.java:67)
[INFO]  at com.atlassian.stash.internal.spring.lifecycle.LifecycleJohnsonServletFilterModuleContainerFilter.doFilter(LifecycleJohnsonServletFilterModuleContainerFilter.java:42)
[INFO]  at com.atlassian.plugin.connect.plugin.auth.oauth2.DefaultSalAuthenticationFilter.doFilter(DefaultSalAuthenticationFilter.java:69)
[INFO]  at com.atlassian.plugin.connect.plugin.auth.user.ThreeLeggedAuthFilter.doFilter(ThreeLeggedAuthFilter.java:109)
[INFO]  at com.atlassian.jwt.internal.servlet.JwtAuthFilter.doFilter(JwtAuthFilter.java:32)
[INFO]  at com.atlassian.analytics.client.filter.DefaultAnalyticsFilter.doFilter(DefaultAnalyticsFilter.java:38)
[INFO]  at com.atlassian.analytics.client.filter.AbstractHttpFilter.doFilter(AbstractHttpFilter.java:39)
[INFO]  at com.atlassian.stash.internal.spring.lifecycle.LifecycleJohnsonServletFilterModuleContainerFilter.doFilter(LifecycleJohnsonServletFilterModuleContainerFilter.java:42)
[INFO]  at com.atlassian.stash.internal.web.auth.BeforeLoginPluginAuthenticationFilter.doBeforeBeforeLoginFilters(BeforeLoginPluginAuthenticationFilter.java:90)
[INFO]  at com.atlassian.stash.internal.web.auth.BeforeLoginPluginAuthenticationFilter.doFilter(BeforeLoginPluginAuthenticationFilter.java:73)
[INFO]  at com.atlassian.stash.internal.request.DefaultRequestManager.doAsRequest(DefaultRequestManager.java:89)
[INFO]  at com.atlassian.stash.internal.hazelcast.ConfigurableWebFilter.doFilter(ConfigurableWebFilter.java:38)
[INFO]  at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[INFO]  at java.lang.Thread.run(Thread.java:748)
[INFO]  ... 345 frames trimmed

I see it when I enable Merge Check script that exit 1 with following flag set:

[x] Add comments to the pull request, when it is rejected

dploeger commented 5 years ago

Argh, sorry. That was quite dumb code there. I fixed it.

seletskiy commented 5 years ago

@dploeger: now it adds comment that states 'See Pull-Request comments for more info' :rofl:

screenshot

dploeger commented 5 years ago

Ah, damn. Sorry. That's why you don't implement stuff without a working test suite. 😆 I'll fix that.

dploeger commented 5 years ago

There. I had to rework the result handling workflow. I think, it's also cleaner now. Please check it out. One thing though: If you receive a multi line stdout from the check: Were the newlines preservered when displaying the rejection in the "Merge"-Tooltip? They are converted to \<br/>s in the comment, but not in the tooltip... 🤔

seletskiy commented 5 years ago

@dploeger: Thanks, I will check it soon.

Were the newlines preservered when displaying the rejection in the "Merge"-Tooltip?

Nope, I see same behavior. I don't even know does Bitbucket allow to use any formatting in this tooltip at all or not.

Also, we have related issue regarding this: https://github.com/reconquest/atlassian-external-hooks/issues/66

dploeger commented 5 years ago

👍 Good. Then I don't have to care. 😉

dploeger commented 5 years ago

@seletskiy Got any time to merge this?

seletskiy commented 5 years ago

@dploeger: yeah, sorry, got it completely out of my mind.

I tested everything and it seems to work correctly now.

Thanks for a great contribution!

reconquest / atlassian-external-hooks

Commenting and declining support for pull requests #68