Add previous Features for each revision #2936
Conversation
| @@ -117,6 +118,13 @@ def character_sum(revisions, namespace) | |||
| .sum(:characters) || 0 | |||
| end | |||
|
|
|||
| def refs_sum_ms(revisions) | |||
There was a problem hiding this comment.
This method has the same name as the attribute, which I think will cause problems.
I also think we should avoid the shorthand "refs" and instead call the attribute something like references_added.
app/models/wiki_content/revision.rb
Outdated
| @@ -66,4 +67,10 @@ def plagiarism_report_link | |||
| return unless ithenticate_id | |||
| "/recent-activity/plagiarism/report?ithenticate_id=#{ithenticate_id}" | |||
| end | |||
|
|
|||
| def ref_tags_added | |||
| current_refs = self.features && self.features["feature.wikitext.revision.ref_tags"] || 0 | |||
There was a problem hiding this comment.
The guard here against nil features shouldn't be necessary, since that attribute is serialized to hash. If the value is NULL in the database, it will serialize as an empty hash.
self. is also not needed here. The Revision instance will be the default receiver of a method call, so we only need self. in cases where it overlaps with a Ruby keyword or similar edge cases (or when it's important for clarity).
|
Looks like you're picking up the Rails stuff quickly! I suggest that we break up the two schema changes into separate PRs. First, one that adds the Revisions is a very large table, so that particular change I want to make sure we test out on a large database before we deploy. (@bwreid, I'm going to recruit your help for that: we should copy the production database over to staging, and then run that migration via the normal travis deployment process. The thing we want to make sure of is that the migration doesn't take too long, because if it does, the travis build times out after 10 minutes of no output, and will cut the connection and break the migration and deployment. So if it's anywhere close to that, we'll want to run the migration manually when it comes time to deploy to both P&E and Wiki Education production. Depending on how long it takes, we may also need to pause the update processes first.) |
|
For tests, the starting point will be to add some that exercise the basic flow of importing features for several revisions and returning the expected values for
|
db/schema.rb
Outdated
| @@ -237,6 +237,7 @@ | |||
| t.string "real_name" | |||
| t.string "role_description" | |||
| t.integer "total_uploads" | |||
| t.integer "refs_sum_ms", default: 0 | |||
There was a problem hiding this comment.
This is left over from the other now-removed migration.
spec/models/revision_spec.rb
Outdated
| val = Revision.find_by(mw_rev_id: 870348507) | ||
| ref = val.features[refs_tags_key] | ||
| ref_previous = val.features_previous[refs_tags_key] | ||
| expect(val.references_added).to eq(ref - ref_previous) |
There was a problem hiding this comment.
I would do this with explicit values; we can see easily that the test includes 22 refs now vs 17 previously, so instead of doing ref - ref_previous in the test, it ought to just hard-code 5.
app/models/wiki_content/revision.rb
Outdated
| def references_added | ||
| current_refs = features['feature.wikitext.revision.ref_tags'] | ||
| prev_refs = features_previous['feature.wikitext.revision.ref_tags'] || 0 | ||
| return 0 if current_refs.nil? || current_refs < prev_refs |
There was a problem hiding this comment.
Negative value for references added is okay; it should return the negative number instead of zero in that case.
|
This looks good to me. @bwreid can you review it and, when you have a chance, test out the migration as described above? Once the migration is complete, this version shouldn't have any significant performance impact on the system (at least, I don't think), as it's just saving additional data for the same requests that are already happening. For the next stage of it, fetching |
No description provided.