Launchpad itself

Merge ~ilasc/launchpad:delete-revisionstatusreports-for-gitrepository into launchpad:master

  1. Git
  2. lp:~ilasc/launchpad
  3. delete-revisionstatusreports-for-gitrepository
  4. Merge into master
Proposed by Ioana Lasc
Status: Merged
Approved by: Ioana Lasc
Approved revision: 629c234977d2df1359dfb487cc5f9f3e8649fba4
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~ilasc/launchpad:delete-revisionstatusreports-for-gitrepository
Merge into: launchpad:master
Diff against target: 125 lines (+77/-1)
3 files modified
lib/lp/code/interfaces/gitrepository.py (+3/-0)
lib/lp/code/model/gitrepository.py (+17/-1)
lib/lp/code/model/tests/test_gitrepository.py (+57/-0)
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Review via email: mp+413748@code.launchpad.net

Commit message

Delete status reports and artifacts in GitRepository.destroySelf

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) :
review: Needs Fixing
~ilasc/launchpad:delete-revisionstatusreports-for-gitrepository updated
629c234... by Ioana Lasc

Optimize delete query

Revision history for this message
Ioana Lasc (ilasc) wrote :

Indeed the delete query is much better in your suggested optimized format and noted for future reference the "USING" bug, thanks Colin!
Also added other repo, report and artifacts to the unit test - good point!

Revision history for this message
Colin Watson (cjwatson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
2index d2d6a90..e901001 100644
3--- a/lib/lp/code/interfaces/gitrepository.py
4+++ b/lib/lp/code/interfaces/gitrepository.py
5@@ -942,6 +942,9 @@ class IRevisionStatusReportSet(Interface):
6 def findByCommit(repository, commit_sha1):
7 """Returns all `RevisionStatusReport` for a repository and commit."""
8
9+ def deleteForRepository(repository):
10+ """Delete all `RevisionStatusReport` for a repository."""
11+
12
13 class IRevisionStatusArtifactSet(Interface):
14 """The set of all revision status artifacts."""
15diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
16index 6d4fd11..5e2c864 100644
17--- a/lib/lp/code/model/gitrepository.py
18+++ b/lib/lp/code/model/gitrepository.py
19@@ -197,7 +197,10 @@ from lp.services.database.constants import (
20 from lp.services.database.decoratedresultset import DecoratedResultSet
21 from lp.services.database.enumcol import DBEnum
22 from lp.services.database.interfaces import IStore
23-from lp.services.database.sqlbase import get_transaction_timestamp
24+from lp.services.database.sqlbase import (
25+ convert_storm_clause_to_string,
26+ get_transaction_timestamp,
27+ )
28 from lp.services.database.stormbase import StormBase
29 from lp.services.database.stormexpr import (
30 Array,
31@@ -394,6 +397,18 @@ class RevisionStatusReportSet:
32 RevisionStatusReport.date_created,
33 RevisionStatusReport.id)
34
35+ def deleteForRepository(self, repository):
36+ clauses = [
37+ RevisionStatusArtifact.report == RevisionStatusReport.id,
38+ RevisionStatusReport.git_repository == repository,
39+ ]
40+ where = convert_storm_clause_to_string(And(*clauses))
41+ IStore(RevisionStatusArtifact).execute("""
42+ DELETE FROM RevisionStatusArtifact
43+ USING RevisionStatusReport
44+ WHERE """ + where)
45+ self.findByRepository(repository).remove()
46+
47
48 class RevisionStatusArtifact(StormBase):
49 __storm_table__ = 'RevisionStatusArtifact'
50@@ -1944,6 +1959,7 @@ class GitRepository(StormBase, WebhookTargetMixin, AccessTokenTargetMixin,
51 # activity logs for removed repositories anyway.
52 self.grants.remove()
53 self.rules.remove()
54+ getUtility(IRevisionStatusReportSet).deleteForRepository(self)
55
56 # Now destroy the repository.
57 repository_name = self.unique_name
58diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
59index 6155836..f9be757 100644
60--- a/lib/lp/code/model/tests/test_gitrepository.py
61+++ b/lib/lp/code/model/tests/test_gitrepository.py
62@@ -818,6 +818,63 @@ class TestGitRepositoryDeletion(TestCaseWithFactory):
63 getUtility(IGitLookup).get(repository_id),
64 "The repository has not been deleted.")
65
66+ def test_revision_status_reports_do_not_disable_deletion(self):
67+ title = self.factory.getUniqueUnicode('report-title')
68+ result_summary = "120/120 tests passed"
69+ commit_sha1 = hashlib.sha1(
70+ self.factory.getUniqueBytes()).hexdigest()
71+ result_summary2 = "Lint"
72+ title2 = "Invalid import in test_file.py"
73+
74+ self.factory.makeRevisionStatusReport(
75+ user=self.repository.owner, git_repository=self.repository,
76+ title=title, commit_sha1=commit_sha1,
77+ result_summary=result_summary,
78+ result=RevisionStatusResult.SUCCEEDED)
79+
80+ report2 = self.factory.makeRevisionStatusReport(
81+ user=self.repository.owner, git_repository=self.repository,
82+ title=title2, commit_sha1=commit_sha1,
83+ result_summary=result_summary2,
84+ result=RevisionStatusResult.FAILED)
85+
86+ self.factory.makeRevisionStatusArtifact(report=report2)
87+ self.factory.makeRevisionStatusArtifact(report=report2)
88+ self.assertEqual(2, len(list(
89+ getUtility(IRevisionStatusArtifactSet).findByReport(report2))))
90+
91+ # Create here one report and artifact under a different repo
92+ # and ensure below once self.repository is deleted the report
93+ # and artifacts under other_repository remain intact
94+ other_repository = self.factory.makeGitRepository()
95+ other_report = self.factory.makeRevisionStatusReport(
96+ user=self.repository.owner, git_repository=other_repository,
97+ title=title, commit_sha1=commit_sha1,
98+ result_summary=result_summary,
99+ result=RevisionStatusResult.SUCCEEDED)
100+ self.factory.makeRevisionStatusArtifact(report=other_report)
101+ self.factory.makeRevisionStatusArtifact(report=other_report)
102+
103+ self.assertTrue(
104+ self.repository.canBeDeleted(),
105+ "A newly created repository should be able to be deleted.")
106+ repository_id = self.repository.id
107+ self.repository.destroySelf()
108+ self.assertIsNone(
109+ getUtility(IGitLookup).get(repository_id),
110+ "The repository has not been deleted.")
111+ self.assertEqual(0, len(list(getUtility(
112+ IRevisionStatusReportSet).findByRepository(self.repository))))
113+ self.assertEqual(0, len(list(getUtility(
114+ IRevisionStatusArtifactSet).findByReport(report2))))
115+
116+ # Ensure that once self.repository is deleted the report
117+ # and artifacts under other_repository remain intact
118+ self.assertEqual(2, len(list(getUtility(
119+ IRevisionStatusArtifactSet).findByReport(other_report))))
120+ self.assertEqual(1, len(list(getUtility(
121+ IRevisionStatusReportSet).findByRepository(other_repository))))
122+
123 def test_subscription_does_not_disable_deletion(self):
124 # A repository that has a subscription can be deleted.
125 self.repository.subscribe(

Subscribers

People subscribed via source and target branches

to status/vote changes: