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.

Sign up for GitHub

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

db query bugfix #202

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from
Open

db query bugfix #202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f40b0d3
select syntax group by error bugfix change to order by
FarukMZKC Aug 2, 2023
e2633c0
incorrect datetime value bugfix
FarukMZKC Aug 2, 2023
786f756
incorrect datetime bugfix
FarukMZKC Aug 2, 2023
de86d0a
Update tasks.php
FarukMZKC Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions modules/files/index_table.php
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused by this change. 🤔

According to the comment, the decision to use GROUP BY instead of ORDER BY is deliberate, for performance reasons. But you seem to have reverted to ORDER BY instead. I wonder why? Was the previously existing query giving errors — or not finding the expected results?

I'm just really asking for curiosity's sake, I have no way to accept or reject your PR anyway! 😹

Copy link

Choose a reason for hiding this comment

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

I think the issue is related to

Incompatible change: As of MySQL 8.0.13, the deprecated ASC or DESC qualifiers for GROUP BY clauses have been removed. Queries that previously relied on GROUP BY sorting may produce results that differ from previous MySQL versions. To produce a given sort order, provide an ORDER BY clause.

I think the fix should have been.

$q2->addGroup('p.project_id');
$q2->addOrder('f.file_version_id DESC');

Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,8 @@
// Adding an Order by that is different to a group by can cause
// performance issues. It is far better to rearrange the group
// by to get the correct ordering.
$q2->addGroup('p.project_id');
$q2->addGroup('f.file_version_id DESC');
$q2->addOrder('p.project_id');
$q2->addOrder('f.file_version_id DESC');


$q3 = new DBQuery;
Expand Down
4 changes: 2 additions & 2 deletions modules/tasks/tasks.php
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

As before... well caught, this is really not correct for a DATETIME.

Original file line number Diff line number Diff line change
Expand Up @@ -229,15 +229,15 @@
// patch 2.12.04 finish date required to be consider finish
$where .= (' AND task_project = prj.project_id AND ut3.user_id = ' . $user_id
. ' AND ut3.task_id = tsk.task_id '
. "AND (task_percent_complete < 100 OR task_end_date = '') "
. "AND (task_percent_complete < 100 OR task_end_date IS NULL) "
. 'AND prj.project_status <> 7 AND prj.project_status <> 4 '
. 'AND prj.project_status <> 5');
break;
case 'allunfinished':
// patch 2.12.04 finish date required to be consider finish
// patch 2.12.04 2, also show unassigned tasks
$where .= (' AND task_project = prj.project_id '
. "AND (task_percent_complete < 100 OR task_end_date = '') "
. "AND (task_percent_complete < 100 OR task_end_date IS NULL) "
. 'AND prj.project_status <> 7 AND prj.project_status <> 4 '
. 'AND prj.project_status <> 5');
break;
Expand Down
2 changes: 1 addition & 1 deletion modules/tasks/todo.php
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds quite reasonable, considering that task_start_dateis a DATETIME! Fixed on my own fork, too.

Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@
$q->addWhere('task_pinned = 1');
}
if (!$showEmptyDate) {
$q->addWhere("ta.task_start_date != '' AND ta.task_start_date != '0000-00-00 00:00:00'");
$q->addWhere("ta.task_start_date IS NOT NULL AND ta.task_start_date != '0000-00-00 00:00:00'");
}


Expand Down