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

[tools] CouchDB Importer Integrity not removing inactive candidates #2801

Merged
merged 5 commits into from
Aug 8, 2017

Conversation

gluneau
Copy link
Contributor

@gluneau gluneau commented May 12, 2017

  • Delete documents if candidate is inactive
  • Checking if sql result array is not empty before deleting

@gluneau gluneau added the Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) label May 12, 2017
@gluneau gluneau added this to the 17.0 milestone May 12, 2017
@gluneau gluneau requested review from driusan and xlecours May 12, 2017 14:07
@@ -80,31 +81,40 @@ function run()
"VL" => $vl
)
);
$inactiveCandID = $this->SQLDB->pselectRow("SELECT * FROM candidate c WHERE c.Active='N' AND c.PSCID=:PID", array("PID" => $pscid));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see why this extra query is required over just adding Active='Y' (for both the session and candidate) to the query above just above it (that isn't showing up in the diff.)

@driusan driusan changed the base branch from 17.0-dev to 17.1-dev May 16, 2017 18:32
@gluneau gluneau force-pushed the integrityNotRemoveingInactiveCandID branch from 18c95e4 to b9a635b Compare May 16, 2017 19:44
@gluneau
Copy link
Contributor Author

gluneau commented May 19, 2017

@jstirling91 Please review and test as suggested by @driusan, Thank you.

@driusan driusan modified the milestones: 17.1, 17.0 May 23, 2017
if ($numActive[0]['count'] == '0') {
print "PSCID $pscid VL $vl is cancelled and has no active"

if (!array_key_exists('count', $numActive[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gluneau why do we need this !array_key_exists() ???

Copy link
Collaborator

Choose a reason for hiding this comment

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

to avoid printing stuff to the log. just answered myself

@ridz1208 ridz1208 self-requested a review May 27, 2017 22:08
ridz1208
ridz1208 previously approved these changes May 27, 2017
Copy link
Collaborator

@ridz1208 ridz1208 left a comment

Choose a reason for hiding this comment

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

@jstirling91 you should definitely re-review. LGTM

@driusan driusan added this to Needs Testing in Code review 18.1 May 31, 2017
@gluneau
Copy link
Contributor Author

gluneau commented Jun 2, 2017

@jstirling91 Let me know when you can review and test.

@gluneau
Copy link
Contributor Author

gluneau commented Jun 15, 2017

@driusan @jstirling91 Can we get this tested?

@gluneau
Copy link
Contributor Author

gluneau commented Jun 28, 2017

@driusan @jstirling91 Please test.

driusan
driusan previously approved these changes Jul 10, 2017
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

Code looks good (pending it being tested..)

$this->CouchDB->deleteDoc($row['id']);
}

if (!empty($sqlDB) && $sqlDB['Active'] != 'Y') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be an 'else if' statement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that going to affect the functionality or just make it cleaner?

(I'm not sure what happens in our code if you try and delete an already deleted CouchDB doc.)

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 trying to handle the case of the last 'else' statement. Currently it'll still print "Nothing wrong with $row[id]!\n" even if it hit the first if case above.

@gluneau gluneau moved this from Needs Testing to Author Needs To Incorporate Changes in Code review 18.1 Jul 11, 2017
@driusan
Copy link
Collaborator

driusan commented Jul 25, 2017

@Jkat @gluneau Can we get an update on this?

$this->CouchDB->deleteDoc($row['id']);
}

if (!empty($sqlDB) && $sqlDB['Active'] != 'Y') {
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 trying to handle the case of the last 'else' statement. Currently it'll still print "Nothing wrong with $row[id]!\n" even if it hit the first if case above.

@Jkat
Copy link
Contributor

Jkat commented Jul 25, 2017

Okay this github review system can be confusing

…eau/Loris into integrityNotRemoveingInactiveCandID
@gluneau gluneau dismissed stale reviews from driusan and ridz1208 via 814e303 July 31, 2017 15:42
@gluneau
Copy link
Contributor Author

gluneau commented Jul 31, 2017

@Jkat Please take a look at the change

@samirdas samirdas added the Critical to release PR or issue is key for the release to which it has been assigned label Jul 31, 2017
@gluneau gluneau moved this from Author Needs To Incorporate Changes to Needs Testing in Code review 18.1 Jul 31, 2017
@Jkat Jkat added the Passed Manual Tests PR has undergone proper testing by at least one peer label Aug 8, 2017
@ridz1208 ridz1208 moved this from Needs Testing to Needs Senior Developer Review And Senior Developer Should Merge If Everything Is Fine So There's No Further States in Code review 18.1 Aug 8, 2017
@driusan driusan merged commit b161713 into aces:17.1-dev Aug 8, 2017
@driusan driusan removed this from Needs Senior Developer Review And Senior Developer Should Merge If Everything Is Fine So There's No Further States in Code review 18.1 Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Critical to release PR or issue is key for the release to which it has been assigned Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants