-
-
Notifications
You must be signed in to change notification settings - Fork 786
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
[16.0][mig] project_deadline: Migration to 16.0 #1351
base: 16.0
Are you sure you want to change the base?
Conversation
@heliaktiv , I have reopened PR of migrating project_deadline to 16.0 and I would request you to review it |
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.
Code review: LGTM
Please squash administrative commits, as described here: https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate |
self.assertIn("date_start", res["arch"]) | ||
self.assertIn("date", res["arch"]) |
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.
question: why were these two lines changed to be the opposite of what they were before?
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.
As part of Odoo 16.0 addons project view, date_start and date fields are included in the xml whose external id is "edit_project" which is displayed as "Planned Date".
https://github.com/odoo/odoo/blob/16.0/addons/project/views/project_views.xml#L506C28-L511C119
As this is available as part of Odoo addons project module, I am removing project_project related code changes which appends date_start and date field to the form view from project_deadline module.
Also, the testcase related to it is also no longer valid for 16.0 version
I tried to squash the commit, but it resulted in merge conflict issue related to project_hr commits, so could not do it. When git rebase -i origin/16.0 is executed, it lists other project related repos like project_hr and showing merge conflicts related to hr project during squash Is there any way to get only project_deadline related commits alone, so that it can be squashed? |
I suspect your repo may be outdated. Can you try |
[FIX] Flake [ADD] Use fields_view_get [ADD] Use fields_view_get [ADD] Tests [FIX] Quick create form bug [ADD] usage description
133848f
to
d08d199
Compare
Currently translated at 80.0% (4 of 5 strings) Translation: project-14.0/project-14.0-project_deadline Translate-URL: https://translation.odoo-community.org/projects/project-14-0/project-14-0-project_deadline/es/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: project-14.0/project-14.0-project_deadline Translate-URL: https://translation.odoo-community.org/projects/project-14-0/project-14-0-project_deadline/
Currently translated at 100.0% (8 of 8 strings) Translation: project-14.0/project-14.0-project_deadline Translate-URL: https://translation.odoo-community.org/projects/project-14-0/project-14-0-project_deadline/es_AR/
Currently translated at 100.0% (8 of 8 strings) Translation: project-14.0/project-14.0-project_deadline Translate-URL: https://translation.odoo-community.org/projects/project-14-0/project-14-0-project_deadline/it/
b66b060
to
a635b8a
Compare
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.
Code review, mostly looks good to me, only a couple of suggestions.
I also think the changes to the translation files should be removed from the migration commit.
<xpath expr="//span[@t-if='record.partner_id.value']" position="after"> | ||
<t | ||
t-if="record.date.raw_value and record.date.raw_value lt (new Date())" | ||
t-if="record.date.raw_value and record.date.raw_value lt (new Date().toISOString())" | ||
t-set="red" | ||
>oe_kanban_text_red</t> |
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.
suggestion (non-blocking): Instead of comparing two strings, we could turn record.date.raw_value
into a Date object. See here for an example:
https://github.com/OCA/social/blob/16.0/mail_activity_board/views/mail_activity_view.xml#L186-L190
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.
resolved
<t t-if="record.partner_id.value"> | ||
<span style="margin-left: 10px;" t-attf-class="#{red || ''}"> | ||
<i> | ||
<field name="date" /> | ||
</i> | ||
</span> | ||
</t> | ||
<t t-else=""> | ||
<span t-attf-class="#{red || ''}"> | ||
<i> | ||
<field name="date" /> | ||
</i> | ||
</span> | ||
</t> |
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.
issue(non-blocking): This could be done more elegantly without the t-if
and t-else
, maybe with a t-att-style
?
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.
Resolved
a635b8a
to
359d681
Compare
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.
Code review, ok.
However, upon functional review, I wonder if this module is worth migrating at all. Since we don't need to add the core fields that this module would offer, all we're adding is two related fields on project.task
.
At the very least, the readme should be updated to reflect the much more limited scope of this module, but maybe the module can be dropped entirely.
<odoo> | ||
<record id="view_project_kanban" model="ir.ui.view"> | ||
<field name="name">view_project_kanban</field> | ||
<field name="model">project.project</field> | ||
<field name="inherit_id" ref="project.view_project_kanban" /> | ||
<field name="arch" type="xml"> | ||
<xpath expr="//span[@t-if='record.partner_id.value']" position="after"> | ||
<t | ||
t-if="record.date.raw_value and ((new Date(record.date.raw_value)) lt (new Date()))" | ||
t-set="red" | ||
>oe_kanban_text_red</t> | ||
<span | ||
style="margin-left: 10px;" | ||
t-attf-class="{{ record.partner_id.value ? red : ''}}" | ||
> | ||
<i> | ||
<field name="date" /> | ||
</i> | ||
</span> | ||
</xpath> | ||
</field> | ||
</record> | ||
</odoo> |
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.
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.
Though I saw this information on the kanban view , since in 14.0 date was shown seperately near Partner_id I thought it should be shown in 16.0 even we have start and end date, hence added this.
Removed some of the code as those were not necessary in 16.0 however I misinterpreted this date information behaviour and added here
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.
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.
e38671f
to
af04c35
Compare
af04c35
to
743ccbb
Compare
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.
Code review, LGTM.
My doubts about whether this module is really useful remain, but I'll leave it up to a maintainer to decide.
Migrating project_deadline to 16.0
Reopened PR: #1123