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

[9.0][website_canonical_url] Migrate. #299

Merged
merged 6 commits into from
Jan 4, 2017
Merged

[9.0][website_canonical_url] Migrate. #299

merged 6 commits into from
Jan 4, 2017

Conversation

yajo
Copy link
Member

@yajo yajo commented Dec 19, 2016

Did almost nothing from 8.0 branch. Just a squash on Transifex's commits, and most basic changes.

@Tecnativa

@yajo yajo added this to the 9.0 milestone Dec 19, 2016
@pedrobaeza pedrobaeza mentioned this pull request Dec 19, 2016
Closed
34 tasks
@yajo yajo force-pushed the 9.0-website_canonical_url-mig branch from 9639e84 to 10d63f4 Compare December 19, 2016 12:53
@pedrobaeza
Copy link
Member

What you can do is to propose this to master.

@yajo
Copy link
Member Author

yajo commented Dec 19, 2016

I'd love to, but I think I'd need authorisation from @tremlin and @rami-wafaie, because that would imply a license change.

@pedrobaeza
Copy link
Member

pedrobaeza commented Dec 19, 2016 •
edited
Loading

OK, let's see what they say, but we can also rewrite the code from scratch (and we'll probably need it as v10 controller handling is different - even there are already included some things I think - like the pager).

@yajo yajo force-pushed the 9.0-website_canonical_url-mig branch 2 times, most recently from d038f15 to 42da245 Compare December 21, 2016 09:19
@pedrobaeza pedrobaeza force-pushed the 9.0-website_canonical_url-mig branch from 42da245 to 87d6579 Compare December 24, 2016 03:24
@pedrobaeza
Copy link
Member

@yajo, tests are not correct.

@yajo yajo force-pushed the 9.0-website_canonical_url-mig branch from 87d6579 to 8a79a3e Compare December 27, 2016 09:10
@yajo
Copy link
Member Author

yajo commented Dec 27, 2016

OCA/server-tools#666 should have fixed it, I rebased, let's see.

@yajo yajo force-pushed the 9.0-website_canonical_url-mig branch 3 times, most recently from b13231e to 6e14227 Compare December 27, 2016 13:57
@yajo yajo force-pushed the 9.0-website_canonical_url-mig branch 2 times, most recently from 9720916 to dd0fa04 Compare December 28, 2016 08:18
@yajo
Copy link
Member Author

yajo commented Dec 28, 2016

Finally tests are working 😅

However Travis went ❌ because of some weird thing... Only OCB build fails with website_portal_contact tour, with no traceback... It makes no sense, Odoo build works!

I think I need some help with that...

@pedrobaeza
Copy link
Member

I have no clue either. Have you checked commit history to see if there's an specific commit for that module in OCB that it's not in Odoo?

@yajo
Copy link
Member Author

yajo commented Dec 28, 2016

But it is an OCA addon.

@pedrobaeza
Copy link
Member

Uhm, and it's failing also on main branch?

@yajo
Copy link
Member Author

yajo commented Dec 28, 2016

Seems like not.

@pedrobaeza
Copy link
Member

Yeah, I see that it's failing in both Odoo and OCB. First step should be to fix the main branch.

@yajo
Copy link
Member Author

yajo commented Dec 28, 2016

There was a problem with Sass indentation and encoding I fixed, please check again now, only OCB fails.

@yajo yajo force-pushed the 9.0-website_canonical_url-mig branch from dd0fa04 to a7a5dd2 Compare December 28, 2016 12:53
@yajo
Copy link
Member Author

yajo commented Dec 28, 2016

Yay! #306 made Travis 💚

@pedrobaeza
Copy link
Member

Please squash a bit commits from 8.0

Thomas Rehn and others added 5 commits December 28, 2016 14:32
@yajo
first release of website_canonical_url
436950f 
add a proper description

correct import according to review
@yajo
FIX the review remarks
477f2a2 
bug fixes + add languages

clean the code

fix README file
@yajo
[FIX] remove empty section from README.rst
bdbc816 
[IMP] add pagination support
@oca-transbot @yajo
OCA Transbot updated translations from Transifex
36049fb
@yajo
[8.0][IMP][website_canonical_url] Safer, prefer root, relative paths (O…
87ea7da 
…CA#260)

[IMP] website_canoncial_url: Several improvements

* Use safe method to retrieve lang.

This avoids a possible `KeyError`.

* Use relative paths.

Makes this work fine behind an HTTPS proxy.

* Prefer `/` over `/page/homepage`.

The root path canonicalized is more SEO and user friendly.
@yajo yajo force-pushed the 9.0-website_canonical_url-mig branch from a7a5dd2 to 9414839 Compare December 28, 2016 13:33
@yajo
Copy link
Member Author

yajo commented Dec 28, 2016

Done.

cubells
cubells approved these changes Jan 4, 2017
View reviewed changes
Copy link
Member

@cubells cubells left a comment

Choose a reason for hiding this comment

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

👍

LGTM

@pedrobaeza pedrobaeza force-pushed the 9.0-website_canonical_url-mig branch from 9414839 to 47ab7bd Compare January 4, 2017 10:36
@pedrobaeza pedrobaeza merged commit 703aaa2 into OCA:9.0 Jan 4, 2017
@pedrobaeza pedrobaeza deleted the 9.0-website_canonical_url-mig branch January 4, 2017 11:06
@rruebner
Copy link

rruebner commented May 2, 2017

@yajo We want to use this add-on for odoo 10. Is there a PR for odoo 10 already in progress? Otherwise we would migrate it to odoo 10.

rruebner
rruebner reviewed May 2, 2017
View reviewed changes
website_canonical_url/models/website.py
canonical_url = "/"
except: # pragma: no cover
pass
return canonical_url
Copy link

Choose a reason for hiding this comment

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

@yajo Do you think it is enough to use relative paths as canonical links? Based on the google docs you should avoid using relative urls.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not know about that, definitely a 💩, but it should be fixed.

@yajo
Copy link
Member Author

yajo commented May 3, 2017

@rruebner you can check #262 to see migration attempts for v10. Normally the first thing you have to do when you want to migrate something is to go there and ask a maintainer to add you as the migrator for X addon, and when it's done ask them to link the PR. It's or way to avoid dupe efforts.

@reinka reinka mentioned this pull request May 23, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rruebner rruebner rruebner left review comments

@pedrobaeza pedrobaeza pedrobaeza approved these changes

@cubells cubells cubells approved these changes

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
9.0
Development

Successfully merging this pull request may close these issues.
5 participants
@yajo @pedrobaeza @rruebner @cubells @oca-transbot