-
-
Notifications
You must be signed in to change notification settings - Fork 447
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
3729 wider spread of dates in the seed #4114
3729 wider spread of dates in the seed #4114
Conversation
4cf08c5
to
3f5fde5
Compare
@dorner sorry for force push, this is how rebase main branch works, will use merge next time |
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.
This looks good to me, but I'm not sure I understand some of @cielf 's concerns with the overall idea. Over to her for a once-over.
@isidzukuri Thanks for this -- it will make my testing life significantly better. Not a full review yet -- I'll want to 'kick the tires' -- but the one thing that leaps out is that "Requests" in the issue didn't mean Account Requests, but Requests. When looking at that, from a "the data makes sense" pov, any Requests would happen before any associated Distributiosn in real life. |
" in the issue didn't mean Account Requests, but Requests." no problem, updated |
sounds reasonable. But at first glance, I didn't see the connection between Distribution and Request in the seed |
(nods) You can make a distrbution without a request, so it's quite possible that the seed was created without any of those connections. |
As for my concerns, it was 8 months ago -- I don't really remember! There are a number of things that are dependent on date created rather than the dates that are shown. In many cases that is exactly what we want to happen, but it may be confusing to people who aren't aware of it. The inventory history is one of those things, obviously. And in some cases, having the seed like this will make things that might not be quite right more obvious (I am questioning the default sort on the donations, for example, Should it be by creation date (as it is now), or by date of donation?). So it's helpful in that regard, and makes it a lot easier to test anything around the date filtering. We might want to amend the text on the staging warning to make it explicit that everything is created anew every day, and some things depend on that -- but that would be a separate issue (a Good First Issue for someone, perhaps) |
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.
LGTM - @dorner do you have anything to add based on the last commits and my commentary?
Nope, I think we'll have to see the data to find out if there are any issues. In general I feel like the codebase relies way too heavily on randomization... both here and in testning. There is really no reason to try to randomize the number of things created if we know what a "good starting data set" is. Obviously not going to fix the world here, but if it turns out some of the data isn't helpful, that might be a good road to start going down. |
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.
Onward and upward then. This is, to my mind, definitely a step in the right direction.
@cielf Will it help if every distribution will have assigned request with date one day before? |
There are always exceptions -- some banks have specific pickup days for different partners, so you could end up with the dates being in a different order than creation. |
@isidzukuri: Your PR |
Resolves #3729
Description
Wider spread of dates on requests, distributions, purchases, donations in the seed, including some more than 1 week ago.
An initial suggestion to use date ranges from the past (days ago):
Generated dates will look like:
Type of change
How Has This Been Tested?
rails db:truncate_all
)rails db:seed
Screenshots