737018 - Add balrog admin-ui to add a new release

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Erick Dransch [:edransch]]

The Balrog admin-ui should expose a method to add new releases to the releases table.

Comment 1

[Erick Dransch [:edransch]]

Attachment: Preliminary Patch

There are some issues with file validation in this patch.
I'd like to call flaskwtf's 'file_required' but it doesn't seem to be working as intended.
There have been recent changes to flaskwtf's implementation ( https://github.com/rduplain/flask-wtf/commit/1c4e9d57fb794127fd005de70649042daef463da#L0R30 )

However, when I try to update flaskwtf, numerous other things break.

I'm not sure what the best workaround is. I suspect it might take a day or two to get everything updated & tested and I'm running a little short on time.

Comment 2

[Erick Dransch [:edransch]]

Attachment: Preliminary Patch

Missing file from previous patch.
Still missing some tests.

Comment 3

[[e-mail] (:bhearsum)]

Comment on attachment 618107 [details] [diff] [review]
Preliminary Patch

Review of attachment 618107 [details] [diff] [review]:
-----------------------------------------------------------------

::: auslib/web/templates/fragments/release_row.html
@@ +1,1 @@
> +<tr>

Woo! Thanks for factoring this out!

::: auslib/web/views/forms.py
@@ +35,2 @@
>              self.data = {}
> +    return process_formdata

Hmmm, I'm not sure both of the JSON field's should be sharing all of this logic. They both need to catch and convert the ValueError that comes up with loading the JSON, but JSONBlobFileField should also be calling .validate() on the blob, and default to an empty ReleaseBlobV1 object instead of a dict.

It might be better to duplicate the try/except block rather than try to factor it out - it's really only a few lines of shared code here.

::: auslib/web/views/releases.py
@@ +22,5 @@
>          locale = db.releases.getLocale(release, platform, locale)
>          return jsonify(locale)
>  
>      @requirelogin
> +    @requirepermission('/releases/:name/builds/:platform/:locale', options=[])

Good catch on the options here.

@@ +91,5 @@
> +        form = NewReleaseForm(prefix="new_release")
> +        return render_template('releases.html', releases=releases, addForm=form)
> +
> +class SingleBlobView(AdminView):
> +    """ /releases/[release]/blob"""

Naming nit: this is called 'data' in the db, so we should call it data here too.

@@ +93,5 @@
> +
> +class SingleBlobView(AdminView):
> +    """ /releases/[release]/blob"""
> +    def get(self, release):
> +        release_blob = retry(db.releases.getReleaseBlob, sleeptime=5, retry_exceptions=(SQLAlchemyError,),

Keep this as 'release_blob' though. I've been trying to follow the convention of blob being a Blob class, and data being a plain dict.

Comment 4

[Erick Dransch [:edransch]]

Attachment: Address Feedback

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 619051 [details] [diff] [review]
Address Feedback

Review of attachment 619051 [details] [diff] [review]:
-----------------------------------------------------------------

There's two tiny things noted below that I fixed on check-in.

::: auslib/test/web/views/test_releases.py
@@ +244,5 @@
> +        ret = self._get("/releases/d")
> +        self.assertStatusCode(ret, 200)
> +        self.assertTrue("<td> <a href='releases/d/data'>link</a></td>" in ret.data, msg=ret.data)
> +
> +    def testNewReleasePost(self):

s/Post/Put/

::: auslib/web/views/forms.py
@@ +19,5 @@
>          if self.disabled:
>              kwargs['disabled'] = 'disabled'
>          return TextInput.__call__(self, *args, **kwargs)
>  
> +class JSONFieldMixin():

You should inherit from 'object' here, to use new style classes.

Comment 6

[[e-mail] (:bhearsum)]

Is there anything left to do here?

Comment 7

[Peter Bengtsson [:peterbe]]

I can confirm that it IS possible to add new releases. I've done it when testing the UI fixes I added. Resolve?

Comment 8

[[e-mail] (:bhearsum)]

I verified this too when testing bug 748435. Hooray!