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

Add updated_at field to task #590

Merged
merged 4 commits into from
Jun 21, 2022
Merged

Add updated_at field to task #590

merged 4 commits into from
Jun 21, 2022

Conversation

anarute
Copy link
Member

@anarute anarute commented Jun 9, 2022

We need to know when users update their tasks, specially the ones related to their vacation plans.

This patch adds a column called updated_at to the task table as well as their migration scripts.

It also starts populating the field whenever a task is created or updated.

@anarute anarute requested a review from jaragunde June 9, 2022 17:03
Copy link
Member

@jaragunde jaragunde left a comment

Choose a reason for hiding this comment

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

There is another function in the DAO that modifies the tasks data, PostgreSQLTaskDAO::update(), but I'm not sure when/if we use it. We should either:

  1. Set the updated_at field in that function too.
  2. Verify whether it's actually unused and delete it!

I've been reviewing the usage of every DAO function while working on #513 to prevent rewriting unused code, so you may do only 1 this time and we leave 2 for later if you are in a hurry.

model/vo/TaskVO.php Outdated Show resolved Hide resolved
* IMPORTANT: they must be ordered for their proper execution.
*/
$sqlFiles = array();
$sqlFiles[] = SQLPATH . "add-updated-at-to-task.sql";
Copy link
Member

Choose a reason for hiding this comment

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

I missed this before: I think we should also include bump-db-version-2-22.sql here. I wasn't planning to do any new "stable" releases so every change in the DB should bump its version so we can easily track where the DB is with regard to the code.

Copy link
Member

Choose a reason for hiding this comment

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

Related: #511.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's all good, could you do just this small modification?

@anarute
Copy link
Member Author

anarute commented Jun 10, 2022

There is another function in the DAO that modifies the tasks data, PostgreSQLTaskDAO::update(), but I'm not sure when/if we use it. We should either:

  1. Set the updated_at field in that function too.
  2. Verify whether it's actually unused and delete it!

I've been reviewing the usage of every DAO function while working on #513 to prevent rewriting unused code, so you may do only 1 this time and we leave 2 for later if you are in a hurry.

I couldn't find anywhere this method was being used so I removed it.

I'm trying to port the task DAO to use PDO, but it's been a bit tricky 😬

@jaragunde
Copy link
Member

I'm trying to port the task DAO to use PDO, but it's been a bit tricky 😬

You may want to try to migrate a small DAO first to "practice", we still have some!

It will be used mainly to track when users update their vacation plans
Whenever a task is created or updated, save the current moment
in the updated_at column.
Copy link
Member

@jaragunde jaragunde left a comment

Choose a reason for hiding this comment

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

Thanks!

@anarute anarute merged commit 96c3992 into main Jun 21, 2022
@anarute anarute deleted the add-updated-at-to-task branch June 21, 2022 14:10
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.

2 participants