Skip to content

Conversation

@vpk-odoo
Copy link
Contributor

@vpk-odoo vpk-odoo commented Apr 1, 2021

@xpl-odoo
Copy link
Contributor

xpl-odoo commented Apr 9, 2021

You also need to add a Label to your PR on GitHub (in most cases it will be "content improvement") and a Milestone (here, v14)
I'll let you do it :)

@vpk-odoo vpk-odoo added this to the 14.0 milestone Apr 9, 2021
@vpk-odoo vpk-odoo force-pushed the 14.0-crm-leadscoring-vpk branch 2 times, most recently from 68bfbbc to 4851e8e Compare May 25, 2021 16:49
Copy link
Contributor

@xpl-odoo xpl-odoo left a comment

Choose a reason for hiding this comment

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

Good job with the changes, it's a lot better! I have been very nitpicky - don't be afraid by the number of comments :)

Note, I just learned the following:

  • the commit message title shouldn't be longer than 72 characters, here there are 85. ([IMP] CRM: updated some of the menu naming conventions as well as did a grammar pass.)
  • You should also include the Odoo task number in the commit message (the .txt file you edit on PyCharm). It can be found on the task URL. Here is the one for your task
  • And include a link to the Odoo task on GitHub (i.e. when you submit the PR message on GitHub, which uses your commit message as a basis)

@vpk-odoo vpk-odoo force-pushed the 14.0-crm-leadscoring-vpk branch 2 times, most recently from 3f03a00 to d751f2c Compare May 27, 2021 22:19
Copy link
Contributor

@xpl-odoo xpl-odoo left a comment

Choose a reason for hiding this comment

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

We are good to go! There might be a comma or two we could add (according to Grammarly), but I don't feel they are necessary. But maybe Jon will think otherwise :)

Copy link
Contributor

@jcs-odoo jcs-odoo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @vpk-odoo and for the review @xpl-odoo !

I'm afraid I may have bad news though.
I'm wondering if you checked the content of CRM with the PO or an expert?

I think this feature is already deprecated. As a matter of fact, it is even completely removed in 14.2, but this doc is about 14.0 so it is still slightly valid.

As a rule of thumb, if a feature is useful for the current version, then we should expect to see it in the settings, and not in a module to install. (this is not always true, but it's a rule of thumb :p )

I think that this old module has been replaced by the "Predictive Lead Scoring" feature, which works with "AI", based on the previous results of your opportunities. I am quite sure it also impacts Leads. However, in 14.0 lead assignation and multi-sales teams are still in this "lead scoring" module, but this is moved directly into CRM from 14.2. So the "assignation" part is still good.

(For info, there will probably be databases in 14.3 but they will have to deal with the doc about 14.0 until we also update it for 15.0)

I'm afraid this PR isn't valid as is, normally. (For real, it pains me to write this). But please double-check with the experts, they would know better than I do :)
I guess we can still merge it as it is an improvement. Just make sure that you'll cover the new feature in another PR.

It's not enough to understand the old docs and update them: we must ask PO/experts/colleagues about the apps, make sure to understand it fully, and only then rewrite the content. By starting "from scratch", you can get rid of weird structures that some docs may have, and make sure that your content is relevant for the current version and well structured.

Thank you,
Jonathan

Comment on lines 63 to 62
Evaluate & use the unassigned leads
===================================
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, about the ampersand

@jcs-odoo
Copy link
Contributor

jcs-odoo commented Jun 2, 2021

@vpk-odoo @xpl-odoo
I forgot the part about fixing the conflicts:

  • git checkout 14.0 + git pull to update your local 14.0
  • checkout your branch
  • git rebase 14.0

You'll have some conflicts to fix. Here is a short video showing how to do it:
https://drive.google.com/file/d/1ITESVzMao7IrhhStsR7vJ7IwN9W4b4q2/view

@vpk-odoo vpk-odoo force-pushed the 14.0-crm-leadscoring-vpk branch from d751f2c to 70ed79a Compare June 24, 2021 15:57
@vpk-odoo
Copy link
Contributor Author

@jcs-odoo and @xpl-odoo, pretty sure that I was able to fix the conflicts. I added a "warning" at the top of the page that this feature would be completely deprecated in V14.3.

@C3POdoo C3POdoo requested a review from a team December 9, 2021 11:26
@jcs-odoo jcs-odoo closed this Mar 22, 2022
@jcs-odoo jcs-odoo reopened this Mar 22, 2022
@robodoo
Copy link
Collaborator

robodoo commented Mar 22, 2022

Sorry, I didn't know about this PR and had to retrieve its information, you may have to re-approve it.

@robodoo
Copy link
Collaborator

robodoo commented Mar 22, 2022

@StraubCreative
Copy link
Contributor

@jcs-odoo how best to proceed here?
I'm guessing just pick back up with 14.0 rebase and then have @meng-odoo review it?

Thank you + please advise 🙏

@StraubCreative StraubCreative self-assigned this Apr 22, 2022
@jcs-odoo
Copy link
Contributor

(For info, there will probably be databases in 14.3 but they will have to deal with the doc about 14.0 until we also update it for 15.0)

Hello @StraubCreative and many thanks for taking this one :)

