Skip to content

fix(markDirectoryAsDecisionTypeRec): Properly handle clearing state when removing irrelevant decisions #3015

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(markDirectoryAsDecisionTypeRec): Properly handle clearing state when removing irrelevant decisions #3015

wants to merge 1 commit into from

Conversation

Vaibhavsahu2810
Copy link
Contributor

@Vaibhavsahu2810 Vaibhavsahu2810 commented Mar 18, 2025 •
edited
Loading

Description

Fixes #2215

Problem

When a file with an identified license is marked as irrelevant and then the irrelevant decision is removed, the following issues occur:

  • The "Clearing Status" incorrectly stays green
  • The "Cleared / Open / Total" counters remain at 1 / 1 / 1
  • The file appears as cleared when it's actually not cleared

This happens because when removing a decision type, the system doesn't properly reset the file's status to its original "no decision" state.

Solution

Modified the markDirectoryAsDecisionTypeRec method in the ClearingDao class to properly handle the removal of decision types. When a decision type is removed, the method now:

  • Captures the affected items before deleting the decisions
  • Deletes the existing decisions as before
  • Explicitly inserts a decision with the "no license known" decision type

Changes

  • Modified the markDirectoryAsDecisionTypeRec method to insert a null decision ("no license known") for each affected item after removing irrelevant decisions
  • Added a new helper method insertNullDecision that correctly resets the clearing status by inserting a decision with the "no license known" decision type by using the existing ClearingDecisionProcessor instance to access the NO_LICENSE_KNOWN_DECISION_TYPE constant

Proof of Work

Before changes

bef1.mov

After changes

aft1.mov

How to test

  • Selected a cleared file with identified license
  • Marked file as irrelevant
  • Removed marked irrelevant decisions via Actions [Edit]
  • Verified that "Edited Results" now show the correct state
  • Verified that "Clearing Status" correctly shows as uncleared

Kaushl2208
Kaushl2208 requested changes Apr 15, 2025
View reviewed changes
Copy link
Member

@Kaushl2208 Kaushl2208 left a comment

Choose a reason for hiding this comment

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

Squash multiple commits, Follow commit guidelines.

src/lib/php/Dao/ClearingDao.php Outdated
Comment on lines 844 to 846
$sql = $uploadTreeProxy->asCTE() . ' SELECT uploadtree_pk FROM UploadTreeView;';
$itemRows = $this->dbManager->getRows($sql, $params, __METHOD__ . ".getItemsForReset");
$uploadTreeTableName = $itemTreeBounds->getUploadTreeTableName();
Copy link
Member

Choose a reason for hiding this comment

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

