-
-
Notifications
You must be signed in to change notification settings - Fork 545
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.
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
[18.0][MIG] base_tier_validation: Migration to 18.0 #966
Conversation
…on and validate if possible.
* able to restart validation * sudo() not needed anymore
and reject can be hidden according to this computed field.
… and who asks for reviews in new fields 'done_by' and 'requested_by'.
fixup and extend tests [ADD] systray icon for pending reviews [FIX] Remove python safe_eval [ADD] base_tier_validation_formula and migration scripts [ADD] widget domain and python expression to define reviewer in tier definition [ADD] auto updating of systray icon counter [ADD] validation date field [ADD] review widget dropdown menu
…reviews' name and state correctly translated.
* using similar approach to activities has already benn addressed. * add a new point explaining review tooltip improvement possibilities.
Currently translated at 100.0% (59 of 59 strings) Translation: server-ux-12.0/server-ux-12.0-base_tier_validation Translate-URL: https://translation.odoo-community.org/projects/server-ux-12-0/server-ux-12-0-base_tier_validation/zh_CN/
notification to possible reviewers.
@xaviedoanhduy thanks for review |
9b2c2f0
to
02e627e
Compare
thank @kevinkhao, for your improvements in this PR. |
78d8236
to
4025b0a
Compare
4025b0a
to
a85f05c
Compare
@xaviedoanhduy JS should be fine now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks fine to me,
thanks for your effort to improve it
Some improvements are being forward ported to 17.0 #968 |
@kevinkhao thanks for the work. Also in this other PR for 17.0 #979 a new icon was added, could you also cherry pick it in this migration? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything in order,
thanks for the work.
Hi @kevinkhao! Could you please continue with the migration? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, please squash administrative commits.
<DropdownItem> | ||
<div | ||
t-if="state.tierReviewCounter === 0" | ||
class="o-mail-ActivityMenu-empty align-items-center opacity-50 d-flex justify-content-center" | ||
> | ||
<span>No reviews to do.</span> | ||
</div> | ||
</DropdownItem> | ||
<div | ||
class="d-flex flex-column list-group-flush" | ||
name="activityGroups" | ||
> | ||
<t | ||
t-foreach="state.tierReviewGroups" | ||
t-as="group" | ||
t-key="group_index" | ||
name="activityGroupLoop" | ||
> | ||
<div | ||
class="o-mail-ActivityGroup list-group-item list-group-item-action d-flex cursor-pointer" | ||
t-att-data-model_name="group.model" | ||
t-on-click="() => this.openReviewGroup(group)" | ||
> | ||
<img alt="Activity" t-att-src="group.icon" /> | ||
<div class="flex-grow-1 overflow-hidden"> | ||
<div | ||
class="d-flex px-2" | ||
name="activityTitle" | ||
t-out="group.name" | ||
/> | ||
<div class="d-flex"> | ||
<span | ||
t-attf-class="#{group.pending_count ? '' : 'text-muted'} py-0 px-2" | ||
> | ||
<t t-out="group.pending_count" /> Pending | ||
</span> | ||
</div> | ||
</div> | ||
</div> | ||
</t> | ||
</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested with this PR OCA/sale-workflow#3417
The UI is broken; please fix it.
Without revisions, an icon must be displayed instead of text.
V18
V17
The revision count is hidden, and the systray is broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested this. It still shows the text instead of the icon.
@kevinkhao Are you going to fix conflicts? |
Hi, I have created a this PR #992 to apply and fix the above changes. |
Sorry for delay, didn't have time to fix the additional details especially when it comes to JS Thanks @xaviedoanhduy for taking up the fixes Updated original comment to show the superseding PR |
Thanks all for the work, let's continue in the new PR then. Closed in favor of #992 |
supersedes #964
superseded by #992