[PORT] porting travel_journey to v8

[houssine78]

@bwrsandman I've got problems seeing what some of your comment are referering to. anyway i've tackled two of them. This version get installed. please comment.

[houssine78]

Hi oca-clabot,

i've already signed and sent the cla

[bwrsandman]

This branch has conflicts that must be resolved, please rebase

[bwrsandman]

@gurneyalex could you fix the cla issue?

[houssine78]

@bwrsandman i've done the rebase... not sure it was the proper way to do it

[bwrsandman]

The code is mostly :+1:

You have a few commits in double, could you retry rebasing?

See with @dufresnedavid about a secondary review

[houssine78]

@bwrsandman i've just tried to rebase. I got "rebase did nothing, HEAD was already up-to-date"

[bwrsandman]

First note the commit you're at with git log , I see 7393e5e, so I am noting that down. Then do an interactive rebase with the OCA upstream commit b642ed0f04b4938145b4d2084bf476181f1d5f53

git rebase -i b642ed0f04b4938145b4d2084bf476181f1d5f53  # oca/master

Make sure the commits are correct and only there once. Fix any conflicts you have and do git rebase --continue once the conflicts have been resolved

If you mess up you can abort the rebase or reset to the commit we started with (7393e5e).

git rebase --abort
git reset --hard 7393e5e

Here's the rebase todo that seems to work:

pick 9f0fb64 [REF] moving files to the dedicated folder
fixup e67bce3 [REF] moving files to the dedicated folder
reword 19a4cd2 [REF] [WIP] migrating to new api  # Might want to take out WIP
fixup 4e31d72 [REF] [WIP] migrating to new api
fixup a89b48d [REF] [WIP] migrating to new api
fixup fec4bd8 [REF] [WIP] migrating to new api
pick 28be011 [FIX] changing field name as type is a python reserved word
fixup 717ec5f [FIX] changing field name as type is a python reserved word
fixup 6f4e92f [FIX] changing field name as type is a python reserved word
fixup 1d5e03a [FIX] changing field name as type is a python reserved word
pick eae667d [FIX] fix some missing string declaration
fixup 08a4ff2 [FIX] fix some missing string declaration
# pick 01c3935 [FIX] missing string parameter in fields declaration
pick 7393e5e [REFACT] remove commented code

# Rebase b642ed0..7393e5e onto b642ed0 (14 command(s))
#
# Commands:
# p, pick = use commit
# r, reword = use commit, but edit the commit message
# e, edit = use commit, but stop for amending
# s, squash = use commit, but meld into previous commit
# f, fixup = like "squash", but discard this commit's log message
# x, exec = run command (the rest of the line) using shell
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out

And end up with this in the history:

fb1f33683e0166888ec2c5de9c416dbbd601e840 [REF] moving files to the dedicated folder
50dfcca8bf49778c6abd1c3386db39421a05dcd8 [REF] [WIP] migrating to new api
40eb0a872306b9d9cf52764f5fbd6425daeae9f9 [FIX] changing field name as type is a python reserved word
fe558499a5f2dbe5a4330ff4f979afc5d4c17051 [FIX] fix some missing string declaration
c9d0e79de36111fbe3fda4118d4b856f63e8de54 [REFACT] remove commented code

[gurneyalex]

CLA ok

[houssine78]

@bwrsandman done! though rebase still a bit cryptic form me

thanks for you review and help and this pull request. all the best for the next move on your career!

[ghost]

@houssine78 Thanks for porting travel_journey. Watch out for your eclipse config files. Please, add these to the .gitignore. Also, I don't think that the reports in this module should be ported. These are mostly specific to one company.

[houssine78]

@dufresnedavid i've made some clean up removed eclipse file and fixing some bug. I've also got rid of the report

[houssine78]

thanks for your review by the way

[bwrsandman]

Careful about the coding style (PEP8). There are many issues: https://travis-ci.org/OCA/vertical-travel/jobs/73917407

[houssine78]

@bwrsandman @dufresnedavid

fixed most things.

travis will probably have one concern. this is in the _inv_estimate_date.

seems that concerned code wasn't doing something except checking if transportation type was registered

[bwrsandman]

./travel_journey/models/travel_journey.py:112:17: F841 local variable 'journey_class' is assigned to but never used

This needs to be fixed before we can move on. If you don't need the variable, remove it.

Also, squash the pep8 commits, it isn't significant to have multiple fix pep8 commits. I'd argue that pep8 commits which fix previous commits in the same PR shouldn't even be added, instead fixing the original commits.

[houssine78]

@bwrsandman i've removed the code, travis is green now. Sorry i'm still a kind of newbie with git's use. Does it "git commit --amend" that you're suggesting?

[ghost]

@houssine78 When I told you to get rid of the report, I was talking about the travel_journey_order_form report, not the travel_summary. The travel summary report should not be removed.

[houssine78]

@dufresnedavid I've reverted the commit. I was planning to make a report on qweb and didn't want get rid of the travel_journey_order_form. but don't have time to do it right now. Do I remove it or keep it for someone, or I when I get time, to make a pull request for that?

[ghost]

@houssine78 remove it and if someone wants to port it later, let them do it themselves.

[houssine78]

@dufresnedavid done!

[bwrsandman]

Yeah git commit --amend in cases where you need to fix up the previous commit. Git rebase with squash for others.

A commit should be logical additions or corrections to the upstream code, not a log of the changes to the changeset. Think of what's important for a dev to see in the history when looking at the changeset.

It's useful to know that you ported the models, it's useful to know you removed some code. That you regenerated translations, that you fixed a bug. That sort of thing. That's not useful to know is a fix to a commit you are proposing.

See is as proposing a bunch of logical changes. If one of the changes has an error in it, fix that change.

You don't have to be overly careful, just be mindful of the people looking at the commits in the future.

[ghost]

@houssine78 I am currently fixing an important problem with this module in v7. This module uses a registry to manage the types of travel, but it does not work as expected. Once I fix it in v7, I will submit the change to your personal branch in v8.

[houssine78]

thanks @bwrsandman I keep that in mind for the next ones ok @dufresnedavid

[ghost]

@houssine78 One last thing to fix, then this is good to be merged. In the views, options such as options='{"create_name_field": "city", "create": false, "create_edit": false}' should be removed, because these are anti-odooist in my point of view.

[houssine78]

@dufresnedavid what part of the options you want to remove? all the option parameter? even the attribute unallowing create and edit from the field?

[ghost]

@houssine78 Yes, I mean all options. It just make the module less intuitive and more restrictive.

[houssine78]

@dufresnedavid I've saw that your fix has been merged in v7 of SFL repo any chance to get it soon in this branch or it should also merged on OCA/vertical-travel?

FYI, I'm leaving tomorrow on holiday for two weeks

[ghost]

@houssine78 The change in v7 does not impact this PR. :+1:

[bwrsandman]

:+1: Thank you for taking the time to port

[houssine78]

@dufresnedavid this is what I was thinking. anyway porting other modules if I don't think porting it to new api so I'll probably take the module from OCA:7.0

[houssine78]

@bwrsandman thanks for my first PR merged on OCA ;)

[oca-clabot]

Hey @houssine78, We acknowledge that the following users have signed our Contributor License Agreement:

• @houssine78

Appreciation of efforts, OCA CLAbot