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

Add ProjectCard attribute to events model #2102

Merged
merged 3 commits into from
Feb 24, 2020

Conversation

maxim-lobanov
Copy link
Contributor

Hello!
I have faced with issue that Event model doesn't contain project_card property that is important for all events that are related to GitHub boards (Projects).
For example, for event added_to_project, it contains the name of column where issue is added.
For event moved_columns_in_project, it contains the previous and new column names.
This PR includes the following changes:

  1. Change EventInfo model to IssueEvent (please see note below for details)
  2. Add model IssueEventProjectCard accordingly to GitHub API description
  3. Add header IssueEventsApiPreview = "application/vnd.github.starfox-preview" that is needed to get project_card value
  4. Update tests
  5. Update Octokit.Reactive with the same changes

Note: Why EventInfo model was replaced by IssueEvent in IssueClient:
These models are pretty similar but IssueEvent contains additional fields like Issue and ProjectCard.
I am not sure why some methods in IssueClient used IssueEvent but some methods used EventInfo. Based on API, all methods in IssueClient should use IssueEvent.

@codecov
Copy link

codecov bot commented Feb 17, 2020

Codecov Report

Merging #2102 into master will decrease coverage by 0.05%.
The diff coverage is 57.14%.

@@            Coverage Diff            @@
##           master   #2102      +/-   ##
=========================================
- Coverage   67.05%     67%   -0.06%     
=========================================
  Files         538     539       +1     
  Lines       14141   14185      +44     
=========================================
+ Hits         9482    9504      +22     
- Misses       4659    4681      +22
Impacted Files Coverage Δ
Octokit/Models/Response/EventInfo.cs 4.54% <ø> (ø) ⬆️
Octokit/Helpers/AcceptHeaders.cs 100% <ø> (ø) ⬆️
Octokit/Models/Response/TimelineEventInfo.cs 3.33% <0%> (-0.24%) ⬇️
Octokit/Models/Response/IssueEvent.cs 6.66% <0%> (-1.03%) ⬇️
Octokit/Models/Response/IssueEventProjectCard.cs 0% <0%> (ø)
...t.Reactive/Clients/ObservableIssuesEventsClient.cs 100% <100%> (ø) ⬆️
Octokit/Clients/IssueTimelineClient.cs 100% <100%> (ø) ⬆️
Octokit/Clients/IssuesEventsClient.cs 100% <100%> (ø) ⬆️

@maxim-lobanov
Copy link
Contributor Author

@shiftkey , could you please take a look at this PR?
I have noticed that some test coverage checks failed due to new model that I have introduced for the project cards. Since it is just API models I am not sure what tests we need here so we should be okay with these changes.
If there is anything that should be improved, please let me know

@shiftkey
Copy link
Member

I have noticed that some test coverage checks failed due to new model that I have introduced for the project cards. Since it is just API models I am not sure what tests we need here so we should be okay with these changes.

This is fine. I think I need to tweak the Codecov settings to focus on certain namespaces over others.

I am not sure why some methods in IssueClient used IssueEvent but some methods used EventInfo. Based on API, all methods in IssueClient should use IssueEvent.

I am not sure about this either, and need to check the history to understand why these are different.

@maxim-lobanov
Copy link
Contributor Author

maxim-lobanov commented Feb 20, 2020

I am not sure about this either, and need to check the history to understand why these are different.

@shiftkey ,
Unfortunately, nothing is in history. It was introduced with the first commit when IssueEventClient was implemented.

I believe that my change is safe enough because:

  1. EventInfo model:
public long Id { get; protected set; }
public string NodeId { get; protected set; }
public string Url { get; protected set; }
public User Actor { get; protected set; }
public User Assignee { get; protected set; }
public Label Label { get; protected set; }
public StringEnum<EventInfoState> Event { get; protected set; }
public string CommitId { get; protected set; }
public DateTimeOffset CreatedAt { get; protected set; }
  1. IssueEvent model:
public long Id { get; protected set; }
public string NodeId { get; protected set; }
public string Url { get; protected set; }
public User Actor { get; protected set; }
public User Assignee { get; protected set; }
public Label Label { get; protected set; }
public StringEnum<EventInfoState> Event { get; protected set; }
public string CommitId { get; protected set; }
public string CommitUrl { get; protected set; }
public DateTimeOffset CreatedAt { get; protected set; }
public Issue Issue { get; protected set; }

As you can see, IssueEvent contains all fields that EventInfo has.
But, also, it contains the following additional fields (which exist only for events that come with issues) :

  1. public string CommitUrl { get; protected set; }
  2. public Issue Issue { get; protected set; }
    So we just extend model.

I have changed EventInfo-> IssueEvent for the method GetAllForIssue. Based on GitHub API, this method should return Issue field so we should use IssueEvent model here.

Copy link
Member

@shiftkey shiftkey left a comment

Choose a reason for hiding this comment

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

@maxim-lobanov this is looking really great - thanks for the contribution.

Only bit of feedback I have is about updating the tests to assert on the preview accept header, everything else looks great!

Octokit.Tests/Clients/IssuesEventsClientTests.cs Outdated Show resolved Hide resolved
Octokit/Models/Response/TimelineEventInfo.cs Show resolved Hide resolved
@shiftkey shiftkey self-assigned this Feb 24, 2020
@shiftkey shiftkey merged commit fe6639f into octokit:master Feb 24, 2020
@shiftkey
Copy link
Member

release_notes: added ProjectCard property to IssueEvent response model

@nickfloyd nickfloyd added Type: Bug Something isn't working as documented and removed category: bug labels Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants