Skip to content

Conversation

@pabr-odoo
Copy link

Task card: https://www.odoo.com/odoo/project/3835/tasks/3985571

Adding a new doc to CRM on generating a lead distribution report from the CRM pipeline.

@robodoo
Copy link
Collaborator

robodoo commented Jun 21, 2024

Pull request status dashboard

@C3POdoo C3POdoo requested a review from a team June 21, 2024 20:56
Copy link
Contributor

@ksc-odoo ksc-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 @pabr-odoo -- just finished my Review on this PR. Really great job. Awesome attention to detail here. That said, I do have a fairly decent amount of comments/feedback/suggestions, etc. that require your attention. So, if you could give those a look, and implement the necessary adjustments, then you can tag me again for another quick look. Once we iron out those details, I'll give ya the go ahead for the next stage. Thanks! 👍

@pabr-odoo pabr-odoo force-pushed the 17.0-crm-add-lead-distribution-report-pabr branch 2 times, most recently from 1ab4bed to c021c0e Compare June 26, 2024 18:37
@pabr-odoo
Copy link
Author

Hey @ksc-odoo I implemented your suggestions. Please give it a look over when you get the chance and let me know if there's any changes that still need to be made. Thanks!

Copy link
Contributor

@ksc-odoo ksc-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 @pabr-odoo -- just gave this one another look. great job on the revisions so far. Just a handful of other suggestions/comments that require your attention. Once you implement those, feel free to tag this for Tech Review. Thanks! 👍

@pabr-odoo pabr-odoo changed the base branch from 17.0 to saas-17.2 June 26, 2024 21:58
@samueljlieber samueljlieber changed the base branch from saas-17.2 to 17.0 June 27, 2024 15:55
@pabr-odoo pabr-odoo force-pushed the 17.0-crm-add-lead-distribution-report-pabr branch 2 times, most recently from be678b0 to 50e6f03 Compare July 9, 2024 20:16
@C3POdoo C3POdoo requested a review from a team July 9, 2024 20:18
@pabr-odoo
Copy link
Author

Hey @samueljlieber tagging this new doc for tech review when you get the chance. Thanks!

Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Hi @pabr-odoo
A few outstanding items before we can continue the tech review.

  • too many screenshots, try to include only what's necessary or completely filled out forms (instead of multiple micro shots)
  • we cannot use screenshots of the internal odoo database so like filters-dropdown.png for examples is a no go.
  • there are some screenshots (and maybe instructions) that are not necessary for this report, they are supplemental to further filter for quality leads. All of the stuff around lead sourcing (does not contain personal email addresses, Source is livechat) is extra.
    • I'm mentioning it bc I don't want readers to misunderstand what is necessary vs. what is helpful/extra (does that make sense?)
  • merge conflict that has to be solved on crm.rst

@StraubCreative StraubCreative force-pushed the 17.0-crm-add-lead-distribution-report-pabr branch 2 times, most recently from 945959c to 901c0e2 Compare July 25, 2024 22:38
@pabr-odoo pabr-odoo force-pushed the 17.0-crm-add-lead-distribution-report-pabr branch 2 times, most recently from 36b7695 to 5ddb82f Compare July 26, 2024 00:09
@pabr-odoo
Copy link
Author

Hey @StraubCreative thanks again for your feedback! This should hopefully be good to go for tech review now when you get the chance.

@StraubCreative StraubCreative force-pushed the 17.0-crm-add-lead-distribution-report-pabr branch from 5ddb82f to 8d8cdf2 Compare July 26, 2024 01:21
@StraubCreative
Copy link
Contributor

8d8cdf2: fixes all line-break issues (about 9 or so in total).

@pabr-odoo let's take a look at your linter and VSCode setup to make sure those are all set up correctly bc they will help with a lot of things such as early/late line breaks 👍

Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Hi @pabr-odoo

Good push last round!

To continue dialing this in for accuracy, formatting, and being concise, I have some more feedback for you below. You should be able to batch commit a lot of it and then for the latter material we can discuss if you like.

Thank you for your continued effort on this, and please tag me again when it's ready for another review 🤙

@pabr-odoo pabr-odoo force-pushed the 17.0-crm-add-lead-distribution-report-pabr branch from 8d8cdf2 to 9350145 Compare July 26, 2024 20:01
@pabr-odoo
Copy link
Author

Thanks again @StraubCreative for all your effort in helping with this doc. I implemented the changes you suggested before and think it's looking great with this round of suggestions. Please take another look whenever you get the chance.

Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Hi @pabr-odoo

Great effort, I think this came out nicely! 👍
Approving with some optional comments for you to consider.

When you're ready to move forward, please tag @samueljlieber for tech review/merge, thanks!

@pabr-odoo pabr-odoo force-pushed the 17.0-crm-add-lead-distribution-report-pabr branch from 9350145 to c93df3d Compare July 30, 2024 22:44
@pabr-odoo pabr-odoo requested a review from samueljlieber July 30, 2024 22:45
@pabr-odoo
Copy link
Author

Hey @samueljlieber this is ready for tech review whenever you're free. Thanks so much!

Copy link
Contributor

@samueljlieber samueljlieber left a comment

Choose a reason for hiding this comment

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

Hi @pabr-odoo! Fantastic job on this new CRM doc on creating a lead distribution report 👏 This is a really great reference and I like how you covered custom filters in a way that readers should feel empowered to create their own personalized reports.

I know I have a number of fixes, but don't be discouraged! These are all minor changes for RST syntax and our doc guidelines, and some suggestions, overall this PR is looking great!

Please tag me for one more quick look once each of my suggestions are addressed, thank you!

@pabr-odoo pabr-odoo force-pushed the 17.0-crm-add-lead-distribution-report-pabr branch from 823fdc5 to a78de48 Compare July 31, 2024 21:22
@pabr-odoo
Copy link
Author

Hey @samueljlieber thanks so much for the encouragement and thorough review! I addressed all your changes, but added back the seealso admonition at the very end of the doc and kept "active-set.png" as it was used in the doc. Please let me know if I'm mistaken, or if anything else needs to be addressed. Thanks again for all your hard work!

Copy link
Contributor

@samueljlieber samueljlieber left a comment

Choose a reason for hiding this comment

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

@pabr-odoo! Nice work, your PR is looking great! Approving with two more quick fixes, thank you for your work! Please merge after addressing these two :)
.....
@robodoo delegate=pabr-odoo

@samueljlieber samueljlieber added the 5 label Aug 1, 2024
@pabr-odoo pabr-odoo force-pushed the 17.0-crm-add-lead-distribution-report-pabr branch from a78de48 to fc12622 Compare August 1, 2024 17:30
@pabr-odoo
Copy link
Author

@robodoo r+

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants