The default bug view has changed. See this FAQ.

Add REST API entry point to shipit that allows shipit-agent to enter release data into shipit database

RESOLVED FIXED

Status

Release Engineering
Release Automation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: zeller, Assigned: zeller, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [shipit])

Attachments

(2 attachments, 11 obsolete attachments)

(Assignee)

Description

3 years ago
Shipit needs a new entry point REST API at /status/<release-name> for POST messages. The primary use case here is going to be for shipit-agent (bug 1032978) to call the REST API to have release information from pulse entered into the shipit database at the new table(s) (bug 1032975)
(Assignee)

Updated

3 years ago
Assignee: nobody → johnlzeller
Blocks: 826753
Depends on: 1032975, 1032978
Whiteboard: [shipit]
(Assignee)

Comment 1

3 years ago
Created attachment 8448967 [details]
releasedata.sql

I have attached a dump of the schema and 640 release messages from pulse.m.o that *should* represent most, if not all, of the Firefox-31-0b5-build1 release. Please use this dump with "cat releasedata.sql | sqlite3 releasedata.sqlite" to view the messages as rows in a database. It'll help give the questions I have below more context.

Some questions regarding the filtering of messages from pulse.m.o:

 * Should I be filtering for all messages where routing_key = "%release%"? OR, should I be filtering where routing_key = "build.release.%"? OR, some other field like key?
   - It seems like msgs where routing_key contains *release* are related to a release, obviously. But, there are a bunch of additional prefixes that I am not sure how to categorize (unittest.release.*, talos.release.*, etc)
 * I am currently attempting to use the fields product + version + build_number to construct the release name (ie Firefox-31.0b5-build1), but many messages have None in the version and build_number fields. Additionally, sometimes it seems like unrelated products show up like "mobile" or "xulrunner", when I am pretty certain only "Firefox" was running. I could be mistaken... Any advice on what to do with the messages that have None or don't fit the product name?
 * Is there a solid set of messages I should look at to determine each of the following status': Tagging completed, All builds/repacks completed, Updates on betatest, Ready for releasetest, Ready for release, Postrelease? (status steps taken from bug 826753 comment 0

Some of these questions may answer one another, but this was an attempt to put all the unanswered questions out there.
Flags: needinfo?(rail)
Flags: needinfo?(bhearsum)
(In reply to John Zeller [:zeller] from comment #1)
> Created attachment 8448967 [details]
> releasedata.sql
> 
> I have attached a dump of the schema and 640 release messages from pulse.m.o
> that *should* represent most, if not all, of the Firefox-31-0b5-build1
> release. Please use this dump with "cat releasedata.sql | sqlite3
> releasedata.sqlite" to view the messages as rows in a database. It'll help
> give the questions I have below more context.
> 
> Some questions regarding the filtering of messages from pulse.m.o:
> 
>  * Should I be filtering for all messages where routing_key = "%release%"?
> OR, should I be filtering where routing_key = "build.release.%"? OR, some
> other field like key?

I'm not a pulse expert, but "build.release.%" (or "build.release-%"?) sounds better.

>    - It seems like msgs where routing_key contains *release* are related to
> a release, obviously. But, there are a bunch of additional prefixes that I
> am not sure how to categorize (unittest.release.*, talos.release.*, etc)

You can ignore those for now, they are tests we don't usually look at.

>  * I am currently attempting to use the fields product + version +
> build_number to construct the release name (ie Firefox-31.0b5-build1), but
> many messages have None in the version and build_number fields.

There was a bug for that... You can ignore those, they are tests related.

> Additionally, sometimes it seems like unrelated products show up like
> "mobile" or "xulrunner", when I am pretty certain only "Firefox" was
> running. I could be mistaken... Any advice on what to do with the messages
> that have None or don't fit the product name?

Firefox implicitly runs xulrunner jobs, but you can ignore them.

"mobile" is Fennec, but not sure what to say without context. Do you have an example message?

>  * Is there a solid set of messages I should look at to determine each of
> the following status': Tagging completed, All builds/repacks completed,
> Updates on betatest, Ready for releasetest, Ready for release, Postrelease?

What do you mean by "solid"? :)

We have these templates we use to send emails: http://hg.mozilla.org/build/buildbot-configs/file/d93469f2073b/mozilla/release_templates

And here how we construct the names of templates to be used:
http://hg.mozilla.org/build/buildbotcustom/file/006d12abfa59/process/release.py#l182

Some examples for mozilla-beta (builder names):

* Tagging completed: release-mozilla-beta-firefox_tag 
* All builds/repacks completed: I don't think we have a builder for this, we use AggregatingScheduler with multiple upstreams and multiple downstreams when all deliverables are ready: http://hg.mozilla.org/build/buildbotcustom/file/006d12abfa59/process/release.py#l1761
We can add a dummy builder so you can watch its status, shouldn't be hard.

* Updates on betatest: release-mozilla-beta-updates
* Ready for releasetest: release-mozilla-beta-ready_for_releasetest_testing
* Ready for release: release-mozilla-beta-ready_for_release
* Postrelease: release-mozilla-beta-postrelease

You can look at the builders filtered by category:
http://buildbot-master71.srv.releng.use1.mozilla.com:8001/waterfall?category=release-mozilla-beta-

> (status steps taken from bug 826753 comment 0
> 
> Some of these questions may answer one another, but this was an attempt to
> put all the unanswered questions out there.

I hope it helps
Flags: needinfo?(rail)
(Assignee)

Updated

3 years ago
Flags: needinfo?(bhearsum)
(Assignee)

Comment 3

3 years ago
Created attachment 8466035 [details] [diff] [review]
bug1032985.patch

In an effort to keep this progress transparent, I am gonna go ahead and post some more changes for feedback :) These are not complete, but they are a first pass at rewriting model.py and views/status.py. Specifically, the changes here apply to the REST API endpoint /status/<release-name>
Attachment #8466035 - Flags: feedback?(bhearsum)
(Assignee)

Updated

3 years ago
Attachment #8466035 - Flags: feedback?(bhearsum)
(Assignee)

Comment 4

3 years ago
Created attachment 8469543 [details] [diff] [review]
bug1032985.patch

This patch adds the /status/<release-name> REST API endpoint to shipit. This patch shows the get and post functionality as well. Model helper functions may be subject to change a bit more before a review because they are mostly used for the other new endpoints.
Attachment #8466035 - Attachment is obsolete: true
Attachment #8469543 - Flags: review?(bhearsum)
(Assignee)

Comment 5

3 years ago
Created attachment 8470395 [details] [diff] [review]
bug1032985.patch

Did some testing and fixed some errors. An outstanding issue is converting the 'sent' value into a Datetime.Datetime object. The issue lies in that the Datetime.strftime() function won't parse out timezone... the docs suggest that %z will do it, but it is for generating a string, not parsing iiuc. The temporary solution here is to look for a '+' or a '-' in the last 6 characters of the 'sent' string, and if it is there, then split it off, discarding it. I'd like to do this in a nicer way, but it works as is. The 'sent' string looks like '2014-08-08T16:12:59+01:00'
Attachment #8469543 - Attachment is obsolete: true
Attachment #8469543 - Flags: review?(bhearsum)
Attachment #8470395 - Flags: review?(bhearsum)
(Assignee)

Comment 6

3 years ago
Created attachment 8471272 [details] [diff] [review]
bug1032985.patch

Made changes after testing with the mock pulse instance. Changed createFromRequest() to use .get() so that None can be entered as a value, rather than failing on KeyError.

Unit tests for this can come next.

One question: May need to add default=None to the platform Column in model.py, which makes me wonder, what do you think of having a default set on results and/or anything else nullable?
Attachment #8470395 - Attachment is obsolete: true
Attachment #8470395 - Flags: review?(bhearsum)
Attachment #8471272 - Flags: review?(bhearsum)
Comment on attachment 8471272 [details] [diff] [review]
bug1032985.patch

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

