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.

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

[11.0][mail_activity_done] add new module to keep activities that have been completed. #296

Merged
merged 1 commit into from
Jan 25, 2019

Conversation

JordiBForgeFlow
Copy link
Sponsor Member

This module implements the capability to keep activities that have been
completed, for future reporting, by setting them with the boolean 'Done'.

The activities that have been completed will not appear in the chatter.

Copy link

@elicoidal elicoidal left a comment

Choose a reason for hiding this comment

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

LGTM (details)

mail_activity_type_id=activity.activity_type_id.id,
)
message |= record.message_ids[0]
# ---- START OF PATCH

Choose a reason for hiding this comment

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

remove dead code

Copy link
Contributor

Choose a reason for hiding this comment

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

I think he wants to let clear for reviewers where is the patch.

@@ -0,0 +1,73 @@
//-*- coding: utf-8 -*-
//Copyright2018 Eficent <http://www.eficent.com>
//License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).

Choose a reason for hiding this comment

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

AGPL or LGPL?

Copy link
Contributor

Choose a reason for hiding this comment

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

should be LGPL

Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

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

LGTM

if not cr.fetchone():
cr.execute(
"""
ALTER TABLE mail_activity ADD COLUMN done boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the table is empty but already has the column done?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the column exists it won't enter the if clause and won't create the column, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

right 👍

@hbrunn hbrunn added this to the 11.0 milestone Aug 28, 2018
@hbrunn
Copy link
Member

hbrunn commented Aug 28, 2018

please fix lints

@MiquelRForgeFlow
Copy link
Contributor

@hbrunn lints fixed

@hbrunn hbrunn added needs tests This PR is correct, but lowers test coverage and removed needs fixing labels Aug 28, 2018
@hbrunn
Copy link
Member

hbrunn commented Aug 28, 2018

thanks, now we only need tests

@dajuayen
Copy link
Contributor

In the tree view it is possible to add a different color for the activities done that allows to distinguish them from the overdue. Ex:

<record id="mail_activity_view_tree" model="ir.ui.view">
    <field name="name">mail.activity.view.tree</field>
    <field name="model">mail.activity</field>
    <field name="inherit_id" ref="mail.mail_activity_view_tree"/>
    <field name="arch" type="xml">
        <xpath expr="//tree" position="attributes">
            <attribute name="decoration-info">(state == 'done')</attribute>
        </xpath>
        <field name="date_deadline" position="after">
            <field name="state"/>
        </field>
    </field>
</record>

The definition in the mail module of this view adds colors according to its 'state'. If a color is not added to the 'Done' state, all activities will end up with the 'Overdue' color.
And the list would look something like this:

captura de pantalla de 2018-09-16 22-08-56

@dajuayen
Copy link
Contributor

dajuayen commented Nov 2, 2018

@jbeficent @hbrunn
Would you allow me to give you a hand for the tests?

@JordiBForgeFlow
Copy link
Sponsor Member Author

@dajuayen I have added colors and other small improvements. Please feel free to contribute and add tests! :)

@dajuayen
Copy link
Contributor

dajuayen commented Nov 4, 2018

@jbeficent Sorry for the question, but how can I contribute to the PR? I have tried to make a fork of your code and when I make the fork of 'social' your module does not appear to me. If I upload a commit with new development, I'm probably going to do a new PR and I do not think that's the right procedure. What should I do is a PR to your last commit?

@JordiBForgeFlow
Copy link
Sponsor Member Author

JordiBForgeFlow commented Nov 4, 2018 via email

@dajuayen
Copy link
Contributor

dajuayen commented Nov 4, 2018 via email

@JordiBForgeFlow
Copy link
Sponsor Member Author

JordiBForgeFlow commented Nov 4, 2018 via email

@HviorForgeFlow HviorForgeFlow force-pushed the 11.0-mail_activity_done branch 6 times, most recently from f9d0f4c to 1cadd25 Compare November 9, 2018 11:17
@JordiBForgeFlow
Copy link
Sponsor Member Author

@ageficent can you squash the commits, and prepare a PR for v12?

self.assertEquals(self.act1.state, 'done')

def test_activity_user_count(self):
self.employee.activity_user_count()
Copy link
Contributor

Choose a reason for hiding this comment

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

An assert after this will make more sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Done 👍

@JordiBForgeFlow
Copy link
Sponsor Member Author

@ageficent can you attend the open comments? And consider the. Also for v12 PR?

@AdriaGForgeFlow
Copy link
Contributor

@ageficent can you attend the open comments? And consider the. Also for v12 PR?

Done 👍

Copy link
Member

@HviorForgeFlow HviorForgeFlow left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@JordiBForgeFlow JordiBForgeFlow merged commit 8eca724 into OCA:11.0 Jan 25, 2019
@MiquelRForgeFlow MiquelRForgeFlow deleted the 11.0-mail_activity_done branch January 25, 2019 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved needs tests This PR is correct, but lowers test coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants