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.

Sign up for GitHub

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

Jump to bottom

skips issue opened/closed event duplicates #294

Merged
merged 2 commits into from
Aug 20, 2024
Merged

skips issue opened/closed event duplicates #294

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8474e4a
skips issue opened/closed event duplicates
tank-bohr Aug 18, 2024
50900a7
applies CR comments
tank-bohr Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions judges/github-events/github-events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

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

"(eq what \"#{what}\") " \
"(eq issue #{fact.issue}))"
).each.any?
end

def self.count_appreciated_comments(pr, issue_comments, code_comments)
issue_appreciations =
issue_comments.sum do |comment|
Expand Down Expand Up @@ -197,9 +205,11 @@ def self.fill_up_event(fact, json)
fact.issue = json[:payload][:issue][:number]
case json[:payload][:action]
when 'closed'
skip_event(json) if issue_event_exists?(fact, 'issue-was-closed')
fact.what = 'issue-was-closed'
fact.details = "The issue #{Fbe.issue(fact)} has been closed by #{Fbe.who(fact)}."
when 'opened'
skip_event(json) if issue_event_exists?(fact, 'issue-was-opened')
fact.what = 'issue-was-opened'
fact.details = "The issue #{Fbe.issue(fact)} has been opened by #{Fbe.who(fact)}."
else
Expand Down
132 changes: 132 additions & 0 deletions test/judges/test-github-events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,138 @@ def test_skip_event_when_user_equals_pr_author
assert_nil(f[1])
end

def test_skip_issue_was_opened_event
WebMock.disable_net_connect!
stub_request(:get, 'https://api.github.com/repos/foo/foo').to_return(
body: { id: 42, full_name: 'foo/foo' }.to_json, headers: {
'content-type': 'application/json'
}
)
stub_request(:get, 'https://api.github.com/repositories/42').to_return(
body: { id: 42, full_name: 'foo/foo' }.to_json, headers: {
'content-type': 'application/json'
}
)
stub_request(:get, 'https://api.github.com/repositories/42/events?per_page=100').to_return(
body: [
{
id: '40623323541',
type: 'IssuesEvent',
public: true,
created_at: '2024-07-31 12:45:09 UTC',
actor: {
id: 42,
login: 'yegor256',
display_login: 'yegor256',
gravatar_id: '',
url: 'https://api.github.com/users/yegor256'
},
repo: {
id: 42,
name: 'yegor256/judges',
url: 'https://api.github.com/repos/yegor256/judges'
},
payload: {
action: 'opened',
issue: {
number: 1347,
state: 'open',
title: 'Found a bug',
body: "I'm having a problem with this."
}
}
}
].to_json,
headers: {
'content-type': 'application/json'
}
)
stub_request(:get, 'https://api.github.com/user/42').to_return(
body: { id: 42, login: 'torvalds' }.to_json, headers: {
'content-type': 'application/json'
}
)
stub_request(:get, 'https://api.github.com/repos/yegor256/judges/issues/1347').to_return(
status: 200,
body: { number: 1347, state: 'open' }.to_json,
headers: { 'content-type': 'application/json' }
)
fb = Factbase.new
op = fb.insert
op.what = 'issue-was-opened'
op.repository = 42
op.issue = 1347
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


def test_skip_issue_was_closed_event
WebMock.disable_net_connect!
stub_request(:get, 'https://api.github.com/repos/foo/foo').to_return(
body: { id: 42, full_name: 'foo/foo' }.to_json, headers: {
'content-type': 'application/json'
}
)
stub_request(:get, 'https://api.github.com/repositories/42').to_return(
body: { id: 42, full_name: 'foo/foo' }.to_json, headers: {
'content-type': 'application/json'
}
)
stub_request(:get, 'https://api.github.com/repositories/42/events?per_page=100').to_return(
body: [
{
id: '40623323541',
type: 'IssuesEvent',
public: true,
created_at: '2024-07-31 12:45:09 UTC',
actor: {
id: 42,
login: 'yegor256',
display_login: 'yegor256',
gravatar_id: '',
url: 'https://api.github.com/users/yegor256'
},
repo: {
id: 42,
name: 'yegor256/judges',
url: 'https://api.github.com/repos/yegor256/judges'
},
payload: {
action: 'closed',
issue: {
number: 1347,
state: 'closed',
title: 'Found a bug',
body: "I'm having a problem with this."
}
}
}
].to_json,
headers: {
'content-type': 'application/json'
}
)
stub_request(:get, 'https://api.github.com/user/42').to_return(
body: { id: 42, login: 'torvalds' }.to_json, headers: {
'content-type': 'application/json'
}
)
stub_request(:get, 'https://api.github.com/repos/yegor256/judges/issues/1347').to_return(
status: 200,
body: { number: 1347, state: 'closed' }.to_json,
headers: { 'content-type': 'application/json' }
)
fb = Factbase.new
op = fb.insert
op.what = 'issue-was-closed'
op.repository = 42
op.issue = 1347
load_it('github-events', fb)
f = fb.query('(eq what "issue-was-closed")').each.to_a
assert_equal(1, f.length)
end

def test_watch_pull_request_review_events
WebMock.disable_net_connect!
stub_request(:get, 'https://api.github.com/repos/foo/foo').to_return(
Expand Down