DRY.
You can shift this logic outside conditional statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/lib/php/Dao/ClearingDao.php Outdated
Comment on lines 825 to 839
$options = array(
UploadTreeProxy::OPT_SKIP_THESE => 'noLicense',
UploadTreeProxy::OPT_ITEM_FILTER => ' AND (lft BETWEEN $1 AND $2)',
UploadTreeProxy::OPT_GROUP_ID => '$' . $a
);
$uploadTreeProxy = new UploadTreeProxy($itemTreeBounds->getUploadId(), $options, $itemTreeBounds->getUploadTreeTableName());
if (!$removeDecision) {
$sql = $uploadTreeProxy->asCTE() .
' SELECT uploadtree_pk FROM UploadTreeView;';
$itemRows = $this->dbManager->getRows($sql, $params,
__METHOD__ . ".getRevelantItems");
$sql = $uploadTreeProxy->asCTE() . ' SELECT uploadtree_pk FROM UploadTreeView;';
$itemRows = $this->dbManager->getRows($sql, $params, __METHOD__ . ".getRevelantItems");
$uploadTreeTableName = $itemTreeBounds->getUploadTreeTableName();
/** @var ClearingDecisionProcessor $clearingDecisionEventProcessor */
$clearingDecisionEventProcessor = $GLOBALS['container']->get(
'businessrules.clearing_decision_processor');
$clearingDecisionEventProcessor = $GLOBALS['container']->get('businessrules.clearing_decision_processor');
foreach ($itemRows as $itemRow) {
$itemBounds = $this->uploadDao->getItemTreeBounds(
$itemRow['uploadtree_pk'], $uploadTreeTableName);
$clearingDecisionEventProcessor->makeDecisionFromLastEvents(
$itemBounds, $userId, $groupId, $decisionMark, DecisionScopes::ITEM);
$itemBounds = $this->uploadDao->getItemTreeBounds($itemRow['uploadtree_pk'], $uploadTreeTableName);
$clearingDecisionEventProcessor->makeDecisionFromLastEvents($itemBounds, $userId, $groupId, $decisionMark, DecisionScopes::ITEM);
Copy link
Member

Choose a reason for hiding this comment

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

Irrelevant beautification. Makes hard to review

Copy link
Contributor Author

@Vaibhavsahu2810 Vaibhavsahu2810 Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for pointing that out — I forgot to revert the automatic formatting done by the PHP Sniffer & Beautifier extension in between the actual changes. I’ll clean that up and push the corrected diff.

src/lib/php/Dao/ClearingDao.php Outdated
$this->dbManager->getSingleRow($delCdEventSql, array($clearingDecisions), __METHOD__ . ".deleteCdEvent");

/** @var ClearingDecisionProcessor $clearingDecisionEventProcessor */
$clearingDecisionEventProcessor = $GLOBALS['container']->get('businessrules.clearing_decision_processor');
Copy link
Member

Choose a reason for hiding this comment

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

where are we using this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used earlier , but removed now

src/lib/php/Dao/ClearingDao.php Outdated
Comment on lines 859 to 874
$clearingDecisionRows = $this->dbManager->getRows($sql, $params,
__METHOD__ . ".getRelevantDecisions");
$clearingDecisions = array_map(function($x) {
$clearingDecisionRows = $this->dbManager->getRows($sql, $params, __METHOD__ . ".getRelevantDecisions");
$clearingDecisions = array_map(function ($x) {
return $x['clearing_decision_pk'];
}, $clearingDecisionRows);
$clearingDecisions = "{" . join(",", $clearingDecisions) . "}";

$delEventSql = "DELETE FROM clearing_event WHERE clearing_event_pk IN (" .
"SELECT clearing_event_fk FROM clearing_decision_event " .
"WHERE clearing_decision_fk = ANY($1::int[]));";
$this->dbManager->getSingleRow($delEventSql, array($clearingDecisions),
__METHOD__ . ".deleteEvent");
$this->dbManager->getSingleRow($delEventSql, array($clearingDecisions), __METHOD__ . ".deleteEvent");

$delCdEventSql = "DELETE FROM clearing_decision_event WHERE " .
"clearing_decision_fk = ANY($1::int[]);";
$this->dbManager->getSingleRow($delCdEventSql, array($clearingDecisions),
__METHOD__ . ".deleteCdEvent");
$this->dbManager->getSingleRow($delCdEventSql, array($clearingDecisions), __METHOD__ . ".deleteCdEvent");
Copy link
Member

Choose a reason for hiding this comment

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

Too much beautification for my eyes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

src/lib/php/Dao/ClearingDao.php Outdated
Comment on lines 901 to 903
$clearingDecisionProcessor = $GLOBALS['container']->get('businessrules.clearing_decision_processor');

$noDecisionType = $clearingDecisionProcessor::NO_LICENSE_KNOWN_DECISION_TYPE;
Copy link
Member

Choose a reason for hiding this comment

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

Can be taken out directly from the class: ClearingDecisionProcessor

Something like: ClearingDecisionProcessor::NO_LICENSE_KNOWN_DECISION_TYPE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/lib/php/Dao/ClearingDao.php Outdated

$sql = "INSERT INTO clearing_decision
(uploadtree_fk, pfile_fk, user_fk, group_fk, decision_type, scope, date_added)
SELECT $1, pfile_fk, $2, $3, $4, " . DecisionScopes::ITEM . ", now()
Copy link
Member

Choose a reason for hiding this comment

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

DecisionScopes::ITEM can also be made a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created a variable for it

src/lib/php/Dao/ClearingDao.php Outdated
$clearingDecisionProcessor = $GLOBALS['container']->get('businessrules.clearing_decision_processor');

$noDecisionType = $clearingDecisionProcessor::NO_LICENSE_KNOWN_DECISION_TYPE;

Copy link
Member

Choose a reason for hiding this comment

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

Before inserting clearing decision, Check if the record already exist. To prevent duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually here we need to ensure a fresh "no license known" decision with the current timestamp exists after removing an irrelevant flag. Simply checking for existing records would block the insertion.
The approach i implemented - deleting old "no license known" decisions and always inserting a new one - ensures proper status resetting with an up-to-date timestamp and preventing duplication as well

@Vaibhavsahu2810 Vaibhavsahu2810 force-pushed the Vaibhavsahu2810/fix/Clearing_Status_Display branch from 13aec3b to 67a1f9a Compare April 17, 2025 20:53
@Vaibhavsahu2810
fix(markDirectoryAsDecisionTypeRec): clear status display after remov…
460344e 
…ing irrelevant decisions

changes

preventing duplication

Signed-off-by: Vaibhav <sahusv4527@gmail.com>

changes

Signed-off-by: Vaibhav <sahusv4527@gmail.com>
@Vaibhavsahu2810 Vaibhavsahu2810 force-pushed the Vaibhavsahu2810/fix/Clearing_Status_Display branch from 67a1f9a to 460344e Compare April 17, 2025 21:07
@Vaibhavsahu2810
Copy link
Contributor Author

Hey @Kaushl2208,
I've made the requested changes.
Let me know if there's anything else you'd like me to update or improve!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kaushl2208 Kaushl2208 Kaushl2208 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
needs code review needs test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

After remove irrelevant decisions (via Actions [Edit]) the Edited Results are empty and clearing status stays green
3 participants
@Vaibhavsahu2810 @Kaushl2208 @shaheemazmalmmd