Technically speaking... yes, a good rebase on the latest 14.0 would be a great start.
Then @meng-odoo can directly apply some changes if needed.

I don't remember the details, but I think there were issues with versioning at the time. It's mainly because we didn't have branches for intermediary versions at the time.

Careful though, as it probably implies that the content won't be valid for versions >=14.3 .

As a rule of thumb, I would say to merge the improvement if they're ready to go (I haven't re-read the diff since my old review) and focus more on current content rather than on old features that have no future. That's why we primarily work based on master or the last frozen version.

Cheers :)

@StraubCreative StraubCreative force-pushed the 14.0-crm-leadscoring-vpk branch from 70ed79a to ddae5da Compare April 29, 2022 23:37
@StraubCreative
Copy link
Contributor

Technically speaking... yes, a good rebase on the latest 14.0 would be a great start. Then @meng-odoo can directly apply some changes if needed.

Ok, @jcs-odoo thanks for clarifying.
The branch has been rebased via ddae5da.
@meng-odoo you're up :)

I don't remember the details, but I think there were issues with versioning at the time. It's mainly because we didn't have branches for intermediary versions at the time.

Careful though, as it probably implies that the content won't be valid for versions >=14.3 .

For this part @jcs-odoo, what do you think about including a Note or similar section at the top of the doc mentioning which versions these features apply to? Would this be useful? Open to ideas...

@jcs-odoo
Copy link
Contributor

jcs-odoo commented May 5, 2022

I don't remember the details, but I think there were issues with versioning at the time. It's mainly because we didn't have branches for intermediary versions at the time.
Careful though, as it probably implies that the content won't be valid for versions >=14.3 .

For this part @jcs-odoo, what do you think about including a Note or similar section at the top of the doc mentioning which versions these features apply to? Would this be useful? Open to ideas...

No, let's avoid any mention of the version as it often ends up unmaintained. Also, that's why we have a version switcher: people read the documentation they need, according to their version of Odoo.
You simply adapt the content from the version needed and that's it :)

Technically speaking there are two ways to do so:

  • either indicate to the fw-bot to create PR up to a specific branches, and you create a different PR from that version.
  • you merge the primary branch, let GitHub create all the forward-port branches, then you checkout the branch that needs some changes, force-push the changes, then merge. (the "next" branches in the forward-port chain are updated as well after the force-push)

Cheers :)

Copy link
Contributor

@meng-odoo meng-odoo left a comment

Choose a reason for hiding this comment

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

Hi @StraubCreative, I changed the wording in this doc to be clear, concise, and instructional as we've previously discussed. Please let me know if you have any more suggestions :)

@jcs-odoo Regarding your comment from Jun 2, 2021, I'm planning to create a brand new doc on Predictive Lead Scoring in collaboration with the product expert, Jessica (ebj), to replace this one for v15 :)

Comment on lines 29 to 31
Here's an example of a scoring rule for English-speaking Canadian users. You can modify your rules
to match whatever criteria you want to score leads on, and you can add as many criteria as you would
like.
Copy link
Contributor

Choose a reason for hiding this comment

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

Edited for wording and clarification.

Suggested change
Here's an example of a scoring rule for English-speaking Canadian users. You can modify your rules
to match whatever criteria you want to score leads on, and you can add as many criteria as you would
like.
Here's an example of a scoring rule for leads from Canada that have English listed as their language. Customize your rules to fit the needs of your business, and add as many criteria as you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might have to come back to this. We should refrain from saying anything along the lines of "look at this image to understand x". Images are supplemental alongside text descriptions, not the sole focus for instruction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's delete that entire first sentence; the paragraph is fine without it.

@StraubCreative
Copy link
Contributor

@meng-odoo 5/5 change requests implemented on 7ed91f9.

Please let me know if we need anything else :)

@StraubCreative StraubCreative requested a review from meng-odoo May 12, 2022 22:34
Comment on lines 29 to 31
Here's an example of a scoring rule for English-speaking Canadian users. You can modify your rules
to match whatever criteria you want to score leads on, and you can add as many criteria as you would
like.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's delete that entire first sentence; the paragraph is fine without it.

Copy link
Contributor

@meng-odoo meng-odoo left a comment

Choose a reason for hiding this comment

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

Hi @StraubCreative! On this pass, I deleted the sentence that directly refers to an image, and fixed a couple other small things that I missed before. Thanks :)

@StraubCreative StraubCreative force-pushed the 14.0-crm-leadscoring-vpk branch from 7ed91f9 to 174c483 Compare May 17, 2022 23:56
@StraubCreative
Copy link
Contributor

@meng-odoo your latest batch of change requests from today (5/17) have been made on 174c483.

Please let me know if there's anything else before we pass off to DR :)

@StraubCreative StraubCreative requested a review from meng-odoo May 17, 2022 23:59
Copy link
Contributor

@meng-odoo meng-odoo left a comment

Choose a reason for hiding this comment

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

Hey @StraubCreative thanks for making those changes. Everything looks good to me! I'll tag Doc Review.

@meng-odoo meng-odoo requested a review from a team May 18, 2022 17:08
Copy link
Collaborator

@AntoineVDV AntoineVDV left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants