1097661 - Update ship-it to add the capability to tag a release as shipped or not

Status: RESOLVED FIXED

(Release Engineering :: Release Automation, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Necessary to implement bug 1083718, we need add the information that we shipped a build or not.

It is common that we do several builds for a release and not always the last one it used...

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Bug-1097661-Update-ship-it-to-add-the-capability-to-.patch

Comment 2

[[e-mail] (:bhearsum)]

Comment on attachment 8521404 [details] [diff] [review]
0001-Bug-1097661-Update-ship-it-to-add-the-capability-to-.patch

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

::: kickoff/__init__.py
@@ +56,4 @@
>  app.add_url_rule('/releases/<releaseName>/l10n', view_func=ReleaseL10nAPI.as_view('release_l10n_api'), methods=['GET'])
>  app.add_url_rule('/releases/<releaseName>/status', view_func=StatusAPI.as_view('status_api'), methods=['GET', 'POST'])
>  app.add_url_rule('/releases/<releaseName>/comment', view_func=ReleaseCommentAPI.as_view('comment_api'), methods=['GET'])
> +app.add_url_rule('/releasesShipped/<releaseName>/', view_func=ReleasesShippedAPI.as_view('releases_api'), methods=['GET'])

Being shipped or not is an attribute of a release, you shouldn't need a special endpoint for this because we already have /releases/<releaseName>. More on that below.

::: kickoff/model.py
@@ +31,4 @@
>      enUSPlatforms = db.Column(db.String(500), default=None, nullable=True)
>      comment = db.Column(db.Text, default=None, nullable=True)
>      starter = db.Column(db.String(250), nullable=True)
> +    shipped = db.Column(db.Boolean(), nullable=False, default=False)

This is something that should be part of "status" - it doesn't need its own column. IIRC, Zeller and I decided that when a release's status is "postrelease", it can be considered shipped. You could also add a new state if you'd like, though. Doing it this way also means that you don't need any new endpoints - you can POST to existing /releases/<releaseName> endpoint to update the status. If you need to query for only shipped releases, that should be done through /releases, with some code added to filter by shipped or not if it can't already do that.

::: kickoff/templates/includes/releases_reviewed.html
@@ +1,2 @@
> +<script type='text/javascript'>
> +function updateShip(e) {

This needs to be changed in accordance with the above comments, but please also move it to kickoff.js or another external js file to keep things organized.

@@ +42,4 @@
>    <th>Partial Versions</th>
>    <th>Update Prompt Wait Time</th>
>    <th>Comment</th>
> +  <th>Shipped</th>

nit: "Shipped?" would be better here, since it implies boolean.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Bug-1097661-Update-ship-it-to-add-the-capability-to-.patch

I tried but I failed.

I am trying to use ajax to avoid any useless refresh.
However, Ajax + CSRF is only available in flask wtf from version 0.9 (and we use 0.8). http://flask-wtf.readthedocs.org/en/latest/csrf.html

The attached patch is implementing:
* Use ajax post
* points to /releases/<releaseName> instead of /releasesShipped/<releaseName>/

However, it is failing on 
ReleaseAPIForm.validate because of "CSRF token missing"

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 8525346 [details] [diff] [review]
0001-Bug-1097661-Update-ship-it-to-add-the-capability-to-.patch

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

::: kickoff/templates/includes/releases_reviewed.html
@@ +6,5 @@
> +<!--         if (!/^(GET|HEAD|OPTIONS|TRACE)$/i.test(settings.type) && !this.crossDomain) { -->
> +<!--             xhr.setRequestHeader("X-CSRFToken", csrftoken) -->
> +<!--         } -->
> +<!--     } -->
> +<!-- }) -->

I'm pretty sure that reason this doesn't work is because the csrf token doesn't exist anywhere in this page, so the $() call on line 2 doesn't find anything. You're going to need to store it somewhere when you build this page up. The best bet is probably to add a <form>, and then call hidden_tag like we do for other forms.

I'd suggesting avoiding the magic above too -- that's meant to be an app-wide thing. Since this isn't a real AJAX app, that could cause issues with other form submissions. Instead, you can just grab the csrf token inside of updateShip, and pass it in the request body (I think that's the "data" argument). You're also going to need to pass whatever you want the new value of "status" to be, otherwise the submission won't change update anything!

@@ +101,5 @@
>    {% endif %}
>    </td>
> +  <td>
> +      <div id="divshipped-{{rel.name|replace(".","-")}}" {% if rel.status != "postrelease" %} style="display: none;" {% endif %}  >Shipped</div>
> +      <div id="divnotshipped-{{rel.name|replace(".","-")}}" {% if rel.status == "postrelease" %} style="display: none;" {% endif %}>Not shipped</div>

I'm a bit confused about this - why is there two divs? It seems like the "if" block should be before the <div> creation.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Bug-1097661-Update-ship-it-to-add-the-capability-to-.patch

Sorry, top stuff was left to show what is recommended by upstream. I removed it in this patch.
I will give it a try to your form proposal but I still think that it is overkill/waste of time compared to my initial proposition...

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Bug-1097661-Update-ship-it-to-add-the-capability-to-.patch

More your taste?

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 8526141 [details] [diff] [review]
0001-Bug-1097661-Update-ship-it-to-add-the-capability-to-.patch

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

Yup, this looks right. I assume you've tested it? ;)

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Bug-1097661-Update-ship-it-to-add-the-capability-to-.patch

Moved to "Running/Complete"

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8532598 [details] [diff] [review]
0001-Bug-1097661-Update-ship-it-to-add-the-capability-to-.patch

http://git.mozilla.org/?p=build/release-kickoff.git;a=commit;h=cd12a36e63f4ef7f85673e845d20fd8ae0a72099

Comment 10

[[e-mail] (:bhearsum)]

I pushed this to prod this morning.