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

[#1959] Missing contribution bar for merged groups after refresh #1960

Merged
merged 10 commits into from
Apr 4, 2023
Merged

[#1959] Missing contribution bar for merged groups after refresh #1960

merged 10 commits into from
Apr 4, 2023

Conversation

MarcusTXK
Copy link
Member

@MarcusTXK MarcusTXK commented Mar 20, 2023 •
edited
Loading

Fixes #1959

Proposed commit message

Currently, upon refreshing the page, the contribution bar disappears 
due to getContributionBars returning NaN instead of the expected array. 
This was caused by division by 0, due to the user's 
checkedFileTypeContribution not being initialized properly. 

Let's fix this checkedFileTypeContribution not being initialized 
properly and add guard clauses against division by 0.

Other information

This was likely introduced in PR #1903 when the initial value of checkedFileTypeContribution was refactored to be set as 0, when it was previously expected to be undefined. This resulted in this.updateCheckedFileTypeContribution(user) not being called when the page was refreshed, and resulted in both totalLines and totalCount to be 0. There were no guard clauses for division of 0 which resulted in NaN being returned for avgContributionSize(), causing cnt to be NaN as well when it was divided by contributionLimit .

@MarcusTXK MarcusTXK changed the title [#1959] Missing contribution bar for merged groups [#1959] Missing contribution bar for merged groups after refresh Mar 20, 2023
@MarcusTXK MarcusTXK requested a review from a team March 20, 2023 19:18
@ckcherry23
Copy link
Member

Hi @MarcusTXK, this is a great catch, and thanks for taking up the issue!
However, it seems like the cypress test for the contribution bar is falling, can you take a look?

MarcusTXK
MarcusTXK commented Mar 24, 2023
View reviewed changes
frontend/src/views/c-summary.vue
Comment on lines +213 to +215
if (totalCount === 0) {
return 0;
}
Copy link
Member Author

@MarcusTXK MarcusTXK Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Interestingly, this is what is causing the cypress test to fail. By adding this guard clause, the browser hangs if you check "breakdown by file type", uncheck "All" and then try to check any one of the file types. There is a part of the frontend that only works when the output is NaN due to the division of 0.

vvidday
vvidday reviewed Mar 27, 2023
View reviewed changes
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

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

The changes LGTM, thanks for catching this! Do you think it will be helpful to add a cypress test case to make sure this functionality doesn't break in the future?

@MarcusTXK
Copy link
Member Author

MarcusTXK commented Mar 27, 2023 •
edited
Loading

The changes LGTM, thanks for catching this! Do you think it will be helpful to add a cypress test case to make sure this functionality doesn't break in the future?

I think that is a great suggestion to help detect such issues in advance in the future. Would it be a good idea to add this as a separate issue?

@vvidday
Copy link
Contributor

vvidday commented Mar 27, 2023

The changes LGTM, thanks for catching this! Do you think it will be helpful to add a cypress test case to make sure this functionality doesn't break in the future?

I think that is a great suggestion to help detect such issues in advance in the future. Would it be a good idea to add this as a separate issue?

Yup I think it will be good to add it as a separate issue, especially since this is a bug fix and not a new feature

@MarcusTXK MarcusTXK mentioned this pull request Mar 27, 2023
Closed
ckcherry23
ckcherry23 reviewed Mar 27, 2023
View reviewed changes
frontend/src/utils/api.ts Outdated
@@ -269,7 +269,7 @@ window.api = {
searchPath: searchParams.join('_').toLowerCase(),
repoName: `${repo.displayName}`,
location: `${repo.location.location}`,
checkedFileTypeContribution: 0,
checkedFileTypeContribution: -1,
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we are using -1 instead of undefined here as the default value? I feel that undefined or maybe null would be more intuitive for developers.

Copy link
Member Author

Choose a reason for hiding this comment

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

The initial reason was so I did not want to deal with undefined, however I do not have a strong opinion on this. Let me update it to undefined.

@ckcherry23 ckcherry23 requested a review from a team March 27, 2023 07:19
HCY123902
HCY123902 requested changes Mar 27, 2023
View reviewed changes
frontend/src/components/c-summary-charts.vue Outdated
@@ -318,7 +322,7 @@ export default {
contributionBars.push(fullBarWidth);
}
const remainingBarWidth = barWidth % fullBarWidth;
if (remainingBarWidth !== 0) {
if (remainingBarWidth >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be a > or >= here

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, let me change this.

frontend/src/views/c-summary.vue Outdated
@@ -201,7 +201,7 @@ export default {
let totalCount = 0;
this.repos.forEach((repo) => {
repo.users.forEach((user) => {
if (user.checkedFileTypeContribution === undefined) {
if (user.checkedFileTypeContribution === -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I also think it might be more appropriate to replace -1 with values such as undefined as default. Can you take a look again

@HCY123902 HCY123902 requested a review from a team March 29, 2023 08:16
zhoukerrr
zhoukerrr approved these changes Apr 1, 2023
View reviewed changes
Copy link
Contributor

@zhoukerrr zhoukerrr left a comment

Choose a reason for hiding this comment

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

LGTM. It was a good catch. Just remember to add a test on this to make sure we do not repeat the same bug in future.

@MarcusTXK MarcusTXK mentioned this pull request Apr 2, 2023
Merged
@dcshzj
Copy link
Member

dcshzj commented Apr 4, 2023

@MarcusTXK Are you able to resolve the issues with the undefined type?

@dcshzj dcshzj merged commit bfb648b into reposense:master Apr 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

The following links are for previewing this pull request:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vvidday vvidday vvidday left review comments

@ckcherry23 ckcherry23 ckcherry23 left review comments

@dcshzj dcshzj dcshzj approved these changes

@zhoukerrr zhoukerrr zhoukerrr approved these changes

@HCY123902 HCY123902 HCY123902 approved these changes

Assignees
No one assigned
Labels
s.ToMerge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing contribution bar for merged groups after refresh
6 participants
@MarcusTXK @ckcherry23 @vvidday @dcshzj @zhoukerrr @HCY123902