Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

skips issue opened/closed event duplicates #294

Merged
merged 2 commits into from
Aug 20, 2024
Merged

Conversation

tank-bohr
Copy link
Contributor

fixes #293

@tank-bohr
Copy link
Contributor Author

@Suban05 @Yegorov please take a look

Copy link
Contributor

@Yegorov Yegorov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tank-bohr Looks good, but need add more test cases.

load_it('github-events', fb)
f = fb.query('(eq what "issue-was-opened")').each.to_a
assert_equal(1, f.length)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tank-bohr Maybe need add test for issue-was-closed case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tank-bohr I agree with @Yegorov. We need a test for issue-was-closed case

@tank-bohr
Copy link
Contributor Author

@Yegorov @Suban05 I've just added the test for issue-was-closed

Copy link
Contributor

@Suban05 Suban05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tank-bohr all good, but I have a small comment

@@ -98,6 +98,14 @@ def self.comments_info(pr)
}
end

def self.issue_event_exists?(fact, what)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tank-bohr The self.issue_event_exists? method checks whether an issue exists or not, but based on the name of the method, it seems that the method is checking for the existence of an event in an issue. What about the name self.issue_exists??
@Yegorov What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Suban05 I find it difficult to comment, but the event is called IssuesEvent, but on the other hand, an issue is being processed here.
@yegor256 what do you think?

Copy link
Member

@yegor256 yegor256 Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yegorov I would call it self.issue_seen_already?(fact, what), since we are checking the presence of a fact with the given what

Copy link
Contributor

@Yegorov Yegorov Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yegor256 issue_seen_already? - readable name.

@Suban05 @yegor256 What do you think if call issue_seen_already? method after get what fact, we may not use the last argument in this method. For example:

     def self.issue_seen_already?(fact)
        Fbe.fb.query(
          "(and (eq repository #{fact.repository}) " \
          "(eq what #{fact.what}) " \
          "(eq issue #{fact.issue}))"
        ).each.any?
      end

      # ...
      case json[:payload][:action]
      when 'closed'
        fact.what = 'issue-was-closed'
        skip_event(json) if issue_seen_already?(fact)
        fact.details = "The issue #{Fbe.issue(fact)} has been closed by #{Fbe.who(fact)}."
      when 'opened'
        fact.what = 'issue-was-opened'
        skip_event(json) if issue_seen_already?(fact)
        fact.details = "The issue #{Fbe.issue(fact)} has been opened by #{Fbe.who(fact)}."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yegorov excellent idea! Maybe, we can do this checking at the end of the entire processing of all events? We don't want to have duplicates with any of them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yegor256 Good point, as an option I think yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yegorov yes, perfect, it looks much better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set what before checking the factbase, will the issue_seen_already? find this fact exactly?

Copy link
Member

@yegor256 yegor256 Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tank-bohr good point. You can do this:

count = Fbe.fb.query("
  (agg 
    (and 
      (eq repository #{fact.repository}) 
      (eq what #{fact.what}) 
      (eq issue #{fact.issue}))
    (count))
).each.to_a.first

You should get a count of facts that satisfy the (and ...) term. If the count is larger than 1, you can do skip_event.

You can also do a simple Ruby counting:

count = Fbe.fb.query("
    (and 
      (eq repository #{fact.repository}) 
      (eq what #{fact.what}) 
      (eq issue #{fact.issue}))
).each.to_a.count

@@ -98,6 +98,14 @@ def self.comments_info(pr)
}
end

def self.issue_event_exists?(fact, what)
Fbe.fb.query(
"(and (eq repository #{fact.repository}) " \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tank-bohr don't forget about where, it should be set to github. We may have GitLab, for example, in the same factbase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yegor256 But can we handle the events of gitlab in a file that called github - judges/github-events/github-events.rb ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yegorov In the github-events file, we process events only from github, I think it is proposed to put a filter only on events from github like eq where 'github', so as not to accidentally receive events from gitlab, for example

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Suban05 @yegor256 ok, understood (the search was taking place in the general storage Fbe.fb).
Then maybe there is the same check should be done here - #278

@tank-bohr
Copy link
Contributor Author

@yegor256 updated

@yegor256 yegor256 merged commit 562fba4 into zerocracy:master Aug 20, 2024
8 checks passed
@yegor256
Copy link
Member

@tank-bohr thanks!

@tank-bohr tank-bohr deleted the 293 branch August 20, 2024 08:18
@0crat
Copy link

0crat commented Aug 20, 2024

@Yegorov Hey there! 👋 Great job on the review, although we'd love to see more comments next time. You've snagged +4 points: +8 for the review, -10 for having only 2 comments (we aim for at least 6), and +6 to keep you motivated. Remember, more hits-of-code and comments can boost your bonus up to 32 points! Your current balance is +431. Keep up the good work! 💪

@0crat
Copy link

0crat commented Aug 20, 2024

@Suban05 Great job on the review! 🌟 You've earned +4 points: +8 base, -10 for limited comments (4), +6 bonus. Keep up the good work! Your current balance is +452.

@0crat
Copy link

0crat commented Aug 20, 2024

@yegor256 Hey there, awesome job on the review! 🎉 You've snagged +8 points, which is pretty sweet considering all the factors at play. Your running balance is now +12. Keep up the great work – remember, more comments and hits-of-code can boost your rewards even higher. Looking forward to your next contribution!

@0crat
Copy link

0crat commented Aug 20, 2024

@tank-bohr Hey there, awesome work on your contribution! 🎉 You've scored a sweet +31 points: 16 for showing up, and an extra 14 for those 147 hits-of-code you cranked out. That's some solid coding! Your balance is now sitting pretty at +120. Keep the momentum going and remember, quality matters just as much as quantity. Looking forward to seeing what you'll bring to the table next!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

duplicate issue-was-closed fact
5 participants