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 all time stats option filter #6050

Merged
merged 2 commits into from
Oct 13, 2020

Conversation

cesswairimu
Copy link
Collaborator

@cesswairimu cesswairimu commented Jul 23, 2019

Fixes #5904

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

@cesswairimu cesswairimu force-pushed the add-all-time-stats-option-filter branch 3 times, most recently from c5da0b9 to 23d1099 Compare July 24, 2019 18:14
@cesswairimu
Copy link
Collaborator Author

@skilfullycurled I think the data error still persists tried a query from Jan2013 - Dec 2013...same error #5490. So I have set it as from 2014 for the all time option for now until we can solve the data issue. What do you think?

@jywarren
Copy link
Member

jywarren commented Aug 3, 2019

Hi @cesswairimu just marking this as in-progress until it's ready. thanks!

@skilfullycurled
Copy link
Contributor

skilfullycurled commented Aug 7, 2019

Sorry for the delay @cesswairimu, I was traveling. I think what you did (limiting to 2014) is perfect for what I hope will be a short time being. The good news is that I believe I've narrowed down the day of the problem and I have a theory as to it's cause.

Short story: I believe one of the original offending dates (4/25/2013) is still causing trouble and although the post-fix test which ended in 4/25/2013 passed, this was only because the "start date" is inclusive but the "end date" is exclusive.

Proposed Solution: Assuming the problem is the same, the solution is the fix @jywarren found in #5490. There must be remaining NodeTag record(s) which point to a non-existant tid(s) and need to be removed.

My reasoning is below, but you might as well go ahead and try the solution as @jywarren outlined in #5490 and it if doesn't work you can take a look at how I arrived there and see if there is a flaw in my logic.

||
||
||
\/

Reasoning: In #5490, @cesswairimu had narrowed down the problem date to 4/25/13. @jywarren successfully fixed the issue by removing NodeTag records pointing at non-existent tids afterwhich the test case from 1/1/12 - 4/25/13 worked. Therefore, I figured that the problem must be after that date. To find the next offending date, I did a month by month query from 4/26/2013 through 12/31/2013 yet everything worked as expected.

Then I did a series of queries with successively decreasing time windows starting at 4/25/2013 through 4/30/2013 by increasing the start date by one day each query (4/25/2013 - 4/30/2013, 4/26/2013 - 4/30/2013...). The only query that did not work was the first one, 4/25/2013 - 4/30/2013).

Where it got strange was when I performed a day by day query starting at 4/25/2013 through 4/30/2013 (4/25/2013 - 4/25/2013, 4/26/2013 - 4/26/2013...) and in this case, 4/25/2013 did work.

At this point, I suspected the issue might be one of start/end date inclusion/exclusion, so I repeated the day by day test but this time under the assumption that the start date was inclusive but the end date was exclusive (e.g., 4/25/13 - 4/26/13). Once again, the date with 4/25/13 returned to causing a failure. This would also explain why that 4/25/13-4/25/13 works because it doesn't return anything.

@cesswairimu
Copy link
Collaborator Author

Great investigation @skilfullycurled 💯 , I believe your proposed solution will work. I guess we shall wait for Jeff to come back and so that he can remove the fault record(s). I could write a rake task that deletes the nodes with no tids..but I am not sure if its necessary will have to wait for Jeff on that too. Thanks a lot 👍

@jywarren
Copy link
Member

Hi, all, just catching up here, thanks! So, is there a target date I should look for a tid-less NodeTag at? Could I simply look for ALL NodeTags with no tid, or no matching Tag, and delete them? I would hope there aren't many.

@skilfullycurled
Copy link
Contributor

So, is there a target date I should look for a tid-less NodeTag at?

@jywarren, based upon my investigation (see logic above) I believe the only day that should have one is 4/25/13. If it was not very time/computationally expensive, you could look from 4/25/13 (inclusive) to the end of the year (inclusive), but I believe the result of my analysis leaves only 4/25/13 as the offending time period. Since I've downloaded every yearly set of data, I think running the inquiry on the entire database would be overkill but it's the only way to be sure. I suppose we could split the difference and only look from the beginning until the point at which the tag setup changed in the database but I don't know when that was. Or, perhaps until Plots2 was deployed.

Could I simply look for ALL NodeTags with no tid, or no matching Tag, and delete them?

I think this was your strategy last time. You can see what you past @jywarren did here.

As an aside: I believe the reason why we missed this date was due to confusion over the end date being inclusive when it was exclusive. Should we be making all dates inclusive? Or perhaps stating on the page that the start date is inclusive but the end date is exclusive? Not sure how hard the change is, but my expectation would be that when I ask for a collection of data, it will return that data through the specified end date.

@cesswairimu cesswairimu force-pushed the add-all-time-stats-option-filter branch from 23d1099 to 708ddd8 Compare September 2, 2019 06:34
@plotsbot
Copy link
Collaborator

plotsbot commented Sep 2, 2019

1 Warning
⚠️ There was an error with Danger bot’s Junit parsing: No JUnit file was found at output.xml
3 Messages
📖 @cesswairimu Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progress’, please include ‘[WIP]’ in the title.
📖 #
Screenshots 📸 (click to expand)

6050-test_viewing_question_post.png

6050-test_signup_modal.png

6050-test_wiki.png

6050-test_tag_page.png

6050-test_searching_an_item_from_the_homepage.png

6050-test_blog_page_with_location_modal.png

6050-test_login.png

6050-test_wiki_page_with_inline_grids.png

6050-test_questions.png

6050-test_methods.png

6050-test_tag_by_author_page.png

6050-test_viewing_the_dropdown_menu.png

6050-test_simple-data-grapher_powertag.png

6050-test_viewing_the_settings_page.png

6050-test_login_modal.png

6050-test_comments.png

6050-test_stats.png

6050-test_tags.png

6050-test_wiki_revisions.png

6050-test_people.png

6050-test_front.png

6050-test_signup.png

6050-test_questions_shadow.png

6050-test_blog.png

6050-test_question_page.png

6050-test_front_page_with_navbar_search_autocomplete.png

6050-test_viewing_the_dashboard.png

Learn about automated screenshots

Generated by 🚫 Danger

@jywarren jywarren changed the base branch from master to main June 30, 2020 18:00
@stale
Copy link

stale bot commented Oct 7, 2020

Hi 😄, this issue has been automatically marked as stale because it has not had recent activity. Don't worry you can continue to work on this and ask @publiclab/reviewers to add "work in progress" label 🎉 . Otherwise, it will be closed if no further activity occurs in 5 days -- but you can always re-open it if you like! 💯 Thank you for your contributions 🙌 🎈.

@stale stale bot added the stale label Oct 7, 2020
@jywarren jywarren closed this Oct 13, 2020
@jywarren jywarren reopened this Oct 13, 2020
@stale stale bot removed the stale label Oct 13, 2020
@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@1a619cb). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6050   +/-   ##
=======================================
  Coverage        ?   81.76%           
=======================================
  Files           ?      101           
  Lines           ?     5901           
  Branches        ?        0           
=======================================
  Hits            ?     4825           
  Misses          ?     1076           
  Partials        ?        0           

@jywarren jywarren merged commit 6c51f2e into publiclab:main Oct 13, 2020
@jywarren
Copy link
Member

Finally merging this, thank you @cesswairimu !!!!

@ebarry
Copy link
Member

ebarry commented Oct 13, 2020

This is super exciting, thanks @cesswairimu @jywarren @skilfullycurled !!!!

piyushswain pushed a commit to piyushswain/plots2 that referenced this pull request Oct 22, 2020
* add all time stats filter

* start tracking from 2014
manchere pushed a commit to manchere/plots2 that referenced this pull request Feb 13, 2021
* add all time stats filter

* start tracking from 2014
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this pull request Mar 2, 2021
* add all time stats filter

* start tracking from 2014
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
* add all time stats filter

* start tracking from 2014
ampwang pushed a commit to ampwang/plots2 that referenced this pull request Oct 26, 2021
* add all time stats filter

* start tracking from 2014
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
* add all time stats filter

* start tracking from 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add 'all time' option for stats
5 participants