I know there's a lot of feedback below, but this is still a fine start. Sorry I didn't look at an earlier version of the patch to catch some of this sooner.

::: kickoff/__init__.py
@@ +49,4 @@
>  app.add_url_rule('/releases', view_func=ReleasesAPI.as_view('releases_api'), methods=['GET'])
>  app.add_url_rule('/releases/<releaseName>', view_func=ReleaseAPI.as_view('release_api'), methods=['GET', 'POST'])
>  app.add_url_rule('/releases/<releaseName>/l10n', view_func=ReleaseL10nAPI.as_view('release_l10n_api'), methods=['GET'])
> +app.add_url_rule('/status/<releaseName>', view_func=StatusAPI.as_view('status_api'), methods=['GET', 'POST'])

Status is a property of a release, so it should be a child of it. Eg: /releases/<releaseName>/status.

::: kickoff/model.py
@@ +238,5 @@
> +        #      solution it to just cut it off and remove it completely
> +        if '+' in form['sent'][-6:] or '-' in form['sent'][-6:]:
> +            form['sent'] = datetime.strptime(form['sent'][:-6], "%Y-%m-%dT%H:%M:%S")
> +        else:
> +            form['sent'] = datetime.strptime(form['sent'], "%Y-%m-%dT%H:%M:%S")

Yeah, don't do this. You're going to start getting off-by-1h issues when we hit daylight savings, if not already. By the looks of it, it seems that "sent" comes in as an ISO8601 string (I'm not sure why you're trying to handle both cases - I'd be very surprised if it can come in in multiple different formats). It's almost always best to store dates and times in UTC, and translate them to whatever timezone you need later on. You can see example of this in how we handle submittedAt:
http://git.mozilla.org/?p=build/release-kickoff.git;a=blob;f=kickoff/model.py;h=c2f9f404d2633b8cadc7ced32a03d82d2c8e0c75;hb=HEAD#l33
http://git.mozilla.org/?p=build/release-kickoff.git;a=blob;f=kickoff/static/kickoff.js;h=421c7a8980d47d237b390dd9632b321267ea9f04;hb=HEAD#l50

I think the best thing to do here is actually assume that "sent" comes in as an UTC ISO8601 string, and update ship it notifier to do that translation for you. Because it's the thing talking directly to Pulse, it's the most appropriate place to translate from P[SD]T to UTC. The third answer in http://stackoverflow.com/questions/969285/how-do-i-translate-a-iso-8601-datetime-string-into-a-python-datetime-object should help you with that translation.

You probably also want a @hybrid_property for this, like we have for submittedAt.

I know this all very confusing, so dfeel free to ping me if it's not clear.

@@ +247,5 @@
>      def __repr__(self):
>          return '<ReleaseEvents %r>' % self.name
> +
> +
> +def getEvents(group=None):

All of these methods that query ReleaseEvents can just be methods on the class. The only reason that getReleases is not, is because it operates on multiple tables.

@@ +257,5 @@
> +        for r in ReleaseEvents.query.filter_by(**filters):
> +            events.append(r)
> +    else:
> +        for r in ReleaseEvents.query.all():
> +            events.append(r)

There's no reason to create an "events" list here. If you're trying to be memory efficient, you should be yielding "r" instead of appending it. However, I think you're best off just dropping the loops and returning the results of filter_by() and all() directly.

@@ +274,5 @@
> +    return status
> +
> +
> +def tagStatus(name):
> +    tag_events = ReleaseEvents.query.filter_by(name=name, group='tag')

For tag and any other event group that that is binary, I think you can simply this by just looking for .count() > 0. Eg:
if ReleaseEvents.query.filter_by(name=name, group="tag").count() > 0:
  return {"complete": True, progress=1}
else:
  return {"complete": False, progress=0}

I also think that "complete" is unnecessary here. AFAICT, progress being 1 implies complete, so I think "complete" is unnecessary. That may help simplify some of the code in other methods, too.

@@ +289,5 @@
> +    build_events = ReleaseEvents.query.filter_by(name=name, group='build')
> +
> +    builds = {'complete': False, 'platforms': {}, 'progress': 0.00}
> +    for platform in getEnUSPlatforms(name):
> +        builds['platforms'][platform] = {'complete': False, 'chunks': {'num': 0, 'total': 0}, 'progress': 0.00}

Why are there chunks here? Each platform is binary - there should be no need for this. AFAIK, you should only need something like this:
{
  "progress": 0.0,
  "platforms": {
    "foo": True,
    "bar": False,
  }
}

You can continue to use a subdict per platform if you like, but it will probably make computing the overall progress more difficult.

@@ +303,5 @@
> +    for key, build in builds['platforms'].items():
> +        total += build['chunks']['total']
> +        num += build['chunks']['num']
> +    try:
> +        builds['progress'] = num / total

You can calculate this by comparing len(builds["platforms"]) to builds["platforms"].values().count(True).

@@ +326,5 @@
> +            repacks['platforms'][repack.platform]['chunks']['total'] = repack.chunkTotal
> +            repacks['platforms'][repack.platform]['chunks']['num'] += 1
> +            repacks['platforms'][repack.platform]['progress'] += (1.00/repack.chunkTotal) * 1.00
> +        else:
> +            repacks['platforms'][repack.platform]['complete'] = True

Since complete is going to be dropped, this can be updated to just set progress to 1.0

@@ +365,5 @@
> +    for releasetest in releasetest_events:
> +        if releasetest.name:
> +            releasetests['complete'] = True
> +            releasetests['progress'] = 1.00
> +    return releasetests

After seeing a few of these binary methods, it looks to me like they could be easily factored out. The only thing different about each of them is the group AFAICT.

@@ +368,5 @@
> +            releasetests['progress'] = 1.00
> +    return releasetests
> +
> +
> +def releaseStatus(name):

"releaseStatus" is an ambigious name for this - it sounds like it's talking about overall release status. I think you should call this "readyForReleaseStatus" instead.

@@ +416,5 @@
> +
> +
> +def getEnUSPlatforms(name):
> +    release_tables = {'Firefox': FirefoxRelease, 'Fennec': FennecRelease,
> +                      'Thunderbird': ThunderbirdRelease}

There's already a helper function for this (getReleaseTable) - use it instead.

::: kickoff/views/status.py
@@ +24,5 @@
> +    def get(self, releaseName):
> +        events = {'release': releaseName, 'events': [], 'status': {}}
> +        rows = ReleaseEvents.query.filter_by(name=releaseName)
> +        for row in rows:
> +            events['events'].append(row.toDict())

Is there a specific reason you're returning every single individual event? It seems like it's not necessary when you've gone to the trouble of summarizing it all in "status". At the very least, this should be optional.

@@ +34,5 @@
> +        releaseEventsUpdate = {}
> +        for field, value in request.form.items():
> +            releaseEventsUpdate[field] = value
> +        releaseEventsUpdate['name'] = releaseName
> +        releaseEventsUpdate.pop('csrf_token')

This should be unnecessary. Once you have a proper Form for this, all the necessary fields should be there. You probably want to update ReleaseEvents.createFromRequest to be ReleaseEvents.createFromForm, too. It also won't complain if csrf_token is still in there.

@@ +52,5 @@
> +        # TODO: Validate may not be necessary
> +        validity = True
> +        if not validity:
> +            cef_event('User Input Failed', CEF_INFO)
> +            return Response(status=400, response="Error in data")

You almost certainly want to use a Form to do this. Hopefully the existing ones in views/forms.py will be a good example. You should probably be doing this before creating the ReleaseEvents object, too.

@@ +63,5 @@
> +                   format(releaseEventsUpdate.name, releaseEventsUpdate.event_name)
> +            log.error('{}'.format(msg))
> +            cef_event('User Input Failed', CEF_INFO,
> +                      ReleaseName=releaseEventsUpdate.name)
> +            return Response(status=400, response=msg)

You should do this before creating the object, too -- basically, you should do validation as early as possible.
Attachment #8471272 - Flags: review?(bhearsum) → feedback+
(Assignee)

Comment 8

3 years ago
Created attachment 8472142 [details] [diff] [review]
bug1032985.patch

I am still adding the form component of this, and the timestamp parsing hasn't changed, but I have made the rest of these changes. So I will throw this in and get the rest of the changes in early tomorrow.
Attachment #8471272 - Attachment is obsolete: true
Attachment #8472142 - Flags: feedback?(bhearsum)
Comment on attachment 8472142 [details] [diff] [review]
bug1032985.patch

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

::: kickoff/model.py
@@ +247,5 @@
>      def __repr__(self):
>          return '<ReleaseEvents %r>' % self.name
> +
> +
> +    def getEvents(group=None):

Did you test this? I don't think this code will compile without a "self" argument.
Attachment #8472142 - Flags: feedback?(bhearsum)
(Assignee)

Comment 10

3 years ago
Created attachment 8472833 [details] [diff] [review]
bug1032985.patch

Yeah, you're right, I hadn't tested the previous patch :) I was mostly posting to keep things flowing while I was testing.

Anyway, here is the next version. I am having some trouble with the generators for converting sent to datetime to use the isoformat function of pytz. The sent value looks like this when coming in now '2014-08-08T15:12:59' (shipit-notifier is handling the conversion of these into UTC).

Beyond that I am also getting the form working properly still.

I will continue getting these hammered out, but I think the rest of this is ready for any feedback you may have. I will try to get another patch up with the form and isoformat stuff setup correctly before 4pm EST, ideally.
Attachment #8472142 - Attachment is obsolete: true
Attachment #8472833 - Flags: feedback?(bhearsum)
Comment on attachment 8472833 [details] [diff] [review]
bug1032985.patch

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

This patch is very much improved, nice work!

(In reply to John Zeller [:zeller] from comment #10)
> Anyway, here is the next version. I am having some trouble with the
> generators for converting sent to datetime to use the isoformat function of
> pytz. The sent value looks like this when coming in now
> '2014-08-08T15:12:59' (shipit-notifier is handling the conversion of these
> into UTC).

Nice - this sounds like it's correct now.

::: kickoff/model.py
@@ +220,5 @@
> +        return pytz.utc.localize(self.sent).isoformat()
> +
> +    @sent.setter
> +    def sent(self, _sent):
> +        self.sent = datetime.strptime(_sent, "%Y-%m-%dT%H:%M:%S")

I think this is okay...

::: kickoff/views/forms.py
@@ +359,5 @@
> +
> +class ReleaseEventsAPIForm(Form):
> +    # TODO: Add default=0 to results?
> +    name = StringField('Name:', validators=[DataRequired('Name is required.'), Length(0, 100), Regexp(NAME_REGEX, message='Invalid name format.')])
> +    sent = StringField('Sent:', validators=[DataRequired('Sent is required.'), Length(19), Regexp(DATETIME_REGEX, message='Invalid sent format.')])

There's a DateTimeField for this - no need to do it yourself: http://wtforms.readthedocs.org/en/latest/fields.html#basic-fields

::: kickoff/views/status.py
@@ +23,5 @@
> +
> +    def get(self, releaseName):
> +        status = {'status': {}}
> +        status['status'] = ReleaseEvents.getStatus(releaseName)
> +        events = request.args.get('events', type=int)

You probably want type=bool instead.

@@ +34,5 @@
> +
> +    def post(self, releaseName):
> +        form = ReleaseEventsAPIForm()
> +        release_event = request.form.copy()
> +        release_event['name'] = releaseName

It's a bit weird to add things to the form. I think it would be better to just pass both name and the unmodified form to createFromForm instead.
Attachment #8472833 - Flags: feedback?(bhearsum) → feedback+
(Assignee)

Comment 12

3 years ago
Created attachment 8473490 [details] [diff] [review]
bug1032985.patch

Okay after a lot of fighting, I didn't get the generators working for ISO conversion without changing the model a bit. I mimicked what is done in the Release table, so sent becomes _sent with the first argument to Column being 'sent'. ISO seems to work now, but I am aware that the _sent change may need to be a different patch.

Form works well, I am manually checking the name in validate since name comes in on the URL and not in a request parameter, so it's never added to the form automatically.

I think that this is pretty much ready for production :)
Attachment #8472833 - Attachment is obsolete: true
Attachment #8473490 - Flags: review?(bhearsum)
(Assignee)

Comment 13

3 years ago
Created attachment 8473501 [details] [diff] [review]
bug1032985.patch

Previous comment still applies, but I can the randomized tests through our fake publisher and made some adjustments, mostly to how we are validating the release name from the REST URL. Good to go for review now :)
Attachment #8473490 - Attachment is obsolete: true
Attachment #8473490 - Flags: review?(bhearsum)
Attachment #8473501 - Flags: review?(bhearsum)
(Assignee)

Comment 14

3 years ago
Created attachment 8473504 [details] [diff] [review]
bug1032985.patch

Okay this is the last patch for the night, I just added results=0 to every status helper and fixed the tag helper. The results change is to make sure that we are only showing status from events that were successful.
Attachment #8473501 - Attachment is obsolete: true
Attachment #8473501 - Flags: review?(bhearsum)
Attachment #8473504 - Flags: review?(bhearsum)
Comment on attachment 8473504 [details] [diff] [review]
bug1032985.patch

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

I have to r- this, but it's really really close now.

::: kickoff/model.py
@@ +209,1 @@
>      event_name = db.Column(db.String(150), nullable=False, primary_key=True)

You'll need another change to the model to support multiple events. Right now, if you get an event with name="Firefox-32.0b5-build1" and event_name="release-mozilla-beta-firefox_tag" that has failed, the next one that comes in will hit an integrity error, because the key won't be unique.

@@ +240,2 @@
>          for c in self.__table__.columns:
> +            me[c.name] = str(getattr(self, c.name))

Why are you casting everything to a string here? That doesn't seem right, especially for _sent, which is a DateTime

::: kickoff/views/forms.py
@@ +358,5 @@
>          raise ValueError("Can't find release table for release %s" % release)
> +
> +
> +class ReleaseEventsAPIForm(Form):
> +    # TODO: Add default=0 to results?

You should probably make this a required field instead. There's no way to intelligently set a default, and it's a required column in the database, so it needs to be present.

@@ +374,5 @@
> +        if len(releaseName) < 1 or len(releaseName) > 100:
> +            valid = False
> +            if 'releaseName' not in self.errors:
> +                self.errors['releaseName'] = []
> +            self.errors['releaseName'].append('Release name too short or too long. Must be greater than 0 and less than 100.')

I hadn't considering doing this validation in here, but I think I like it - nice job!

::: kickoff/views/status.py
@@ +44,5 @@
> +                                                                 form.event_name.data))
> +            cef_event('User Input Failed', CEF_ALERT)
> +            return Response(status=400, response=e)
> +
> +        if not form.validate(releaseName, releaseEventsUpdate):

AFAICT, releaseEventsUpdate is never used as part of the validation - please remove it from this call. This also means you can move the above block down below the rest of the validation where it belongs - it should probably happen just above .add().
Attachment #8473504 - Flags: review?(bhearsum) → review-
(Assignee)

Comment 16

3 years ago
Created attachment 8474685 [details] [diff] [review]
bug1032985.patch
Attachment #8473504 - Attachment is obsolete: true
Attachment #8474685 - Flags: review?(bhearsum)
Comment on attachment 8474685 [details] [diff] [review]
bug1032985.patch

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

This patch looks fine, but I got confused when I went to land it. It appears that it's mostly landed already in http://git.mozilla.org/?p=build/release-kickoff.git;a=blobdiff;f=kickoff/model.py;h=6e822d8d27eb67ee1cbb9737ad805701e6f4961d;hp=44bf723c7f9e1f5b4d292d7b8b81f9607a411209;hb=c335cd541b6309a635ba3688a23910a1be7967b4;hpb=5833b50075ef229aa2382ded24b4f8a143f9908d and http://git.mozilla.org/?p=build/release-kickoff.git;a=blobdiff;f=kickoff/model.py;h=c2f9f404d2633b8cadc7ced32a03d82d2c8e0c75;hp=6e822d8d27eb67ee1cbb9737ad805701e6f4961d;hb=1f24d5c5528f4c39295047ee27a32d3ed338a642;hpb=c335cd541b6309a635ba3688a23910a1be7967b4

Can you please pull and attach the proper diff?
Attachment #8474685 - Flags: review?(bhearsum)
(Assignee)

Comment 18

3 years ago
Created attachment 8475319 [details] [diff] [review]
bug1032985.patch

This should land fine now
Attachment #8474685 - Attachment is obsolete: true
Attachment #8475319 - Flags: review?(bhearsum)
Comment on attachment 8475319 [details] [diff] [review]
bug1032985.patch

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

::: kickoff/views/forms.py
@@ +362,5 @@
> +class ReleaseEventsAPIForm(Form):
> +    sent = DateTimeField('Sent:', validators=[InputRequired('Sent is required.')])
> +    event_name = StringField('Event Name:', validators=[InputRequired('Event Name is required.'), Length(0, 150)])
> +    platform = StringField('Platform:')
> +    results = IntegerField('Results:', default=0, validators=[InputRequired('Results is required.'), NumberRange(min=0, max=0)])

This isn't quite what I was expecting here. This field is an illusion of choice - you're only allowed to enter 0! If you want that to be the only allowed value, drop it from the form and make it the column default in the model. However, I think what should be done here is just remove the NumberRange. The default will be correct, but other values will be accepted. Let me know if I'm overlooking something here. r=me with this fixed.
Attachment #8475319 - Flags: review?(bhearsum) → review-
(Assignee)

Comment 20

3 years ago
(In reply to Ben Hearsum [:bhearsum] from comment #19)
> Comment on attachment 8475319 [details] [diff] [review]
> bug1032985.patch
> 
> Review of attachment 8475319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: kickoff/views/forms.py
> @@ +362,5 @@
> > +class ReleaseEventsAPIForm(Form):
> > +    sent = DateTimeField('Sent:', validators=[InputRequired('Sent is required.')])
> > +    event_name = StringField('Event Name:', validators=[InputRequired('Event Name is required.'), Length(0, 150)])
> > +    platform = StringField('Platform:')
> > +    results = IntegerField('Results:', default=0, validators=[InputRequired('Results is required.'), NumberRange(min=0, max=0)])
> 
> This isn't quite what I was expecting here. This field is an illusion of
> choice - you're only allowed to enter 0! If you want that to be the only
> allowed value, drop it from the form and make it the column default in the
> model. However, I think what should be done here is just remove the
> NumberRange. The default will be correct, but other values will be accepted.
> Let me know if I'm overlooking something here. r=me with this fixed.

As it stands shipit-notifier isn't going to ever submit anything but results=0, but we also want to make sure to guard against anything setting results to anything but 0, otherwise we would run into a situation where results=0, but there is already an event entry for the same unique key.

It could be that forcing results to 0 here is overkill, but I figured it was a simple second defense against accidental entries where results!=0.

What do you think?
(Assignee)

Comment 21

3 years ago
Created attachment 8475426 [details] [diff] [review]
status update API for ship it

Removed NumberRange from forms
Attachment #8475319 - Attachment is obsolete: true
Attachment #8475426 - Flags: review?(bhearsum)
Attachment #8475426 - Flags: review?(bhearsum) → review+
Comment on attachment 8475426 [details] [diff] [review]
status update API for ship it

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

John landed this yesterday. I put it into production today.
Attachment #8475426 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Attachment #8475426 - Attachment description: bug1032985.patch → status update API for ship it
You need to log in before you can comment on or make changes to this bug.