Merge ~ilasc/launchpad:person-oci-registry-credentials-edit into launchpad:master
- Git
- lp:~ilasc/launchpad
- person-oci-registry-credentials-edit
- Merge into master
Status: | Merged |
---|---|
Approved by: | Ioana Lasc |
Approved revision: | e69186d4fb331a4444c411ea9c999bb4e2ec1055 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~ilasc/launchpad:person-oci-registry-credentials-edit |
Merge into: | launchpad:master |
Diff against target: |
677 lines (+540/-4) 8 files modified
lib/lp/oci/interfaces/ocipushrule.py (+3/-0) lib/lp/oci/model/ocipushrule.py (+7/-1) lib/lp/oci/model/ociregistrycredentials.py (+5/-3) lib/lp/registry/browser/configure.zcml (+6/-0) lib/lp/registry/browser/person.py (+298/-0) lib/lp/registry/browser/tests/test_person.py (+158/-0) lib/lp/registry/templates/person-edit-ociregistrycredentials.pt (+59/-0) lib/lp/registry/templates/person-ociregistrycredentials.pt (+4/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tom Wardill (community) | Approve | ||
Thiago F. Pappacena (community) | Approve | ||
Review via email: mp+385825@code.launchpad.net |
Commit message
Add edit screen for OCIRegistryCred
Description of the change
Thiago F. Pappacena (pappacena) wrote : | # |
Ioana Lasc (ilasc) wrote : | # |
Thanks Thiago, I added replies to each inline comment and will commit all changes in a few minutes - I agree with all comments and addressed accordingly in the code.
Thiago F. Pappacena (pappacena) wrote : | # |
Added a reply, but other than that it LGTM.
Maybe twom would like to take a quick look here too.
Ioana Lasc (ilasc) wrote : | # |
Thiago that last suggestion was good - agreed and added.
Tom Wardill (twom) wrote : | # |
Some comments inline, mostly indentation and such.
The use of removeSecurityProxy in both the tests and the especially the view context seems a bit strange to me, is it required?
I can't see why you don't have the right privileges to update the objects without it. That said, my understanding of how all that works is a bit hazy!
Ioana Lasc (ilasc) wrote : | # |
Thanks Tom! Indeed there were a couple of places on the view where access to the person object in the context didn't require removeSecurityProxy (setting owner=self.context) but it is necessary everywhere else.
Preview Diff
1 | diff --git a/lib/lp/oci/interfaces/ocipushrule.py b/lib/lp/oci/interfaces/ocipushrule.py | |||
2 | index b889cdd..45aeb3b 100644 | |||
3 | --- a/lib/lp/oci/interfaces/ocipushrule.py | |||
4 | +++ b/lib/lp/oci/interfaces/ocipushrule.py | |||
5 | @@ -127,5 +127,8 @@ class IOCIPushRuleSet(Interface): | |||
6 | 127 | def new(recipe, registry_credentials, image_name): | 127 | def new(recipe, registry_credentials, image_name): |
7 | 128 | """Create an `IOCIPushRule`.""" | 128 | """Create an `IOCIPushRule`.""" |
8 | 129 | 129 | ||
9 | 130 | def findByRegistryCredentials(self, credentials): | ||
10 | 131 | """Find matching `IOCIPushRule` by credentials.""" | ||
11 | 132 | |||
12 | 130 | def getByID(id): | 133 | def getByID(id): |
13 | 131 | """Get a single `IOCIPushRule` by its ID.""" | 134 | """Get a single `IOCIPushRule` by its ID.""" |
14 | diff --git a/lib/lp/oci/model/ocipushrule.py b/lib/lp/oci/model/ocipushrule.py | |||
15 | index dbdb22e..0354aad 100644 | |||
16 | --- a/lib/lp/oci/model/ocipushrule.py | |||
17 | +++ b/lib/lp/oci/model/ocipushrule.py | |||
18 | @@ -69,7 +69,7 @@ class OCIPushRule(Storm): | |||
19 | 69 | 69 | ||
20 | 70 | def destroySelf(self): | 70 | def destroySelf(self): |
21 | 71 | """See `IOCIPushRule`.""" | 71 | """See `IOCIPushRule`.""" |
23 | 72 | IStore(OCIPushRule).get(self.id).remove() | 72 | IStore(OCIPushRule).remove(self) |
24 | 73 | 73 | ||
25 | 74 | 74 | ||
26 | 75 | @implementer(IOCIPushRuleSet) | 75 | @implementer(IOCIPushRuleSet) |
27 | @@ -90,3 +90,9 @@ class OCIPushRuleSet: | |||
28 | 90 | def getByID(self, id): | 90 | def getByID(self, id): |
29 | 91 | """See `IOCIPushRuleSet`.""" | 91 | """See `IOCIPushRuleSet`.""" |
30 | 92 | return IStore(OCIPushRule).get(OCIPushRule, id) | 92 | return IStore(OCIPushRule).get(OCIPushRule, id) |
31 | 93 | |||
32 | 94 | def findByRegistryCredentials(self, credentials): | ||
33 | 95 | store = IStore(OCIPushRule) | ||
34 | 96 | return store.find( | ||
35 | 97 | OCIPushRule, | ||
36 | 98 | OCIPushRule.registry_credentials == credentials) | ||
37 | diff --git a/lib/lp/oci/model/ociregistrycredentials.py b/lib/lp/oci/model/ociregistrycredentials.py | |||
38 | index 49c740f..d86b140 100644 | |||
39 | --- a/lib/lp/oci/model/ociregistrycredentials.py | |||
40 | +++ b/lib/lp/oci/model/ociregistrycredentials.py | |||
41 | @@ -122,11 +122,13 @@ class OCIRegistryCredentials(Storm): | |||
42 | 122 | def username(self): | 122 | def username(self): |
43 | 123 | return self._credentials.get('username') | 123 | return self._credentials.get('username') |
44 | 124 | 124 | ||
45 | 125 | @username.setter | ||
46 | 126 | def username(self, value): | ||
47 | 127 | self._credentials['username'] = value | ||
48 | 128 | |||
49 | 125 | def destroySelf(self): | 129 | def destroySelf(self): |
50 | 126 | """See `IOCIRegistryCredentials`.""" | 130 | """See `IOCIRegistryCredentials`.""" |
54 | 127 | store = IStore(OCIRegistryCredentials) | 131 | IStore(OCIRegistryCredentials).remove(self) |
52 | 128 | store.find( | ||
53 | 129 | OCIRegistryCredentials, OCIRegistryCredentials.id == self).remove() | ||
55 | 130 | 132 | ||
56 | 131 | 133 | ||
57 | 132 | @implementer(IOCIRegistryCredentialsSet) | 134 | @implementer(IOCIRegistryCredentialsSet) |
58 | diff --git a/lib/lp/registry/browser/configure.zcml b/lib/lp/registry/browser/configure.zcml | |||
59 | index b8f00d5..3707a8e 100644 | |||
60 | --- a/lib/lp/registry/browser/configure.zcml | |||
61 | +++ b/lib/lp/registry/browser/configure.zcml | |||
62 | @@ -1238,6 +1238,12 @@ | |||
63 | 1238 | template="../templates/person-ociregistrycredentials.pt" | 1238 | template="../templates/person-ociregistrycredentials.pt" |
64 | 1239 | /> | 1239 | /> |
65 | 1240 | <browser:page | 1240 | <browser:page |
66 | 1241 | for="lp.registry.interfaces.person.IPerson" | ||
67 | 1242 | class="lp.registry.browser.person.PersonEditOCIRegistryCredentialsView" | ||
68 | 1243 | permission="launchpad.Edit" | ||
69 | 1244 | name="+edit-oci-registry-credentials" | ||
70 | 1245 | template="../templates/person-edit-ociregistrycredentials.pt" /> | ||
71 | 1246 | <browser:page | ||
72 | 1241 | name="+livefs" | 1247 | name="+livefs" |
73 | 1242 | for="lp.registry.interfaces.person.IPerson" | 1248 | for="lp.registry.interfaces.person.IPerson" |
74 | 1243 | class="lp.registry.browser.person.PersonLiveFSView" | 1249 | class="lp.registry.browser.person.PersonLiveFSView" |
75 | diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py | |||
76 | index 535cf4d..e46cce4 100644 | |||
77 | --- a/lib/lp/registry/browser/person.py | |||
78 | +++ b/lib/lp/registry/browser/person.py | |||
79 | @@ -7,6 +7,7 @@ __metaclass__ = type | |||
80 | 7 | __all__ = [ | 7 | __all__ = [ |
81 | 8 | 'BeginTeamClaimView', | 8 | 'BeginTeamClaimView', |
82 | 9 | 'CommonMenuLinks', | 9 | 'CommonMenuLinks', |
83 | 10 | 'PersonEditOCIRegistryCredentialsView', | ||
84 | 10 | 'EmailToPersonView', | 11 | 'EmailToPersonView', |
85 | 11 | 'PeopleSearchView', | 12 | 'PeopleSearchView', |
86 | 12 | 'PersonAccountAdministerView', | 13 | 'PersonAccountAdministerView', |
87 | @@ -90,7 +91,9 @@ from zope.interface import ( | |||
88 | 90 | from zope.interface.exceptions import Invalid | 91 | from zope.interface.exceptions import Invalid |
89 | 91 | from zope.publisher.interfaces import NotFound | 92 | from zope.publisher.interfaces import NotFound |
90 | 92 | from zope.schema import ( | 93 | from zope.schema import ( |
91 | 94 | Bool, | ||
92 | 93 | Choice, | 95 | Choice, |
93 | 96 | Password, | ||
94 | 94 | Text, | 97 | Text, |
95 | 95 | TextLine, | 98 | TextLine, |
96 | 96 | ) | 99 | ) |
97 | @@ -134,8 +137,10 @@ from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin | |||
98 | 134 | from lp.code.errors import InvalidNamespace | 137 | from lp.code.errors import InvalidNamespace |
99 | 135 | from lp.code.interfaces.branchnamespace import IBranchNamespaceSet | 138 | from lp.code.interfaces.branchnamespace import IBranchNamespaceSet |
100 | 136 | from lp.code.interfaces.gitlookup import IGitTraverser | 139 | from lp.code.interfaces.gitlookup import IGitTraverser |
101 | 140 | from lp.oci.interfaces.ocipushrule import IOCIPushRuleSet | ||
102 | 137 | from lp.oci.interfaces.ociregistrycredentials import ( | 141 | from lp.oci.interfaces.ociregistrycredentials import ( |
103 | 138 | IOCIRegistryCredentialsSet, | 142 | IOCIRegistryCredentialsSet, |
104 | 143 | OCIRegistryCredentialsAlreadyExist, | ||
105 | 139 | ) | 144 | ) |
106 | 140 | from lp.registry.browser import BaseRdfView | 145 | from lp.registry.browser import BaseRdfView |
107 | 141 | from lp.registry.browser.branding import BrandingChangeView | 146 | from lp.registry.browser.branding import BrandingChangeView |
108 | @@ -3659,6 +3664,299 @@ class PersonOCIRegistryCredentialsView(LaunchpadView): | |||
109 | 3659 | return len(self.oci_registry_credentials) > 0 | 3664 | return len(self.oci_registry_credentials) > 0 |
110 | 3660 | 3665 | ||
111 | 3661 | 3666 | ||
112 | 3667 | class PersonEditOCIRegistryCredentialsView(LaunchpadFormView): | ||
113 | 3668 | """View for Person:+edit-oci-registry-credentials.""" | ||
114 | 3669 | |||
115 | 3670 | @cachedproperty | ||
116 | 3671 | def oci_registry_credentials(self): | ||
117 | 3672 | return list(getUtility( | ||
118 | 3673 | IOCIRegistryCredentialsSet).findByOwner(self.context)) | ||
119 | 3674 | |||
120 | 3675 | schema = Interface | ||
121 | 3676 | |||
122 | 3677 | def _getFieldName(self, name, credentials_id): | ||
123 | 3678 | """Get the combined field name for an `OCIRegistryCredentials` ID. | ||
124 | 3679 | |||
125 | 3680 | In order to be able to render a table, we encode the credentials ID | ||
126 | 3681 | in the form field name. | ||
127 | 3682 | """ | ||
128 | 3683 | return "%s.%d" % (name, credentials_id) | ||
129 | 3684 | |||
130 | 3685 | def getEditFieldsRow(self, credentials=None): | ||
131 | 3686 | id = getattr(credentials, 'id', None) | ||
132 | 3687 | owner = Choice( | ||
133 | 3688 | title=u'Owner', | ||
134 | 3689 | vocabulary=( | ||
135 | 3690 | 'AllUserTeamsParticipationPlusSelfSimpleDisplay'), | ||
136 | 3691 | default=credentials.owner.name, | ||
137 | 3692 | __name__=self._getFieldName('owner', id)) | ||
138 | 3693 | |||
139 | 3694 | username = TextLine( | ||
140 | 3695 | title=u'Username', | ||
141 | 3696 | __name__=self._getFieldName('username', id), | ||
142 | 3697 | default=credentials.username, | ||
143 | 3698 | required=False, readonly=False) | ||
144 | 3699 | |||
145 | 3700 | password = Password( | ||
146 | 3701 | title=u'Password', | ||
147 | 3702 | __name__=self._getFieldName('password', id), | ||
148 | 3703 | default=None, | ||
149 | 3704 | required=False, readonly=False) | ||
150 | 3705 | |||
151 | 3706 | confirm_password = Password( | ||
152 | 3707 | title=u'Confirm password', | ||
153 | 3708 | __name__=self._getFieldName('confirm_password', id), | ||
154 | 3709 | default=None, | ||
155 | 3710 | required=False, readonly=False) | ||
156 | 3711 | |||
157 | 3712 | url = TextLine( | ||
158 | 3713 | title=u'Registry URL', | ||
159 | 3714 | __name__=self._getFieldName('url', id), | ||
160 | 3715 | default=credentials.url, | ||
161 | 3716 | required=True, readonly=False) | ||
162 | 3717 | |||
163 | 3718 | delete = Bool( | ||
164 | 3719 | title=u'Delete', | ||
165 | 3720 | __name__=self._getFieldName('delete', id), | ||
166 | 3721 | default=False, | ||
167 | 3722 | required=True, readonly=False) | ||
168 | 3723 | |||
169 | 3724 | return owner, username, password, confirm_password, url, delete | ||
170 | 3725 | |||
171 | 3726 | def getAddFieldsRow(self): | ||
172 | 3727 | add_url = TextLine( | ||
173 | 3728 | title=u'Registry URL', | ||
174 | 3729 | __name__=u'add_url', | ||
175 | 3730 | required=False, readonly=False) | ||
176 | 3731 | add_username = TextLine( | ||
177 | 3732 | title=u'Username', | ||
178 | 3733 | __name__=u'add_username', | ||
179 | 3734 | required=False, readonly=False) | ||
180 | 3735 | add_password = Password( | ||
181 | 3736 | title=u'Password', | ||
182 | 3737 | __name__=u'add_password', | ||
183 | 3738 | required=False, readonly=False) | ||
184 | 3739 | add_confirm_password = Password( | ||
185 | 3740 | title=u'Confirm password', | ||
186 | 3741 | __name__=u'add_confirm_password', | ||
187 | 3742 | required=False, readonly=False) | ||
188 | 3743 | |||
189 | 3744 | return add_url, add_username, add_password, add_confirm_password | ||
190 | 3745 | |||
191 | 3746 | def _parseFieldName(self, field_name): | ||
192 | 3747 | """Parse a combined field name as described in `_getFieldName`. | ||
193 | 3748 | |||
194 | 3749 | :raises UnexpectedFormData: if the field name cannot be parsed or | ||
195 | 3750 | the `OCIRegistryCredentials` cannot be found. | ||
196 | 3751 | """ | ||
197 | 3752 | field_bits = field_name.split(".") | ||
198 | 3753 | if len(field_bits) != 2: | ||
199 | 3754 | raise UnexpectedFormData( | ||
200 | 3755 | "Cannot parse field name: %s" % field_name) | ||
201 | 3756 | field_type = field_bits[0] | ||
202 | 3757 | try: | ||
203 | 3758 | credentials_id = int(field_bits[1]) | ||
204 | 3759 | except ValueError: | ||
205 | 3760 | raise UnexpectedFormData( | ||
206 | 3761 | "Cannot parse field name: %s" % field_name) | ||
207 | 3762 | return field_type, credentials_id | ||
208 | 3763 | |||
209 | 3764 | def setUpFields(self): | ||
210 | 3765 | """See `LaunchpadFormView`.""" | ||
211 | 3766 | LaunchpadFormView.setUpFields(self) | ||
212 | 3767 | |||
213 | 3768 | for elem in self.oci_registry_credentials: | ||
214 | 3769 | fields = self.getEditFieldsRow(elem) | ||
215 | 3770 | self.form_fields += FormFields(*fields) | ||
216 | 3771 | |||
217 | 3772 | add_fields = self.getAddFieldsRow() | ||
218 | 3773 | self.form_fields += FormFields(*add_fields) | ||
219 | 3774 | |||
220 | 3775 | @property | ||
221 | 3776 | def label(self): | ||
222 | 3777 | return 'Edit OCI registry credentials' | ||
223 | 3778 | |||
224 | 3779 | @property | ||
225 | 3780 | def cancel_url(self): | ||
226 | 3781 | return canonical_url(self.context) | ||
227 | 3782 | |||
228 | 3783 | def getCredentialsWidgets(self, credentials): | ||
229 | 3784 | widgets_by_name = {widget.name: widget for widget in self.widgets} | ||
230 | 3785 | owner_field_name = ( | ||
231 | 3786 | "field." + self._getFieldName("owner", credentials.id)) | ||
232 | 3787 | username_field_name = ( | ||
233 | 3788 | "field." + self._getFieldName("username", credentials.id)) | ||
234 | 3789 | password_field_name = ( | ||
235 | 3790 | "field." + self._getFieldName("password", credentials.id)) | ||
236 | 3791 | confirm_password_field_name = ( | ||
237 | 3792 | "field." + self._getFieldName("confirm_password", | ||
238 | 3793 | credentials.id)) | ||
239 | 3794 | url_field_name = "field." + self._getFieldName("url", credentials.id) | ||
240 | 3795 | delete_field_name = ( | ||
241 | 3796 | "field." + self._getFieldName("delete", credentials.id)) | ||
242 | 3797 | return { | ||
243 | 3798 | "owner": widgets_by_name[owner_field_name], | ||
244 | 3799 | "username": widgets_by_name[username_field_name], | ||
245 | 3800 | "password": widgets_by_name[password_field_name], | ||
246 | 3801 | "confirm_password": widgets_by_name[confirm_password_field_name], | ||
247 | 3802 | "url": widgets_by_name[url_field_name], | ||
248 | 3803 | "delete": widgets_by_name[delete_field_name] | ||
249 | 3804 | } | ||
250 | 3805 | |||
251 | 3806 | def parseData(self, data): | ||
252 | 3807 | """Rearrange form data to make it easier to process.""" | ||
253 | 3808 | parsed_data = {} | ||
254 | 3809 | add_url = data["add_url"] | ||
255 | 3810 | add_username = data["add_username"] | ||
256 | 3811 | add_password = data["add_password"] | ||
257 | 3812 | add_confirm_password = data["add_confirm_password"] | ||
258 | 3813 | if add_url or add_username or add_password or add_confirm_password: | ||
259 | 3814 | parsed_data.setdefault(None, { | ||
260 | 3815 | "username": add_username, | ||
261 | 3816 | "password": add_password, | ||
262 | 3817 | "confirm_password": add_confirm_password, | ||
263 | 3818 | "url": add_url, | ||
264 | 3819 | "action": "add", | ||
265 | 3820 | }) | ||
266 | 3821 | for field_name in ( | ||
267 | 3822 | name for name in data if name.split(".")[0] == "owner"): | ||
268 | 3823 | _, credentials_id = self._parseFieldName(field_name) | ||
269 | 3824 | owner_field_name = self._getFieldName( | ||
270 | 3825 | "owner", credentials_id) | ||
271 | 3826 | username_field_name = self._getFieldName( | ||
272 | 3827 | "username", credentials_id) | ||
273 | 3828 | password_field_name = self._getFieldName( | ||
274 | 3829 | "password", credentials_id) | ||
275 | 3830 | confirm_password_field_name = self._getFieldName( | ||
276 | 3831 | "confirm_password", credentials_id) | ||
277 | 3832 | url_field_name = self._getFieldName("url", credentials_id) | ||
278 | 3833 | delete_field_name = self._getFieldName("delete", credentials_id) | ||
279 | 3834 | if data.get(delete_field_name): | ||
280 | 3835 | action = "delete" | ||
281 | 3836 | else: | ||
282 | 3837 | action = "change" | ||
283 | 3838 | parsed_data.setdefault(credentials_id, { | ||
284 | 3839 | "username": data.get(username_field_name), | ||
285 | 3840 | "password": data.get(password_field_name), | ||
286 | 3841 | "confirm_password": data.get(confirm_password_field_name), | ||
287 | 3842 | "url": data.get(url_field_name), | ||
288 | 3843 | "owner": data.get(owner_field_name), | ||
289 | 3844 | "action": action, | ||
290 | 3845 | }) | ||
291 | 3846 | |||
292 | 3847 | return parsed_data | ||
293 | 3848 | |||
294 | 3849 | def changeCredentials(self, parsed_credentials, credentials): | ||
295 | 3850 | username = parsed_credentials["username"] | ||
296 | 3851 | password = parsed_credentials["password"] | ||
297 | 3852 | confirm_password = parsed_credentials["confirm_password"] | ||
298 | 3853 | owner = parsed_credentials["owner"] | ||
299 | 3854 | if password or confirm_password: | ||
300 | 3855 | if password != confirm_password: | ||
301 | 3856 | self.setFieldError( | ||
302 | 3857 | self._getFieldName( | ||
303 | 3858 | "confirm_password", credentials.id), | ||
304 | 3859 | "Passwords do not match.") | ||
305 | 3860 | else: | ||
306 | 3861 | credentials.setCredentials( | ||
307 | 3862 | {"username": username, | ||
308 | 3863 | "password": password}) | ||
309 | 3864 | credentials.url = parsed_credentials["url"] | ||
310 | 3865 | elif username != credentials.username: | ||
311 | 3866 | removeSecurityProxy(credentials).username = username | ||
312 | 3867 | credentials.url = parsed_credentials["url"] | ||
313 | 3868 | elif parsed_credentials["url"] != credentials.url: | ||
314 | 3869 | credentials.url = parsed_credentials["url"] | ||
315 | 3870 | if owner != credentials.owner: | ||
316 | 3871 | credentials.owner = owner | ||
317 | 3872 | |||
318 | 3873 | def deleteCredentials(self, credentials): | ||
319 | 3874 | push_rule_set = getUtility(IOCIPushRuleSet) | ||
320 | 3875 | if not push_rule_set.findByRegistryCredentials( | ||
321 | 3876 | credentials).is_empty(): | ||
322 | 3877 | self.setFieldError( | ||
323 | 3878 | self._getFieldName( | ||
324 | 3879 | "delete", credentials.id), | ||
325 | 3880 | "These credentials cannot be deleted as there are " | ||
326 | 3881 | "push rules defined that still use them.") | ||
327 | 3882 | else: | ||
328 | 3883 | credentials.destroySelf() | ||
329 | 3884 | |||
330 | 3885 | def addCredentials(self, parsed_add_credentials): | ||
331 | 3886 | url = parsed_add_credentials["url"] | ||
332 | 3887 | password = parsed_add_credentials["password"] | ||
333 | 3888 | confirm_password = parsed_add_credentials["confirm_password"] | ||
334 | 3889 | username = parsed_add_credentials["username"] | ||
335 | 3890 | if url: | ||
336 | 3891 | if password or confirm_password: | ||
337 | 3892 | if not password == confirm_password: | ||
338 | 3893 | self.setFieldError( | ||
339 | 3894 | "add_password", | ||
340 | 3895 | "Please make sure the new " | ||
341 | 3896 | "password matches the " | ||
342 | 3897 | "confirm password field.") | ||
343 | 3898 | return | ||
344 | 3899 | |||
345 | 3900 | credentials = { | ||
346 | 3901 | 'username': username, | ||
347 | 3902 | 'password': password} | ||
348 | 3903 | try: | ||
349 | 3904 | getUtility(IOCIRegistryCredentialsSet).new( | ||
350 | 3905 | owner=self.context, | ||
351 | 3906 | url=url, | ||
352 | 3907 | credentials=credentials) | ||
353 | 3908 | except OCIRegistryCredentialsAlreadyExist: | ||
354 | 3909 | self.setFieldError( | ||
355 | 3910 | "add_url", | ||
356 | 3911 | "Credentials already exist " | ||
357 | 3912 | "with the same URL and " | ||
358 | 3913 | "username.") | ||
359 | 3914 | else: | ||
360 | 3915 | credentials = {'username': username} | ||
361 | 3916 | try: | ||
362 | 3917 | getUtility(IOCIRegistryCredentialsSet).new( | ||
363 | 3918 | owner=self.context, | ||
364 | 3919 | url=url, | ||
365 | 3920 | credentials=credentials) | ||
366 | 3921 | except OCIRegistryCredentialsAlreadyExist: | ||
367 | 3922 | self.setFieldError( | ||
368 | 3923 | "add_url", | ||
369 | 3924 | "Credentials already exist " | ||
370 | 3925 | "with the same URL and username.") | ||
371 | 3926 | else: | ||
372 | 3927 | self.setFieldError( | ||
373 | 3928 | "add_url", | ||
374 | 3929 | "Registry URL cannot be empty.") | ||
375 | 3930 | |||
376 | 3931 | def updateCredentialsFromData(self, parsed_data): | ||
377 | 3932 | credentials_map = { | ||
378 | 3933 | credentials.id: credentials | ||
379 | 3934 | for credentials in self.oci_registry_credentials} | ||
380 | 3935 | |||
381 | 3936 | for credentials_id, parsed_credentials in parsed_data.items(): | ||
382 | 3937 | credentials = credentials_map.get(credentials_id) | ||
383 | 3938 | action = parsed_credentials["action"] | ||
384 | 3939 | |||
385 | 3940 | if action == "change": | ||
386 | 3941 | self.changeCredentials(parsed_credentials, credentials) | ||
387 | 3942 | elif action == "delete": | ||
388 | 3943 | self.deleteCredentials(credentials) | ||
389 | 3944 | elif action == "add": | ||
390 | 3945 | parsed_add_credentials = parsed_data[credentials] | ||
391 | 3946 | self.addCredentials(parsed_add_credentials) | ||
392 | 3947 | else: | ||
393 | 3948 | raise AssertionError("unknown action: %s" % action) | ||
394 | 3949 | |||
395 | 3950 | @action("Save") | ||
396 | 3951 | def save(self, action, data): | ||
397 | 3952 | parsed_data = self.parseData(data) | ||
398 | 3953 | self.updateCredentialsFromData(parsed_data) | ||
399 | 3954 | |||
400 | 3955 | if not self.errors: | ||
401 | 3956 | self.request.response.addNotification("Saved credentials") | ||
402 | 3957 | self.next_url = canonical_url(self.context) | ||
403 | 3958 | |||
404 | 3959 | |||
405 | 3662 | class PersonLiveFSView(LaunchpadView): | 3960 | class PersonLiveFSView(LaunchpadView): |
406 | 3663 | """Default view for the list of live filesystems owned by a person.""" | 3961 | """Default view for the list of live filesystems owned by a person.""" |
407 | 3664 | page_title = 'LiveFS' | 3962 | page_title = 'LiveFS' |
408 | diff --git a/lib/lp/registry/browser/tests/test_person.py b/lib/lp/registry/browser/tests/test_person.py | |||
409 | index 1264643..cc35863 100644 | |||
410 | --- a/lib/lp/registry/browser/tests/test_person.py | |||
411 | +++ b/lib/lp/registry/browser/tests/test_person.py | |||
412 | @@ -19,6 +19,7 @@ from testtools.matchers import ( | |||
413 | 19 | DocTestMatches, | 19 | DocTestMatches, |
414 | 20 | Equals, | 20 | Equals, |
415 | 21 | LessThan, | 21 | LessThan, |
416 | 22 | MatchesDict, | ||
417 | 22 | Not, | 23 | Not, |
418 | 23 | ) | 24 | ) |
419 | 24 | from testtools.testcase import ExpectedException | 25 | from testtools.testcase import ExpectedException |
420 | @@ -34,6 +35,7 @@ from lp.app.errors import NotFoundError | |||
421 | 34 | from lp.app.interfaces.launchpad import ILaunchpadCelebrities | 35 | from lp.app.interfaces.launchpad import ILaunchpadCelebrities |
422 | 35 | from lp.blueprints.enums import SpecificationImplementationStatus | 36 | from lp.blueprints.enums import SpecificationImplementationStatus |
423 | 36 | from lp.buildmaster.enums import BuildStatus | 37 | from lp.buildmaster.enums import BuildStatus |
424 | 38 | from lp.oci.interfaces.ocipushrule import IOCIPushRuleSet | ||
425 | 37 | from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE | 39 | from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE |
426 | 38 | from lp.oci.interfaces.ociregistrycredentials import ( | 40 | from lp.oci.interfaces.ociregistrycredentials import ( |
427 | 39 | IOCIRegistryCredentialsSet, | 41 | IOCIRegistryCredentialsSet, |
428 | @@ -53,6 +55,7 @@ from lp.registry.model.karma import KarmaCategory | |||
429 | 53 | from lp.registry.model.milestone import milestone_sort_key | 55 | from lp.registry.model.milestone import milestone_sort_key |
430 | 54 | from lp.scripts.garbo import PopulateLatestPersonSourcePackageReleaseCache | 56 | from lp.scripts.garbo import PopulateLatestPersonSourcePackageReleaseCache |
431 | 55 | from lp.services.config import config | 57 | from lp.services.config import config |
432 | 58 | from lp.services.database.interfaces import IStore | ||
433 | 56 | from lp.services.features.testing import FeatureFixture | 59 | from lp.services.features.testing import FeatureFixture |
434 | 57 | from lp.services.identity.interfaces.account import AccountStatus | 60 | from lp.services.identity.interfaces.account import AccountStatus |
435 | 58 | from lp.services.identity.interfaces.emailaddress import IEmailAddressSet | 61 | from lp.services.identity.interfaces.emailaddress import IEmailAddressSet |
436 | @@ -1350,6 +1353,161 @@ class TestPersonOCIRegistryCredentialsView(BrowserTestCase, | |||
437 | 1350 | view.oci_registry_credentials[0].getCredentials()['username']) | 1353 | view.oci_registry_credentials[0].getCredentials()['username']) |
438 | 1351 | self.assertEqual(url, view.oci_registry_credentials[0].url) | 1354 | self.assertEqual(url, view.oci_registry_credentials[0].url) |
439 | 1352 | 1355 | ||
440 | 1356 | def test_edit_oci_registry_creds_on_person_page(self): | ||
441 | 1357 | url = unicode(self.factory.getUniqueURL()) | ||
442 | 1358 | newurl = unicode(self.factory.getUniqueURL()) | ||
443 | 1359 | third_url = unicode(self.factory.getUniqueURL()) | ||
444 | 1360 | credentials = {'username': 'foo', 'password': 'bar'} | ||
445 | 1361 | registry_credentials = getUtility(IOCIRegistryCredentialsSet).new( | ||
446 | 1362 | owner=self.user, | ||
447 | 1363 | url=url, | ||
448 | 1364 | credentials=credentials) | ||
449 | 1365 | |||
450 | 1366 | browser = self.getViewBrowser( | ||
451 | 1367 | self.user, view_name='+oci-registry-credentials', user=self.user) | ||
452 | 1368 | browser.getLink("Edit OCI registry credentials").click() | ||
453 | 1369 | |||
454 | 1370 | # Change only the username | ||
455 | 1371 | registry_credentials_id = removeSecurityProxy(registry_credentials).id | ||
456 | 1372 | username_control = browser.getControl( | ||
457 | 1373 | name="field.username.%d" % registry_credentials_id) | ||
458 | 1374 | username_control.value = 'different_username' | ||
459 | 1375 | browser.getControl("Save").click() | ||
460 | 1376 | with person_logged_in(self.user): | ||
461 | 1377 | self.assertThat( | ||
462 | 1378 | registry_credentials.getCredentials(), | ||
463 | 1379 | MatchesDict( | ||
464 | 1380 | {"username": Equals("different_username"), | ||
465 | 1381 | "password": Equals("bar")})) | ||
466 | 1382 | |||
467 | 1383 | # change only the registry url | ||
468 | 1384 | browser = self.getViewBrowser( | ||
469 | 1385 | self.user, view_name='+oci-registry-credentials', user=self.user) | ||
470 | 1386 | browser.getLink("Edit OCI registry credentials").click() | ||
471 | 1387 | url_control = browser.getControl( | ||
472 | 1388 | name="field.url.%d" % registry_credentials_id) | ||
473 | 1389 | url_control.value = newurl | ||
474 | 1390 | browser.getControl("Save").click() | ||
475 | 1391 | with person_logged_in(self.user): | ||
476 | 1392 | self.assertEqual(newurl, registry_credentials.url) | ||
477 | 1393 | |||
478 | 1394 | # change only the password | ||
479 | 1395 | browser = self.getViewBrowser( | ||
480 | 1396 | self.user, view_name='+oci-registry-credentials', user=self.user) | ||
481 | 1397 | browser.getLink("Edit OCI registry credentials").click() | ||
482 | 1398 | password_control = browser.getControl( | ||
483 | 1399 | name="field.password.%d" % registry_credentials_id) | ||
484 | 1400 | password_control.value = 'newpassword' | ||
485 | 1401 | |||
486 | 1402 | browser.getControl("Save").click() | ||
487 | 1403 | self.assertIn("Passwords do not match.", browser.contents) | ||
488 | 1404 | |||
489 | 1405 | # change all fields with one edit action | ||
490 | 1406 | username_control = browser.getControl( | ||
491 | 1407 | name="field.username.%d" % registry_credentials_id) | ||
492 | 1408 | username_control.value = 'third_different_username' | ||
493 | 1409 | url_control = browser.getControl( | ||
494 | 1410 | name="field.url.%d" % registry_credentials_id) | ||
495 | 1411 | url_control.value = third_url | ||
496 | 1412 | password_control = browser.getControl( | ||
497 | 1413 | name="field.password.%d" % registry_credentials_id) | ||
498 | 1414 | password_control.value = 'third_newpassword' | ||
499 | 1415 | confirm_password_control = browser.getControl( | ||
500 | 1416 | name="field.confirm_password.%d" % registry_credentials_id) | ||
501 | 1417 | confirm_password_control.value = 'third_newpassword' | ||
502 | 1418 | browser.getControl("Save").click() | ||
503 | 1419 | with person_logged_in(self.user): | ||
504 | 1420 | self.assertThat( | ||
505 | 1421 | registry_credentials.getCredentials(), | ||
506 | 1422 | MatchesDict( | ||
507 | 1423 | {"username": Equals("third_different_username"), | ||
508 | 1424 | "password": Equals("third_newpassword")})) | ||
509 | 1425 | self.assertEqual(third_url, registry_credentials.url) | ||
510 | 1426 | |||
511 | 1427 | def test_add_oci_registry_creds_on_person_page(self): | ||
512 | 1428 | url = unicode(self.factory.getUniqueURL()) | ||
513 | 1429 | credentials = {'username': 'foo', 'password': 'bar'} | ||
514 | 1430 | image_name = self.factory.getUniqueUnicode() | ||
515 | 1431 | registry_credentials = getUtility(IOCIRegistryCredentialsSet).new( | ||
516 | 1432 | owner=self.user, | ||
517 | 1433 | url=url, | ||
518 | 1434 | credentials=credentials) | ||
519 | 1435 | getUtility(IOCIPushRuleSet).new( | ||
520 | 1436 | recipe=self.recipe, | ||
521 | 1437 | registry_credentials=registry_credentials, | ||
522 | 1438 | image_name=image_name) | ||
523 | 1439 | |||
524 | 1440 | browser = self.getViewBrowser( | ||
525 | 1441 | self.user, view_name='+oci-registry-credentials', user=self.user) | ||
526 | 1442 | browser.getLink("Edit OCI registry credentials").click() | ||
527 | 1443 | |||
528 | 1444 | browser.getControl(name="field.add_url").value = url | ||
529 | 1445 | browser.getControl(name="field.add_username").value = "new_username" | ||
530 | 1446 | browser.getControl(name="field.add_password").value = "password" | ||
531 | 1447 | browser.getControl( | ||
532 | 1448 | name="field.add_confirm_password").value = "password" | ||
533 | 1449 | browser.getControl("Save").click() | ||
534 | 1450 | |||
535 | 1451 | with person_logged_in(self.user): | ||
536 | 1452 | creds = list(getUtility( | ||
537 | 1453 | IOCIRegistryCredentialsSet).findByOwner( | ||
538 | 1454 | self.user)) | ||
539 | 1455 | self.assertEqual(url, creds[1].url) | ||
540 | 1456 | self.assertThat( | ||
541 | 1457 | removeSecurityProxy(creds[1]).getCredentials(), | ||
542 | 1458 | MatchesDict({"username": Equals("new_username"), | ||
543 | 1459 | "password": Equals("password")})) | ||
544 | 1460 | |||
545 | 1461 | def test_delete_oci_registry_creds_on_person_page(self): | ||
546 | 1462 | # Test that we do not delete creds when there are | ||
547 | 1463 | # push rules defined to use them | ||
548 | 1464 | url = unicode(self.factory.getUniqueURL()) | ||
549 | 1465 | credentials = {'username': 'foo', 'password': 'bar'} | ||
550 | 1466 | registry_credentials = getUtility(IOCIRegistryCredentialsSet).new( | ||
551 | 1467 | owner=self.person, | ||
552 | 1468 | url=url, | ||
553 | 1469 | credentials=credentials) | ||
554 | 1470 | IStore(registry_credentials).flush() | ||
555 | 1471 | registry_credentials_id = removeSecurityProxy(registry_credentials).id | ||
556 | 1472 | image_name = self.factory.getUniqueUnicode() | ||
557 | 1473 | push_rule = getUtility(IOCIPushRuleSet).new( | ||
558 | 1474 | recipe=self.recipe, | ||
559 | 1475 | registry_credentials=registry_credentials, | ||
560 | 1476 | image_name=image_name) | ||
561 | 1477 | |||
562 | 1478 | browser = self.getViewBrowser( | ||
563 | 1479 | self.person, view_name='+oci-registry-credentials', | ||
564 | 1480 | user=self.person) | ||
565 | 1481 | browser.getLink("Edit OCI registry credentials").click() | ||
566 | 1482 | # assert full rule is displayed | ||
567 | 1483 | self.assertEqual(url, browser.getControl( | ||
568 | 1484 | name="field.url.%d" % registry_credentials_id).value) | ||
569 | 1485 | self.assertEqual(credentials.get('username'), browser.getControl( | ||
570 | 1486 | name="field.username.%d" % registry_credentials_id).value) | ||
571 | 1487 | |||
572 | 1488 | # mark one line of credentials for delete | ||
573 | 1489 | delete_control = browser.getControl( | ||
574 | 1490 | name="field.delete.%d" % registry_credentials_id) | ||
575 | 1491 | delete_control.getControl('Delete').selected = True | ||
576 | 1492 | browser.getControl("Save").click() | ||
577 | 1493 | self.assertIn("These credentials cannot be deleted as there are " | ||
578 | 1494 | "push rules defined that still use them.", | ||
579 | 1495 | browser.contents) | ||
580 | 1496 | |||
581 | 1497 | # make sure we don't have any push rules defined to use | ||
582 | 1498 | # the credentials we want to remove | ||
583 | 1499 | with person_logged_in(self.person): | ||
584 | 1500 | removeSecurityProxy(push_rule).destroySelf() | ||
585 | 1501 | |||
586 | 1502 | delete_control = browser.getControl( | ||
587 | 1503 | name="field.delete.%d" % registry_credentials_id) | ||
588 | 1504 | delete_control.getControl('Delete').selected = True | ||
589 | 1505 | browser.getControl("Save").click() | ||
590 | 1506 | credentials_set = getUtility(IOCIRegistryCredentialsSet) | ||
591 | 1507 | with person_logged_in(self.person): | ||
592 | 1508 | self.assertEqual( | ||
593 | 1509 | 0, credentials_set.findByOwner(self.person).count()) | ||
594 | 1510 | |||
595 | 1353 | 1511 | ||
596 | 1354 | class TestPersonLiveFSView(BrowserTestCase): | 1512 | class TestPersonLiveFSView(BrowserTestCase): |
597 | 1355 | layer = DatabaseFunctionalLayer | 1513 | layer = DatabaseFunctionalLayer |
598 | diff --git a/lib/lp/registry/templates/person-edit-ociregistrycredentials.pt b/lib/lp/registry/templates/person-edit-ociregistrycredentials.pt | |||
599 | 1356 | new file mode 100644 | 1514 | new file mode 100644 |
600 | index 0000000..89483da | |||
601 | --- /dev/null | |||
602 | +++ b/lib/lp/registry/templates/person-edit-ociregistrycredentials.pt | |||
603 | @@ -0,0 +1,59 @@ | |||
604 | 1 | <html | ||
605 | 2 | xmlns="http://www.w3.org/1999/xhtml" | ||
606 | 3 | xmlns:tal="http://xml.zope.org/namespaces/tal" | ||
607 | 4 | xmlns:metal="http://xml.zope.org/namespaces/metal" | ||
608 | 5 | xmlns:i18n="http://xml.zope.org/namespaces/i18n" | ||
609 | 6 | metal:use-macro="view/macro:page/main_only" | ||
610 | 7 | i18n:domain="launchpad"> | ||
611 | 8 | <body> | ||
612 | 9 | |||
613 | 10 | <div metal:fill-slot="main"> | ||
614 | 11 | <div metal:use-macro="context/@@launchpad_form/form"> | ||
615 | 12 | <metal:formbody fill-slot="widgets"> | ||
616 | 13 | |||
617 | 14 | <table class="form"> | ||
618 | 15 | <tr tal:repeat="credentials view/oci_registry_credentials"> | ||
619 | 16 | <tal:credentials_widgets | ||
620 | 17 | define="credentials_widgets python:view.getCredentialsWidgets(credentials); | ||
621 | 18 | parity python:'even' if repeat['credentials'].even() else 'odd'"> | ||
622 | 19 | <td tal:define="widget nocall:credentials_widgets/url"> | ||
623 | 20 | <metal:widget use-macro="context/@@launchpad_form/widget_div" /> | ||
624 | 21 | </td> | ||
625 | 22 | <td tal:define="widget nocall:credentials_widgets/owner"> | ||
626 | 23 | <metal:widget use-macro="context/@@launchpad_form/widget_div" /> | ||
627 | 24 | </td> | ||
628 | 25 | <td tal:define="widget nocall:credentials_widgets/username"> | ||
629 | 26 | <metal:widget use-macro="context/@@launchpad_form/widget_div" /> | ||
630 | 27 | </td> | ||
631 | 28 | <td tal:define="widget nocall:credentials_widgets/password"> | ||
632 | 29 | <metal:widget use-macro="context/@@launchpad_form/widget_div" /> | ||
633 | 30 | </td> | ||
634 | 31 | <td tal:define="widget nocall:credentials_widgets/confirm_password"> | ||
635 | 32 | <metal:widget use-macro="context/@@launchpad_form/widget_div" /> | ||
636 | 33 | </td> | ||
637 | 34 | <td tal:define="widget nocall:credentials_widgets/delete"> | ||
638 | 35 | <metal:widget use-macro="context/@@launchpad_form/widget_div" /> | ||
639 | 36 | </td> | ||
640 | 37 | </tal:credentials_widgets> | ||
641 | 38 | </tr> | ||
642 | 39 | <tr> | ||
643 | 40 | <td tal:define="widget nocall:view/widgets/add_url"> | ||
644 | 41 | <metal:widget use-macro="context/@@launchpad_form/widget_div" /> | ||
645 | 42 | </td> | ||
646 | 43 | <td tal:define="widget nocall:view/widgets/add_username"> | ||
647 | 44 | <metal:widget use-macro="context/@@launchpad_form/widget_div" /> | ||
648 | 45 | </td> | ||
649 | 46 | <td tal:define="widget nocall:view/widgets/add_password"> | ||
650 | 47 | <metal:widget use-macro="context/@@launchpad_form/widget_div" /> | ||
651 | 48 | </td> | ||
652 | 49 | <td tal:define="widget nocall:view/widgets/add_confirm_password"> | ||
653 | 50 | <metal:widget use-macro="context/@@launchpad_form/widget_div" /> | ||
654 | 51 | </td> | ||
655 | 52 | </tr> | ||
656 | 53 | </table> | ||
657 | 54 | |||
658 | 55 | </metal:formbody> | ||
659 | 56 | </div> | ||
660 | 57 | </div> | ||
661 | 58 | </body> | ||
662 | 59 | </html> | ||
663 | diff --git a/lib/lp/registry/templates/person-ociregistrycredentials.pt b/lib/lp/registry/templates/person-ociregistrycredentials.pt | |||
664 | index 612056e..bec71e1 100644 | |||
665 | --- a/lib/lp/registry/templates/person-ociregistrycredentials.pt | |||
666 | +++ b/lib/lp/registry/templates/person-ociregistrycredentials.pt | |||
667 | @@ -12,6 +12,10 @@ | |||
668 | 12 | <span tal:replace="context/title"/> | 12 | <span tal:replace="context/title"/> |
669 | 13 | has not set any credentials yet. | 13 | has not set any credentials yet. |
670 | 14 | </p> | 14 | </p> |
671 | 15 | <p condition="context/required:launchpad.Edit"> | ||
672 | 16 | <a class="sprite edit" tal:attributes="href context/fmt:url/+edit-oci-registry-credentials">Edit OCI registry credentials</a> | ||
673 | 17 | </p> | ||
674 | 18 | |||
675 | 15 | <table id="oci-credentials" class="listing" tal:condition="view/has_credentials"> | 19 | <table id="oci-credentials" class="listing" tal:condition="view/has_credentials"> |
676 | 16 | <thead> | 20 | <thead> |
677 | 17 | <tr> | 21 | <tr> |
Really good job!
I've just added a few comments, mostly about coding style that could improve a bit code readability.