-
Notifications
You must be signed in to change notification settings - Fork 154
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
[#2027] Fix date range bug #2034
Conversation
@jq1836 is this ready for review? |
Yep, this PR is ready to be reviewed. |
Hi @jq1836, thanks for working on this. Adding a day's worth of time will definitely help in providing a more accurate representation of the commits within a zoom range compared to what we have now. To update the @reposense/active-reviewers-1 team, I have added some screenshots of the new behaviour of the zoom date range. In the first scenario (After), the zoom range has exactly those commits that are included within our selection. But in the second scenario (After), selecting a few commits of the day selects all of those commits plus a few more after them (which is better than no commits at all). This is not the ideal case, so please respond if you have any objections. Scenario 1: Single commit in a dayBeforeAfterScenario 2: Multiple commits in a dayBeforeAfter |
Something to note would be that the granularity of the zoom window, is currently on a day by day basis, i.e. if the region selected contains the day, even if it were a small part of the day, the zoom window would display all commits made on that day. Note that it is possible to alter the behavior of the zoom window such that the zoom selection more accurately chooses a time period for commits to display, for e.g. by the hour instead of by the day. Though such specificity provides minimal convenience to the user, apart from extreme cases with many commits within the day, with the user wanting to analyse commits made during a certain period of the day, which can still be done to a smaller extent with the current implementation, by zooming into the day and looking at the relative positions of the commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this @jq1836, I think the fix of adding a day is fine - something similar is also done when calculating the positions in the ramps (https://github.com/reposense/RepoSense/blob/master/frontend/src/components/c-ramp.vue#L158), and the behaviour of the zoom view showing all the commits on the day even though not all commits are visually within the drag selection is an existing behaviour and not caused by the changes in this PR, as you mentioned.
Just one minor comment about the commit message - it would be nice to include a brief elaboration on why the change is being made (e.g. what benefit will fixing this bug bring/ why the fix needs to be made), even though it might seem obvious.
The following links are for previewing this pull request:
|
* [#2027] Fix date range bug (#2034) Currently, users are unable to select a zoom range that includes the until date. This results in misleading data being presented to users. * [#2039] Update cypress minimum requirement to 12.15.0 (#2041) Chrome bug is causing cypress to fail to open a browser on Github Actions, causing frontend tests and CI to fail. Upgrading cypress to greater than 12.15.0 will fix this issue. Let's upgrade cypress to fix the failing CI. * [#1936] Migrate c-segment.vue to typescript (#2035) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate the rest of the JavaScript code to TypeScript code to facilitate future changes to the code. * [#1936] Migrate load-font-awesome-icons.js to typescript (#2040) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate the rest of the JavaScript code to TypeScript code to facilitate future changes to the code. * [#2045] Fix cypress zoom feature test (#2047) Currently, Cypress zoom feature tests are failing due to a recent change in behavior caused by a bug fix. With the tests failing, we are unable to detect any future regressions. Let's update the Cypress tests to test for the new intended behavior. * [#1936] Migrate random-color-gen.js to typescript (#2043) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate random-color-generator.js JavaScript code to TypeScript code to facilitate future changes to the code. * [#1936] Migrate c-segment-collection.vue to typescript (#2036) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate the rest of the JavaScript code to TypeScript code to facilitate future changes to the code. * [#1936] Migrate c-resizer.vue to typescript (#2038) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate the rest of the JavaScript code to TypeScript code to facilitate future changes to the code. * Bump zod from 3.20.6 to 3.22.3 in /frontend (#2048) Bumps [zod](https://github.com/colinhacks/zod) from 3.20.6 to 3.22.3. - [Release notes](https://github.com/colinhacks/zod/releases) - [Changelog](https://github.com/colinhacks/zod/blob/master/CHANGELOG.md) - [Commits](colinhacks/zod@v3.20.6...v3.22.3) --- updated-dependencies: - dependency-name: zod dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump @cypress/request and cypress in /frontend/cypress (#2042) Bumps [@cypress/request](https://github.com/cypress-io/request) to 3.0.1 and updates ancestor dependency [cypress](https://github.com/cypress-io/cypress). These dependencies need to be updated together. Updates `@cypress/request` from 2.88.12 to 3.0.1 - [Release notes](https://github.com/cypress-io/request/releases) - [Changelog](https://github.com/cypress-io/request/blob/master/CHANGELOG.md) - [Commits](cypress-io/request@v2.88.12...v3.0.1) Updates `cypress` from 12.17.4 to 13.3.0 - [Release notes](https://github.com/cypress-io/cypress/releases) - [Changelog](https://github.com/cypress-io/cypress/blob/develop/CHANGELOG.md) - [Commits](cypress-io/cypress@v12.17.4...v13.3.0) --- updated-dependencies: - dependency-name: "@cypress/request" dependency-type: indirect - dependency-name: cypress dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * [#1936] Migrate c-ramp.vue to typescript (#2037) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate the rest of the JavaScript code to TypeScript code to facilitate future changes to the code. * Give partial credit if annotated author is not the same as the blame author * [#2054] Fix zoom view bug (#2055) Currently, when granularity is set to day or week, clicking on a ramp will open up a zoom view where commit messages are not being displayed and sorting by insertions does not result in any sorting. Let's fix the unintended behaviour of the zoom view. * [#1936] Migrate repo-sorter.js to typescript (#2052) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate repo-sorter.js to TypeScript code to facilitate future changes to the code. * [#1936] Migrate safari_date.js to typescript (#2053) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate safari_date.js to TypeScript code to facilitate future changes to the code. * Remove frontend JS lint (#2063) Currently, frontend linter is failing due to lint scripts checking javascript files, the last of which has been removed in PR #2053. Lets update the lint command to exclude javascript files front the check. * use full and partial credit color * [#1929] Add dynamic positioning support for tooltips (#2056) Currently, most tooltips are shown above buttons and text. When these tooltips appear at the top of the viewport, part of the tooltips will not be rendered. Let's implement changes such that these tooltips appear below the text or button, when appearing at the top of the viewport. * Add test cases for annotated author overriding last author's credit * revert merge from master * revert merge from master 58b7002 * [#1928] Fix tooltip zIndex such that it doesn't occlude next file title (#2057) Currently, if one hovers over a tooltip of the pinned title of a file whose content is scrolled almost completely, such that the title of the next file is just below the pinned title, the tooltip is not displayed appropriately, as the title of the next file obstructs it. Let's fix this issue. * [#1726] Update GitHub-specific references in codebase and docs (#2050) There are still leftover references specific to GitHub on parts of the codebase and docs that have been generalized to accept other remote git hosts. Let's update these GitHub references to use more general language. * Trigger workflow * Revert "Merge branch 'master' into 944-analyze-authorship" This reverts commit 950c912, reversing changes made to 4bd05a7. * fix frontend test failing --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: jq1836 <95712150+jq1836@users.noreply.github.com> Co-authored-by: Chan Jun Da <65345505+chan-j-d@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pratham Jain <71367149+pratham31012002@users.noreply.github.com>
* [#2027] Fix date range bug (#2034) Currently, users are unable to select a zoom range that includes the until date. This results in misleading data being presented to users. * [#2039] Update cypress minimum requirement to 12.15.0 (#2041) Chrome bug is causing cypress to fail to open a browser on Github Actions, causing frontend tests and CI to fail. Upgrading cypress to greater than 12.15.0 will fix this issue. Let's upgrade cypress to fix the failing CI. * [#1936] Migrate c-segment.vue to typescript (#2035) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate the rest of the JavaScript code to TypeScript code to facilitate future changes to the code. * [#1936] Migrate load-font-awesome-icons.js to typescript (#2040) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate the rest of the JavaScript code to TypeScript code to facilitate future changes to the code. * [#2045] Fix cypress zoom feature test (#2047) Currently, Cypress zoom feature tests are failing due to a recent change in behavior caused by a bug fix. With the tests failing, we are unable to detect any future regressions. Let's update the Cypress tests to test for the new intended behavior. * [#1936] Migrate random-color-gen.js to typescript (#2043) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate random-color-generator.js JavaScript code to TypeScript code to facilitate future changes to the code. * [#1936] Migrate c-segment-collection.vue to typescript (#2036) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate the rest of the JavaScript code to TypeScript code to facilitate future changes to the code. * [#1936] Migrate c-resizer.vue to typescript (#2038) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate the rest of the JavaScript code to TypeScript code to facilitate future changes to the code. * Bump zod from 3.20.6 to 3.22.3 in /frontend (#2048) Bumps [zod](https://github.com/colinhacks/zod) from 3.20.6 to 3.22.3. - [Release notes](https://github.com/colinhacks/zod/releases) - [Changelog](https://github.com/colinhacks/zod/blob/master/CHANGELOG.md) - [Commits](colinhacks/zod@v3.20.6...v3.22.3) --- updated-dependencies: - dependency-name: zod dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump @cypress/request and cypress in /frontend/cypress (#2042) Bumps [@cypress/request](https://github.com/cypress-io/request) to 3.0.1 and updates ancestor dependency [cypress](https://github.com/cypress-io/cypress). These dependencies need to be updated together. Updates `@cypress/request` from 2.88.12 to 3.0.1 - [Release notes](https://github.com/cypress-io/request/releases) - [Changelog](https://github.com/cypress-io/request/blob/master/CHANGELOG.md) - [Commits](cypress-io/request@v2.88.12...v3.0.1) Updates `cypress` from 12.17.4 to 13.3.0 - [Release notes](https://github.com/cypress-io/cypress/releases) - [Changelog](https://github.com/cypress-io/cypress/blob/develop/CHANGELOG.md) - [Commits](cypress-io/cypress@v12.17.4...v13.3.0) --- updated-dependencies: - dependency-name: "@cypress/request" dependency-type: indirect - dependency-name: cypress dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * [#1936] Migrate c-ramp.vue to typescript (#2037) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate the rest of the JavaScript code to TypeScript code to facilitate future changes to the code. * Give partial credit if annotated author is not the same as the blame author * [#2054] Fix zoom view bug (#2055) Currently, when granularity is set to day or week, clicking on a ramp will open up a zoom view where commit messages are not being displayed and sorting by insertions does not result in any sorting. Let's fix the unintended behaviour of the zoom view. * [#1936] Migrate repo-sorter.js to typescript (#2052) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate repo-sorter.js to TypeScript code to facilitate future changes to the code. * [#1936] Migrate safari_date.js to typescript (#2053) Currently, there is still some JavaScript code which remains unmigrated. This allows for type unsafe code to be written, potentially resulting in unintended behavior. Let's migrate safari_date.js to TypeScript code to facilitate future changes to the code. * Remove frontend JS lint (#2063) Currently, frontend linter is failing due to lint scripts checking javascript files, the last of which has been removed in PR #2053. Lets update the lint command to exclude javascript files front the check. * use full and partial credit color * [#1929] Add dynamic positioning support for tooltips (#2056) Currently, most tooltips are shown above buttons and text. When these tooltips appear at the top of the viewport, part of the tooltips will not be rendered. Let's implement changes such that these tooltips appear below the text or button, when appearing at the top of the viewport. * Add test cases for annotated author overriding last author's credit * revert merge from master * revert merge from master 58b7002 * [#1928] Fix tooltip zIndex such that it doesn't occlude next file title (#2057) Currently, if one hovers over a tooltip of the pinned title of a file whose content is scrolled almost completely, such that the title of the next file is just below the pinned title, the tooltip is not displayed appropriately, as the title of the next file obstructs it. Let's fix this issue. * [#1726] Update GitHub-specific references in codebase and docs (#2050) There are still leftover references specific to GitHub on parts of the codebase and docs that have been generalized to accept other remote git hosts. Let's update these GitHub references to use more general language. * Trigger workflow * Revert "Merge branch 'master' into 944-analyze-authorship" This reverts commit 950c912, reversing changes made to 4bd05a7. * fix frontend test failing * switch to originality score and threshold * update originality threshold * revert frontend code changes --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: jq1836 <95712150+jq1836@users.noreply.github.com> Co-authored-by: Chan Jun Da <65345505+chan-j-d@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pratham Jain <71367149+pratham31012002@users.noreply.github.com>
Fixes #2027
Proposed commit message
Other information
Upon further investigation, the bug is caused by inaccurate logic in the
openTabZoomSubrange
method inc-summary-charts.vue
.In line 2, the const
tdiff
is calculated as the time difference in ms between thefilterUntilDate
and thefilterSinceDate
. This is inaccurate as the after creation of eachDate
object using the argumentsthis.filterUntilDate
andthis.filterSinceDate
, which results inDate
objects with the correct date, but at 00:00 (GMT+0000). This results in the date represented bythis.filterUntilDate
not being accounted for when deciding which regions of the ramp chart would be considered a certain date. As such, by adding a day worth of time in ms totdiff
, we can adjust for this.Screenshot of GUI after fix: