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

Move consistency checkers to a class (2) #192

Merged
merged 1 commit into from
May 31, 2021
Merged

Move consistency checkers to a class (2) #192

merged 1 commit into from
May 31, 2021

Conversation

fonsecadeline
Copy link
Contributor

This PR replaces #181

@fonsecadeline
Copy link
Contributor Author

fonsecadeline commented Apr 21, 2021 •
edited
Loading

Needs :

@fonsecadeline fonsecadeline mentioned this pull request Apr 21, 2021
Closed
@fonsecadeline fonsecadeline mentioned this pull request Apr 21, 2021
Merged
@senhalil
Copy link
Contributor

@fonsecadeline is it me or a test that needs to be introduced in PR #193 stayed on the PR #192 side after the split?

@fonsecadeline
Copy link
Contributor Author

@senhalil I think I had added no test, but I will on #193

@senhalil
Copy link
Contributor

I don't remember but I think I saw modified tests and thought that this PR adds tests by mistake (instead of just refacto) but now I checked again, I don't see any modified tests. Maybe I check the PR during rebase or something.

@fonsecadeline
Copy link
Contributor Author

fonsecadeline commented Apr 26, 2021 •
edited
Loading

I don't remember but I think I saw modified tests and thought that this PR adds tests by mistake (instead of just refacto) but now I checked again, I don't see any modified tests. Maybe I check the PR during rebase or something.

For my part I thought I had added some tests, just because max_split tests where failing but in fact it is just that refactoring implies checks that we did not check before : https://gitlab.com/mapotempo/optimizer-api/-/issues/767

If I sum up, we did not use to check that services/shipments' visits_number was == 1 if no configuration was provided. Now, when no configuration is provided we assume no schedule is provided (which is correct) and reject if services.visits_number > 1.

This PR is pending because we need to know what to do about those failing tests, but it will be needed for muti-tour.

@senhalil
Copy link
Contributor

Thanks for the sum up. I forgot that this was that PR ! To not to break mt/dev, we can skip the offending tests, we already have a ticket that mentions these tests, we want to treat this ticket as soon as possible so that way we wouldn't creak out CI for everyone.

Do we know why the "move" of checkers leads to broken tests in this PR? I am asking because we add the check visits_number > 1 in 193 (so it shouldn't break here), right ? (maybe I am confused of the order of the PRs and which one is based on which 🤔 )

@fonsecadeline
Copy link
Contributor Author

Thanks for the sum up. I forgot that this was that PR ! To not to break mt/dev, we can skip the offending tests, we already have a ticket that mentions these tests, we want to treat this ticket as soon as possible so that way we wouldn't creak out CI for everyone.

Do we know why the "move" of checkers leads to broken tests in this PR? I am asking because we add the check visits_number > 1 in 193 (so it shouldn't break here), right ? (maybe I am confused of the order of the PRs and which one is based on which thinking )

You can see more details on the GitLab ticket but it is because before refacto we did not reach this check if we had no configuration in VRP (when coming from max_split there is apparently no configuration so it was okay). But now no configuration implies no schedule so the check fails).

@senhalil
Copy link
Contributor

You can see more details on the GitLab ticket but it is because before refacto we did not reach this check if we had no configuration in VRP (when coming from max_split there is apparently no configuration so it was okay). But now no configuration implies no schedule so the check fails).

Thanks for the explanation @fonsecadeline Sorry by the way, I didn't know that it was already explained in the ticket :( Next time you can just copy/paste the ticket link saying that it is explained -> HERE 😄 It is hard to know what's discussed in the PR, what's discussed in the ticket

@fonsecadeline
Copy link
Contributor Author

You can see more details on the GitLab ticket but it is because before refacto we did not reach this check if we had no configuration in VRP (when coming from max_split there is apparently no configuration so it was okay). But now no configuration implies no schedule so the check fails).

Thanks for the explanation @fonsecadeline Sorry by the way, I didn't know that it was already explained in the ticket :( Next time you can just copy/paste the ticket link saying that it is explained -> HERE smile It is hard to know what's discussed in the PR, what's discussed in the ticket

OK ! Yes, it got a little bit messy :)

fonsecadeline
fonsecadeline commented Apr 27, 2021
View reviewed changes
models/vrp.rb Show resolved Hide resolved
models/concerns/validate_data.rb

# TODO : this should be replaced next line when max_split does not use visits_number > 1 without schedule anymore
# next if schedule && schedule[:range_indices] || mission[:visits_number].to_i <= 1
next if configuration.nil? || schedule && schedule[:range_indices] || mission[:visits_number].to_i <= 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@braktar @senhalil I changed here : for now if we have no configuration (coming from max_split in general) then we do not do this check.

PR is ready to make this check more general after we addit max split : #193

@fonsecadeline fonsecadeline mentioned this pull request Apr 27, 2021
Closed
3 tasks
@fonsecadeline fonsecadeline mentioned this pull request May 6, 2021
Merged
1 task
@fab-girard fab-girard merged commit 54be3a2 into Mapotempo:dev May 31, 2021
@fonsecadeline fonsecadeline deleted the move_consistency_checkers branch May 31, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@braktar braktar braktar approved these changes

@senhalil senhalil senhalil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fonsecadeline @senhalil @braktar @fab-girard