Handle new beta versions with signing (bug 1160593) #555
Conversation
| file_ = File.objects.all().order_by("-created")[0] | ||
| assert self.addon.status == amo.STATUS_PUBLIC | ||
| assert file_.status == amo.STATUS_BETA | ||
| assert mock_sign_file.called |
There was a problem hiding this comment.
@magopian I'm not sure where this test should go. Under TestAddVersion or TestVersionAddFile? I'll get rid of one of the above.
There was a problem hiding this comment.
Or maybe both? One is a new version with the first file and the other is another file on the same version?
There was a problem hiding this comment.
I think it should be both
|
@magopian I'm struggling with this one. The combination of beta, waffle flags, statuses, etc is making my head spin. I think this might be close but would you mind reviewing and giving me a detailed list of what you think is left? Thanks. |
|
I'm not sure there's already a method to reset the form. And thinking about it, that's not what we want: if we reset the form, and resubmit the same file, it'll be detected as beta again (and the checkbox will be re-checked), so there would be no way to submit a file that shouldn't be beta (as long as it has a version that looks like a beta version). What we should do instead: if the checkbox is unchecked, remove the message and enable the submit button. Also, I think the message should be a bit more explicit about the fact that we don't accept beta versions that don't pass the automatic validation, something like:
|
| @@ -27,7 +27,8 @@ class SigningError(Exception): | |||
| def get_endpoint(file_obj): | |||
| """Get the endpoint to sign the file, depending on its review status.""" | |||
| server = settings.SIGNING_SERVER | |||
| if file_obj.version.addon.status != amo.STATUS_PUBLIC: | |||
| if (file_obj.status == amo.STATUS_BETA or | |||
| file_obj.version.addon.status != amo.STATUS_PUBLIC): | |||
There was a problem hiding this comment.
This is weird: the first line is redundant, right? If it's amo.STATUS_BETA it sure isn't amo.STATUS_PUBLIC. I think this change isn't needed.
There was a problem hiding this comment.
One is checking the file status, the other is checking the addon status. If the add-on isn't PUBLIC or the file is BETA, we sign with prelim. Right?
There was a problem hiding this comment.
You are completely right, my bad, and very well thought!
|
You are indeed nearly done I believe. The only thing missing apart the small nits I added is to have the toggling of the error message and the submit button if the beta checkbox is checked/unchecked. |
|
@magopian Ok, I've updated this. I may need help deciphering the tests. I still don't see why the status changes from PUBLIC to UNREVIEWED. But the mechanics of the tests should be correct. Mind taking a look? |
| the flag is enabled.""" | ||
| self.create_flag('automatic-validation') | ||
| version = self.addon.latest_version | ||
| version.all_files[0].update(status=amo.STATUS_BETA) |
There was a problem hiding this comment.
This causes the test to fail on line 2497 because the addon having no reviewed file (only beta files) will have its status reset to "unreviewed".
You're testing that the file you're posting is set as beta and signed, you don't care about the existing files, so you should simply remove this line.
|
When you rebased on master, it seems that line 1186 slipped through, and should now be removed (this will also fix the test With the other comments I posted on the tests, they should pass, and this should be ready to merge! Thanks! r+wc |
robhudson
changed the title
Handle new beta versions with signing (bug 1160593)
TODO:
is_betacheckbox.