Skip to content

don't allow transfers of 0 hours and 0 minutes #421

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
Merged
merged 3 commits into from
Feb 18, 2019
Merged

don't allow transfers of 0 hours and 0 minutes #421

merged 3 commits into from
Feb 18, 2019

Conversation

markets
Copy link
Collaborator

@markets markets commented Sep 1, 2018 •
edited
Loading

Closes #345

I also removed ✂️ a couple of unused methods and added front-end validation:

captura de pantalla 2018-09-01 a la s 19 28 46

@markets markets mentioned this pull request Sep 1, 2018
Closed
sauloperez
sauloperez requested changes Sep 4, 2018
View reviewed changes
@sseerrggii
Copy link
Contributor

@sauloperez @enricostano Could you please review this?

@markets markets force-pushed the avoid_transfers_of_0 branch from 577c109 to af36b95 Compare October 3, 2018 19:21
@markets
Copy link
Collaborator Author

markets commented Oct 3, 2018

Hi @sseerrggii I think the missing part here is the pending decision here: #421 (comment).

Basically, the point is: when adding the DB constraint the existing wrong records fail due to this new restriction, what to do with those records? @sauloperez suggested to a create a data migration, but I'm not sure what to do 😅: delete the records, add 1 min movements, ...

PS develop rebased to keep code updated

@sseerrggii
Copy link
Contributor

We can delete the records with 0 hours and 0 minutes

@markets markets force-pushed the avoid_transfers_of_0 branch from 08711f0 to f7e47d5 Compare October 4, 2018 20:29
@markets
Copy link
Collaborator Author

markets commented Oct 4, 2018

Pushed migration with (f7e47d5):

  • Script to remove "broken" movements and transfers
  • DB constraint

@sseerrggii
Copy link
Contributor

@sauloperez ?

enricostano
enricostano approved these changes Oct 23, 2018
View reviewed changes
@markets markets force-pushed the avoid_transfers_of_0 branch from f7e47d5 to 758f3db Compare October 23, 2018 19:47
@markets
Copy link
Collaborator Author

markets commented Oct 23, 2018

Pushed i18n here 758f3db and rebased from develop.

enricostano
enricostano reviewed Oct 24, 2018
View reviewed changes
@enricostano
Copy link
Contributor

@sauloperez would you mind to finish your review here? 😘

sauloperez
sauloperez approved these changes Feb 12, 2019
View reviewed changes
Copy link
Collaborator

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@enricostano
Copy link
Contributor

Deployed to https://staging.timeoverflow.org

cc/ @sseerrggii

@sseerrggii
Copy link
Contributor

Tested! 👌

@markets can you rebase it? I have no courage 😟

@markets markets force-pushed the avoid_transfers_of_0 branch from 758f3db to c0b22d1 Compare February 14, 2019 19:37
@markets
Copy link
Collaborator Author

markets commented Feb 14, 2019 •
edited
Loading

@sseerrggii rebased with current develop :shipit:

@markets markets mentioned this pull request Feb 14, 2019
Merged
@markets markets force-pushed the avoid_transfers_of_0 branch from c0b22d1 to 50851e9 Compare February 17, 2019 13:29
@sseerrggii sseerrggii merged commit eb04977 into develop Feb 18, 2019
@enricostano enricostano deleted the avoid_transfers_of_0 branch February 18, 2019 11:03
@markets markets mentioned this pull request Apr 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enricostano enricostano enricostano approved these changes

@sseerrggii sseerrggii sseerrggii left review comments

@sauloperez sauloperez sauloperez 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
@markets @sseerrggii @enricostano @sauloperez