Fix #379 -- Add logo to event organizers #431

bolutife-lawrence · 2017-03-10T14:50:28Z

Fixes #379

With this solution, general organizer updates, and settings happen on a single page(single view), but still uses different forms. To improve this, even more, we can have organizer settings tabs similar to events with separate views, just so that things don't start to look messy when organizer settings takes on a little more.

Awaiting feedback :)

codecov · 2017-03-10T17:14:29Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b301d20). Click here to learn what that means.
The diff coverage is 70%.

@@            Coverage Diff            @@
##             master     #431   +/-   ##
=========================================
  Coverage          ?   79.41%           
=========================================
  Files             ?      163           
  Lines             ?    10822           
  Branches          ?        0           
=========================================
  Hits              ?     8594           
  Misses            ?     2228           
  Partials          ?        0

Impacted Files	Coverage Δ
src/pretix/presale/context.py	`81.81% <100%> (ø)`
src/pretix/control/forms/organizer.py	`78.26% <100%> (ø)`
src/pretix/control/views/organizer.py	`48.36% <60.86%> (ø)`

raphaelm

Hi,

first of all: thank you very much! I really like your contribution, it is good and clear to read.
I still found a few points where this could be improved, so if you could give them just another look. No worries, no big issues among them :)

Cheers
Raphael

raphaelm · 2017-03-11T23:13:36Z

src/pretix/control/forms/organizer.py

+        label=_('Logo image'),
+        ext_whitelist=(".png", ".jpg", ".svg", ".gif", ".jpeg"),
+        required=False,
+        help_text=_('Add a logo to your organizer page.')  # TODO: Use a better desc.


How about something similar to the event form, e.g:

help_text=_('If you provide a logo image, we will by default not show your organization name ' 'in the page header. We will show your logo with a maximal height of 120 pixels.')

Thought as much! Just wanted to be sure.

raphaelm · 2017-03-11T23:14:09Z

src/pretix/control/templates/pretixcontrol/organizers/detail.html

@@ -7,7 +7,7 @@
        {% blocktrans with name=organizer.name %}Organizer: {{ name }}{% endblocktrans %}
        <a href="{% url "control:organizer.edit" organizer=organizer.slug %}"
           class="btn btn-default">
-            <span class="fa fa-edit"></span>
+            <span class="fa fa-sliders"></span>


May I ask why you changed this? We use fa-edit a lot for editing (and fa-wrench for event configuration) and I think it is better to stay consistent than to introduce fa-sliders.

Sorry I didn't raise a flag before changing this. My bad!

Bringing in organizer Display settings into the general organizer's edit page; I was thinking of an icon that would intuitively tell the user that he/she can do more than just general edits. Something settings-like. However, now that I know why it must remain that way, I'll roll this back. :)

Its not that it must or that we don't like changes, its just that I value consistency very highly 😉

I do too. 👍

raphaelm · 2017-03-11T23:15:56Z

src/pretix/control/views/organizer.py

@@ -195,13 +198,50 @@ class OrganizerUpdate(OrganizerPermissionRequiredMixin, UpdateView):
    model = Organizer
    form_class = OrganizerUpdateForm
    template_name = 'pretixcontrol/organizers/edit.html'
-    permission = None
+    permission = 'can_change_settings'


This is useless, as OrganizerPermission currently has no can_change_settings attribute. (The permission system is not that well-polished, we know...)

raphaelm · 2017-03-11T23:18:43Z

src/pretix/presale/templates/pretixpresale/event/base.html

            {% if event_logo %}
                <a href="{% eventurl event "presale:event.index" %}" title="{{ event.name }}">
                    <img src="{{ event_logo|thumbnail_url:'logo' }}" alt="{{ event.name }}" class="event-logo" />
                </a>
            {% else %}
-                <a href="{% eventurl event "presale:event.index" %}">{{ event.name }}</a>
+                <h1><a href="{% eventurl event "presale:event.index" %}">{{ event.name }}</a></h1>
                <small>{{ event.get_date_range_display }}</small>


Was this intended? Before, the <small> was a sub-element of the <h1>. Let's keep it this way for now. (It doesn't look that bad in the line below, but we'd need to adjust the language switcher then.)

Surely this wasn't intended. I must have overlooked it or something. Thanks for pointing that out.

raphaelm · 2017-03-11T23:24:24Z

