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

Reset date range does not update the since and until date hashes #1044

Closed
jamessspanggg opened this issue Feb 2, 2020 · 9 comments · Fixed by #1068
Closed

Reset date range does not update the since and until date hashes #1044

jamessspanggg opened this issue Feb 2, 2020 · 9 comments · Fixed by #1068

Comments

@jamessspanggg
Copy link
Contributor

jamessspanggg commented Feb 2, 2020

To reproduce: modify the link since/until date url hashes. Reload the page. Click the reset date range button. Reload again to see that the dates are not updated. link

Kapture 2020-02-03 at 0 08 19

@Tejas2805
Copy link
Contributor

It seems to be working fine on mine.

@jamessspanggg
Copy link
Contributor Author

@Tejas2805 can you try to following the steps to see if the same bug can be reproduced?

To reproduce: modify the link since/until date url hashes. Reload the page. Click the reset date range button. Reload again to see that the dates are not updated. link

@ccyccyccy ccyccyccy self-assigned this Feb 4, 2020
@ccyccyccy
Copy link
Contributor

ccyccyccy commented Feb 4, 2020

I believe this bug only happens you do not specify the since and until date in the command line to generate the report, which as a result sets the hasModifiedSinceDate and hasModifiedUntilDate to false on initial loading of the page.

I can do a PR to remove the check below, but I believe we can remove the hasModifiedSinceDate and hasModifiedUntilDate all together? I'm not too sure if they serve any purpose? @jamessspanggg
image

@0blivious
Copy link
Contributor

The code here did serve a purpose. It stores the date range and help restore the page when refresh. Pls dont remove it.

@ccyccyccy
Copy link
Contributor

Oops I mean removing the if() statements.
So instead of

if (this.hasModifiedSinceDate) {
        addHash('since', this.filterSinceDate);
}

if (this.hasModifiedUntilDate) {
        addHash('until', this.filterUntilDate);
}

We can change it to

addHash('since', this.filterSinceDate);
addHash('until', this.filterUntilDate);

@ccyccyccy
Copy link
Contributor

I believe that the hasModifiedSinceDate indicates whether a value has been specified in the command line argument to generate the report, as written in the line
hasModifiedSinceDate: window.app.isSinceDateProvided
in v_summary.js, but is currently interpreted as whether a filter is set by the user on the dashboard.

@0blivious
Copy link
Contributor

I feel we should not add the hash if there is no need to do so. Maybe you should change the reset date function to rectify the behavior.

@ccyccyccy
Copy link
Contributor

I feel we should not add the hash if there is no need to do so. Maybe you should change the reset date function to rectify the behavior.

Thanks for the suggestion. I implemented a simple fix (#1044) that would solve the issue but I feel that it's not very maintainable.

I believe that the hasModifiedSinceDate indicates whether a value has been specified in the command line argument to generate the report, as written in the line
hasModifiedSinceDate: window.app.isSinceDateProvided
in v_summary.js, but is currently interpreted as whether a filter is set by the user on the dashboard.

@jamessspanggg Can you help verify this? I'm not too certain if this is the case. The code was authored by you in #757

@jamessspanggg
Copy link
Contributor Author

I believe that the hasModifiedSinceDate indicates whether a value has been specified in the command line argument to generate the report,

@0blivious is correct. The purpose of hasModifiedSinceDate and hasModifiedUntilDate was to indicate whether the dates are specified in the CLI. This is because prof wants a way to always display the latest since/until date, if he did not specify the since/until date in the CLI. With this, prof does not need to always send a new link whenever he updates the analysis, but students can use the same link to get the most updated since/until date.

fzdy1914 pushed a commit that referenced this issue Mar 2, 2020
Resetting the date range would occasionally not change the date 
hashes in the URL, causing a refresh of the page to load the 
outdated filter date range. 

This bug occurs when until or since date is not specified on
generating the report.

Let's make sure that the date hashes gets updated all the time.
Tejas2805 added a commit to Tejas2805/RepoSense that referenced this issue Mar 6, 2020
* 'master' of https://github.com/reposense/RepoSense:
  [reposense#1047] v_summary: simplify toDisplay user logic (reposense#1051)
  [reposense#658] Modify checkstyle configuration (reposense#1094)
  Add commit message length configuration border (reposense#1048)
  [reposense#1061] build.gradle: remove unused dependency (reposense#1095)
  [reposense#1044] Update date hashes on reset date range (reposense#1068)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants