Skip issuable without a project in IssuableExtractor#extract

@adamniedzielski @smcgivern What do you think?

added 1 commit

4ded8b1c - Skip issuable without a project in IssuableExtractor#extract

@godfat Thanks for the follow-up merge request

I'm not a huge fan of making the tests more DRY, because I believe that it negatively impact their readability. Of course, I'm biased here because I wrote these specs. I'll pass on to @smcgivern to express his opinion.

As to the fix, I was thinking about something more like:

diff --git a/lib/banzai/filter/issuable_state_filter.rb b/lib/banzai/filter/issuable_state_filter.rb
index 327ea94..75c6cba 100644
--- a/lib/banzai/filter/issuable_state_filter.rb
+++ b/lib/banzai/filter/issuable_state_filter.rb
@@ -15,6 +15,8 @@ module Banzai
         issuables = extractor.extract([doc])
 
         issuables.each do |node, issuable|
+          next unless issuable.project
+
           if VISIBLE_STATES.include?(issuable.state) && node.inner_html == issuable.reference_link_text(project)
             node.content += " (#{issuable.state})"
           end

but your solution looks good to me as well.

assigned to @smcgivern

assigned to @godfat

added 1 commit

58c2d941 - Follow feedback on the review

Compare with previous version

@adamniedzielski In my opinion previously it's not easier to read because it's hard to tell what's the overall behaviour. Sharing the methods as much as possible, would make it clear that they just have the same behaviour. Sorry for touching them. I didn't realize they're intended.

As for the patch you brought up, I would try hard to avoid fixes like that, because that means we might need to check in random places. In my opinion, we should filter those issues as early as possible, because we certainly don't want them almost everywhere.

@adamniedzielski @smcgivern Please review again, thanks! I also added a comment about why we're filtering the issues, as I think it could be confusing in the future.