src/pretix/control/forms/organizer.py

+
+class OrganizerSettingsForm(SettingsForm):
+
+    logo_image = ExtFileField(


Here's a subtle thing that I also did not notice during review and that caught me by surprise when I tested your changes: Our settings key-value store is hierarchical. By this I mean that if you try to access a setting of an event and the setting is not set for that event, the setting of the organizer will be accessed instead. If that does not exist, the global setting will be taken and only if this also does not exist, the hardcoded default will be used.

Currently, we rarely use this hierarchy, as it is hard to represent it in the user interface and normally it does not gain any attention as the different levels normally use different keys. Here, however, the effect is that if you set the organizer logo, all events without a logo of their own will inherit the logo. I believe this is not what a user expects, or at least I did not expect that. The fix is easy: Call this thing organizer_logo_image instead 😉

Interesting! I do see that happening in SettingsProxy. Thanks for pointing that out. 👍

The way you handle settings in the app is pretty neat if you ask me 😄

Aww, thanks a lot! :)

bolutife-lawrence · 2017-03-12T11:41:28Z

Thanks, @raphaelm!

I have updated the PR with fixes to the highlighted issues. I'm working on adding tests to these. 😁 🔥

raphaelm

Nice. This looks all fine! In my opinion, this is ready to merge, but if you want to add one or two tests it would be even more awesome, so I'll keep this open for now. But code-wise, we're done here! 🎈

bolutife-lawrence · 2017-03-12T17:30:06Z

Nice! 🙌 Will have tests in soon enough.

raphaelm · 2017-03-29T13:33:51Z

Hi @b-lawrence, everything going alright? Don't want to build up pressure, just ask if there are any problems concerning the tests or if you just haven't found time so far. :)

bolutife-lawrence · 2017-04-02T18:17:41Z

Hi @raphaelm, Yeah! Everything OK. Sorry, I had to run off on some other project, thus, little time for my open source projects. I have started with the tests now and should have it in by tomorrow. ;)

Cheers,

raphaelm · 2017-04-02T18:53:08Z

No worries, take your time! I know well enough how this is.

raphaelm · 2017-05-06T09:19:54Z

Hey @b-lawrence, everything alright on your side? I'll probably need this feature sometime soon, but don't feel stressed by that, if you don't have time to finish this (and maybe rebase it against current master), I'll just finish it up myself, but I didn't want to do that without prior notice 😉

Refactor code Refactor code

Fix PR issues Fix PR issues

raphaelm · 2017-05-08T15:42:41Z

Since this was basically done, I took the liberty of rebasing this against current master and merging it, so we can continue working in this area :) Should you want to add the unit tests, please open a separate pull request. Thank you for this contribution, it's well appreciated! :)

Sync master with master of pretix/pretix@300f8f6 * Automatically sort new products to the end * Drop "squash your commits" from the dev guide * Add variation descriptions and allow to order addons * Link to Django's runserver options in dev docs * Allow <br> tags in rich text * Copy from event: deal with deleted items * Make validate_cart useful together with addons * Fix collapsing panels in the addon choice step * Button text change if addons are present * Update translations * Squash migrations and bump version * Ticket PDFs: Do not hide attendee name if code is hidden * Add a user guide on payments * Link PayPal and Stripe documentation in the respective forms * Hide payment fees if they are all equal to 0.00 * Refs pretix#39 -- New concept of "teams" (pretix#478) * New models * CRUD UI * UI for adding/removing team members * Log display for teams * Fix invitations, move frontend * Drop old models (incomplete) * Drop more old stuff * Drop even more old stuff * Fix tests * Fix permission test * flake8 fix * Add tests fore the new code * Rebase migrations * Fix typo in method name * Update translations * Force ordering of events on dashboard * Fix typos in events * Prepare the pretixdroid API for an async mode in the app * Pretixdroid tests: Ignore microseconds (chopped by mysql) * pretixdroid API: Add related lookups * Add idempotenty nonces to pretixdroid API * pretixdroid: force-accepting unpaids and time display * Marked webfonts as binary files (pretix#487) Webfonts now listed as binary in `.gitattributes`. Works on pretix#486 * Fix pretix#456 -- Allow products to be excluded from ticket-generation (pretix#483) * Added non-admission setting to event `ticket_download_nonadm` now setting in storage. Still need logic for order page/PDF generation. Works on pretix#456. * Download button considers `ticket_download_nonadm` Modified Django tags to look at item admission attribute and `ticket_download_nonadm` setting. Works on pretix#456. * Ticket output for non-admission disabled PDFs/etc. will only be permitted/generated for items with the `admission` attribute, or if the `ticket_download_nonadm` event setting is true. Applies to single and whole-order ticket downloads. Works on pretix#456. * Fixed product exclusion in PDF output Forgot PDF output was a plugin, now includes same check as base `BaseTicketOutput.generate_order`. Works on pretix#456 * Mail signature (pretix#485) * added signature field -- no function yet * added mail signature feature * fixed style issue * fixed problem with signature default * added unit test for mail signatures * added unit test for mail signatures * [WIP] Fix pretix#447 -- Sendmail plugin: Create new mail based on an old one (pretix#476) * send old email content to the new one * error key event * test commit * query bad ID * query bad ID * query bad ID * query bad ID * Update pretixdroid API version * Refs pretix#447 -- Extend copying old mails to subject and receipients * Fixed bugs and added test for date range rendering (pretix#488) * fixed bug for same dates, added unit check for daterange * fixed local language override in unit test * Fix pretix#297 -- pretixdroid: Show metrics in the control panel (pretix#481) * add checkin status page add dashboard widget add checkin page under orders * modify checkin logic added new fields in checkin page added filter items * add tests for checkins & minor improvement * support addin_product & noadm setting logic * remove name ordering check test case * Fix pretix#379 -- Add logo to event organizers (pretix#431) * [WIP] Add logo to event organizers. * Fix indentation issues. * Refactor code Refactor code Refactor code * Add new migration * Take files into account for organizer sform (settings form) * Fix grammer * Make bootstrap form errors specific to each fieldset * Display logo on organizer's page * Fix PR issues Fix PR issues Fix PR issues * Reorder imports * Remove conflicting migration * Fix rebase conflict * Fix pretix#41 -- Drag-and-drop ticket editor Undo/redo Useful toolbox Font selection Add text content Use hex for colors JS-side dump and load Save Load layout, proper undo/redo First steps to Python rendering More PDF rendering Copy and paste Buttons for keyboard actions Splash Screen Block unbeforeunload in dirty state Remove debugging output Preview Upload new PDFs via the editor Fix bugs during PDF reload, link in settings form New default ticket Add OpenSans BI Custom fonts, fix tests * Added bootstrap-colorpicker * Allow inline PDF display in CSP header * Add fontpack to list of plugins * Update German translation * Add ticketoutputpdf's assets to MANIFEST.i * Fix migration of old ticket styles * Fix iCal download URL * Multi-line location field, new field for admission time * Admission date and time in editor * Remove icon from "add to calendar" * Try to fix PDF display problems in Safari * Proxy cachedfiles that are used as editor previews * Check Event.presale_is_running in more places * Fix CSS generation with an empty color field * Fix missing placeholders and reformat the sendmail view * Fix bug that lead to wrong payment amount when switching payment method to PayPal later * Update translation

* Syncing fork to upstream (#1) Sync master with master of 300f8f6 * Automatically sort new products to the end * Drop "squash your commits" from the dev guide * Add variation descriptions and allow to order addons * Link to Django's runserver options in dev docs * Allow <br> tags in rich text * Copy from event: deal with deleted items * Make validate_cart useful together with addons * Fix collapsing panels in the addon choice step * Button text change if addons are present * Update translations * Squash migrations and bump version * Ticket PDFs: Do not hide attendee name if code is hidden * Add a user guide on payments * Link PayPal and Stripe documentation in the respective forms * Hide payment fees if they are all equal to 0.00 * Refs #39 -- New concept of "teams" (#478) * New models * CRUD UI * UI for adding/removing team members * Log display for teams * Fix invitations, move frontend * Drop old models (incomplete) * Drop more old stuff * Drop even more old stuff * Fix tests * Fix permission test * flake8 fix * Add tests fore the new code * Rebase migrations * Fix typo in method name * Update translations * Force ordering of events on dashboard * Fix typos in events * Prepare the pretixdroid API for an async mode in the app * Pretixdroid tests: Ignore microseconds (chopped by mysql) * pretixdroid API: Add related lookups * Add idempotenty nonces to pretixdroid API * pretixdroid: force-accepting unpaids and time display * Marked webfonts as binary files (#487) Webfonts now listed as binary in `.gitattributes`. Works on #486 * Fix #456 -- Allow products to be excluded from ticket-generation (#483) * Added non-admission setting to event `ticket_download_nonadm` now setting in storage. Still need logic for order page/PDF generation. Works on #456. * Download button considers `ticket_download_nonadm` Modified Django tags to look at item admission attribute and `ticket_download_nonadm` setting. Works on #456. * Ticket output for non-admission disabled PDFs/etc. will only be permitted/generated for items with the `admission` attribute, or if the `ticket_download_nonadm` event setting is true. Applies to single and whole-order ticket downloads. Works on #456. * Fixed product exclusion in PDF output Forgot PDF output was a plugin, now includes same check as base `BaseTicketOutput.generate_order`. Works on #456 * Mail signature (#485) * added signature field -- no function yet * added mail signature feature * fixed style issue * fixed problem with signature default * added unit test for mail signatures * added unit test for mail signatures * [WIP] Fix #447 -- Sendmail plugin: Create new mail based on an old one (#476) * send old email content to the new one * error key event * test commit * query bad ID * query bad ID * query bad ID * query bad ID * Update pretixdroid API version * Refs #447 -- Extend copying old mails to subject and receipients * Fixed bugs and added test for date range rendering (#488) * fixed bug for same dates, added unit check for daterange * fixed local language override in unit test * Fix #297 -- pretixdroid: Show metrics in the control panel (#481) * add checkin status page add dashboard widget add checkin page under orders * modify checkin logic added new fields in checkin page added filter items * add tests for checkins & minor improvement * support addin_product & noadm setting logic * remove name ordering check test case * Fix #379 -- Add logo to event organizers (#431) * [WIP] Add logo to event organizers. * Fix indentation issues. * Refactor code Refactor code Refactor code * Add new migration * Take files into account for organizer sform (settings form) * Fix grammer * Make bootstrap form errors specific to each fieldset * Display logo on organizer's page * Fix PR issues Fix PR issues Fix PR issues * Reorder imports * Remove conflicting migration * Fix rebase conflict * Fix #41 -- Drag-and-drop ticket editor Undo/redo Useful toolbox Font selection Add text content Use hex for colors JS-side dump and load Save Load layout, proper undo/redo First steps to Python rendering More PDF rendering Copy and paste Buttons for keyboard actions Splash Screen Block unbeforeunload in dirty state Remove debugging output Preview Upload new PDFs via the editor Fix bugs during PDF reload, link in settings form New default ticket Add OpenSans BI Custom fonts, fix tests * Added bootstrap-colorpicker * Allow inline PDF display in CSP header * Add fontpack to list of plugins * Update German translation * Add ticketoutputpdf's assets to MANIFEST.i * Fix migration of old ticket styles * Fix iCal download URL * Multi-line location field, new field for admission time * Admission date and time in editor * Remove icon from "add to calendar" * Try to fix PDF display problems in Safari * Proxy cachedfiles that are used as editor previews * Check Event.presale_is_running in more places * Fix CSS generation with an empty color field * Fix missing placeholders and reformat the sendmail view * Fix bug that lead to wrong payment amount when switching payment method to PayPal later * Update translation * Revert "Syncing fork to upstream (#1)" This reverts commit 847d409. Merged wrong, my bad. * Formatted OTP secret New variable `secretGrouped` in `2fa_confirm_totp.html`, user-friendly version of OTP secret (split every 4 characters). Works on #443. * Adds manual secret entry OTP setup screen `secretGrouped` exposed in user-friendly fashion. Includes short instructions, copy-to-clipboard button, and js to hide instructions unless user clicks on "Can't scan the barcode?" link. Works on #443. * Minor indentation issuer Fixed indentation issue (L40). Works on #443. * Minor spacing issues L265 of `user.py` failing flake8 tests, minor spacing fixes. * Fixes indentation in `2fa_confirm_totp.html` Per #490 (comment), fixes an issue with indentation. Works on #443, member of #490. * Removed `aria-*` attributes Per #490 (comment), removes `aria` attributes from sub-tutorial. Works on #443, member of #490. * Pretix capitalization issue Per #490 (comment), fixes an issue with capitalization of pretix. Works on #443, member of #490.

bolutife-lawrence changed the title ~~[WIP] Add logo to event organizers.~~ FIX:#279 [WIP] Add logo to event organizers. Mar 11, 2017

bolutife-lawrence changed the title ~~FIX:#279 [WIP] Add logo to event organizers.~~ FIX:#379 [WIP] Add logo to event organizers. Mar 11, 2017

bolutife-lawrence changed the title ~~FIX:#379 [WIP] Add logo to event organizers.~~ FIX:#379 Add logo to event organizers Mar 11, 2017

raphaelm reviewed Mar 11, 2017

View reviewed changes

raphaelm added the pr status: needs improvement label Mar 11, 2017

bolutife-lawrence force-pushed the organizers-logo branch from a3fd598 to a06d49f Compare March 12, 2017 12:05

raphaelm approved these changes Mar 12, 2017

View reviewed changes

raphaelm removed the pr status: needs improvement label Mar 12, 2017

raphaelm modified the milestone: 1.2.0 Mar 24, 2017

raphaelm removed this from the 1.2.0 milestone Apr 10, 2017

raphaelm changed the title ~~FIX:#379 Add logo to event organizers~~ Fix #379 -- Add logo to event organizers Apr 14, 2017

bolutife-lawrence and others added 11 commits May 8, 2017 17:37

[WIP] Add logo to event organizers.

1d1d894

Fix indentation issues.

3c8b6f2

Refactor code

7e15b79

Refactor code Refactor code

Add new migration

023da5f

Take files into account for organizer sform (settings form)

ca9ee1f

Fix grammer

01f4dc7

Make bootstrap form errors specific to each fieldset

e295f9c

Display logo on organizer's page

07ce93f

Fix PR issues

a6258dd

Fix PR issues Fix PR issues

Reorder imports

9571ce1

Remove conflicting migration

f8cf2a6

raphaelm force-pushed the organizers-logo branch from a06d49f to f8cf2a6 Compare May 8, 2017 15:40

Fix rebase conflict

ef219f0

raphaelm merged commit 962cdef into pretix:master May 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #379 -- Add logo to event organizers #431

Fix #379 -- Add logo to event organizers #431

bolutife-lawrence commented Mar 10, 2017 •

edited

Loading

codecov bot commented Mar 10, 2017 •

edited

Loading

raphaelm left a comment

raphaelm Mar 11, 2017

bolutife-lawrence Mar 12, 2017

raphaelm Mar 11, 2017

bolutife-lawrence Mar 12, 2017 •

edited

Loading

raphaelm Mar 12, 2017

bolutife-lawrence Mar 12, 2017

raphaelm Mar 11, 2017

bolutife-lawrence Mar 12, 2017

raphaelm Mar 11, 2017

bolutife-lawrence Mar 12, 2017 •

edited

Loading

raphaelm Mar 11, 2017

bolutife-lawrence Mar 12, 2017

bolutife-lawrence Mar 12, 2017

raphaelm Mar 12, 2017

bolutife-lawrence commented Mar 12, 2017

raphaelm left a comment

bolutife-lawrence commented Mar 12, 2017

raphaelm commented Mar 29, 2017

bolutife-lawrence commented Apr 2, 2017

raphaelm commented Apr 2, 2017

raphaelm commented May 6, 2017

raphaelm commented May 8, 2017


		class OrganizerSettingsForm(SettingsForm):

		logo_image = ExtFileField(

Fix #379 -- Add logo to event organizers #431

Fix #379 -- Add logo to event organizers #431

Conversation

bolutife-lawrence commented Mar 10, 2017 • edited Loading

codecov bot commented Mar 10, 2017 • edited Loading

Codecov Report

raphaelm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bolutife-lawrence Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bolutife-lawrence Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bolutife-lawrence commented Mar 12, 2017

raphaelm left a comment

Choose a reason for hiding this comment

bolutife-lawrence commented Mar 12, 2017

raphaelm commented Mar 29, 2017

bolutife-lawrence commented Apr 2, 2017

raphaelm commented Apr 2, 2017

raphaelm commented May 6, 2017

raphaelm commented May 8, 2017

bolutife-lawrence commented Mar 10, 2017 •

edited

Loading

codecov bot commented Mar 10, 2017 •

edited

Loading

bolutife-lawrence Mar 12, 2017 •

edited

Loading

bolutife-lawrence Mar 12, 2017 •

edited

Loading