Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #5379: Remove footer field from theme submission form #7718

Merged
merged 3 commits into from Mar 21, 2018

Conversation

CuriousLearner
Copy link
Contributor

@CuriousLearner CuriousLearner commented Mar 4, 2018

Fixes mozilla/addons#4334

Removed footer field from Theme submission form

@CuriousLearner CuriousLearner force-pushed the fix-5379 branch 4 times, most recently from 8afd907 to fb9b215 Compare March 8, 2018 07:22
@CuriousLearner
Copy link
Contributor Author

Fixes mozilla/addons#4334

@eviljeff Can you please have a look at this?

@CuriousLearner CuriousLearner changed the title [WIP] Fix #5379: Remove footer field from theme submission form Fixes #5379: Remove footer field from theme submission form Mar 8, 2018
@eviljeff eviljeff requested a review from EnTeQuAk March 9, 2018 18:00
@CuriousLearner
Copy link
Contributor Author

Hey @EnTeQuAk !

Any updates here? :)

@EnTeQuAk
Copy link
Contributor

Sorry, I missed the notifications, I'll have a look shortly

Copy link
Contributor

@EnTeQuAk EnTeQuAk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor nitpick and a follow-up but otherwise this looks good. Thanks a lot for your work!

I'll merge this once you fixed the comments.

Can you also add a screenshot verifying that the footer isn't being queried in the form anymore? That'll help show QA where they have to verify the issue.

"""Save theme image and calculates checksum after theme save."""
dst_root = os.path.join(user_media_path('addons'), str(addon.id))
header = os.path.join(settings.TMP_PATH, 'persona_header', header)
header_dst = os.path.join(dst_root, 'header.png')
if footer:
footer = os.path.join(settings.TMP_PATH, 'persona_footer', footer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this lands we should remove all files in that folder too (nothing you can do though)

@@ -672,11 +648,12 @@ def test_reupload_legacy_header_only(self, make_checksum_mock):
- On approving, it would see 'footer.png' !== 'leg.png'
- It run move_stored_file('footer.png', 'leg.png').
- But footer.png does not exist. BAM BUG.

Footer has been removed in issue # 5379
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a link to the issue if possible.

@@ -1148,7 +1148,6 @@ def get_queryset(self):
class RereviewQueueTheme(ModelBase):
theme = models.ForeignKey(Persona)
header = models.CharField(max_length=72, blank=True, default='')
footer = models.CharField(max_length=72, blank=True, default='')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a migration in src/olympia/migrations/ that removes this field from the database. This should not be done in this PR though but as a follow-up. Can you file an issue about that?

@CuriousLearner
Copy link
Contributor Author

@EnTeQuAk I'm done with that nitpick. Also, I've created this issue: mozilla/addons#5371

settings.py Outdated
@@ -80,7 +80,7 @@
SERVICES_DOMAIN = urlparse(SITE_URL).netloc
SERVICES_URL = SITE_URL

ALLOWED_HOSTS = ALLOWED_HOSTS + [SERVICES_DOMAIN]
ALLOWED_HOSTS = ALLOWED_HOSTS + [SERVICES_DOMAIN] + ['localhost']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you submitted that accidentally, mind backing that change out?

@@ -1,3 +1,3 @@
from settings import * # noqa

INTERNAL_IPS = ('127.0.0.1',)
INTERNAL_IPS = ('127.0.0.1', 'localhost')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you submitted that accidentally, mind backing that change out?

@@ -672,11 +648,13 @@ def test_reupload_legacy_header_only(self, make_checksum_mock):
- On approving, it would see 'footer.png' !== 'leg.png'
- It run move_stored_file('footer.png', 'leg.png').
- But footer.png does not exist. BAM BUG.

Footer has been removed in issue Issue #5379
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate "Issue"

@CuriousLearner
Copy link
Contributor Author

@EnTeQuAk Oops! Sorry I accidentally added it. Fixed all those things.

@EnTeQuAk EnTeQuAk merged commit e54a169 into mozilla:master Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants