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

Transfer mri violations to resolved tab in bulk #1776

Merged
merged 9 commits into from
Jul 7, 2016

Conversation

gluneau
Copy link
Contributor

@gluneau gluneau commented May 6, 2016

No description provided.

@gluneau gluneau added the Feature PR or issue introducing/requiring at least one new feature label May 6, 2016
@gluneau gluneau added this to the 16.1 milestone May 6, 2016
@codecov-io
Copy link

codecov-io commented May 6, 2016

Current coverage is 13.93%

Merging #1776 into 16.1-dev will decrease coverage by 0.39%

  1. 2 files (not in diff) in php/libraries were modified. more
    • Misses +7
    • Hits -7
  2. 2 files (not in diff) in ...dules/dashboard/ajax were modified. more
    • Hits -4
  3. 1 files (not in diff) in modules/dashboard were modified. more
    • Misses +1
    • Hits -1
@@           16.1-dev      #1776   diff @@
==========================================
  Files           118        117     -1   
  Lines         19670      19795   +125   
  Methods        1085       1101    +16   
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           2818       2758    -60   
- Misses        16852      17037   +185   
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by dd058dc...5f0ebdc

@xlecours
Copy link
Contributor

xlecours commented May 9, 2016

Massive Select statement!

I don't think this script need the whole 'mri violation module' query.

What I see here is a union of 3 select statements on different tables and a final where clause that filter the results from the 2 unnecessary sub-queries.
I think I would be easier to read if it was a single select on the mri_violations_log.

Also, there is a unnecessary ORDER BY at the end.

And there is hard-coded value specific to the IBIS project in the MincFileViolated column for the sub-queries.

I think that the only column you really need in the select statement are md5(...) as hash and mri_violations_log.LogID as join_id.

Final note, for my experience, if there will be more than ~100-500 inserts, it would be better if the inserts were done in bulk instead of one by one. In this case, the violations_resolved only have an auto increment primary key so it is not to hard on index creation but there is all the Database object call stack that could be done once every 1000 rows instead.

@gluneau
Copy link
Contributor Author

gluneau commented May 9, 2016

The massive select statement is there for reproducibility. It leaves less doubt to the user that the result will be the same as it is produced by the front end.

The whole query is also present so that the next developer who finds this script useful can parametrize it and use the results of the other tables.

I'll remove the IBIS specific MincFileViolated column.

To be as close to the query in modules/mri_violations/php/NDB_Menu_Filter_Form_mri_violations.class.inc I left the extra select columns in there.

My production run transferred a few hundreds without problems. This script will be run very occasionally. It should not affect the user experience noticeably.

@stellarxo stellarxo added the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Jun 10, 2016
@gluneau gluneau removed the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Jun 14, 2016
$newlyResolved = array();
$newlyResolved['hash'] = $result['hash'];
$newlyResolved['Resolved'] = 'inserted_flag';
$newlyResolved['User'] = 'lorisadmin';
Copy link
Contributor

Choose a reason for hiding this comment

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

lorisadmin or lorisuser - retrieve from config file?

@stellarxo stellarxo added Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix and removed PassedCodeReview labels Jun 23, 2016
@gluneau gluneau removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Jun 30, 2016
@driusan driusan merged commit 6d10685 into aces:16.1-dev Jul 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature PR or issue introducing/requiring at least one new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants