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

[ready for review] Pre-fill SubmissionInfo form of CfP with track and submission_type #628

Merged
merged 6 commits into from Apr 5, 2019

Conversation

Projects
None yet
2 participants
@Nakaner
Copy link
Contributor

Nakaner commented Mar 9, 2019

If the URL query string for the submission info form or the CfP landing page contains a track or submission_types field, they will be used to pre-fill the submission info form. If the values do not match any in the database, the fields will have their default as initial value.

Add slug fields to the SubmissionType and Track model in order to have a easy to query but still memorizeable (by humans) identifier because the i18n char fields are not designed to be queried.

Slugs for submission types and tracks can be choosen by organisers. The migration tries to set readable but unique values for existing entries in the database.

This allows to have two different CfP for different tracks of a conference outside of Pretalx and to point submitters to the "right" submission form for their track. In my case, a conference has about five "normal" and one academic track. The academic programme committee fears that their submitters don't select the right track due to the high number of thematic tracks for the normal part of the conference. (Sie sehen den Wald vor lauter Bäumen nicht) The behaviour of this feature is a bit like a anchor in an URL pointing to the right section of a longer web page.

I would like to have feedback on the implemention.

This feature is a feature known by experts at the moment due to a lack of a proper user documentation. Where shall I document it?

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My change is listed in the CHANGELOG.rst if appropriate.

@Nakaner Nakaner force-pushed the Nakaner:track_and_submission_type_as_qs branch from f9efda0 to 8487f7c Mar 9, 2019

@Nakaner Nakaner changed the title [WIP] Pre-fill SubmissionInfo form of CfP with track and submission_type Pre-fill SubmissionInfo form of CfP with track and submission_type Mar 9, 2019

@Nakaner

This comment has been minimized.

Copy link
Contributor Author

Nakaner commented Mar 9, 2019

If I knew how to convert this pull request now to a normal pull request, I would do it. Unfortunately, the user interface lacks the button "mark ready for review" next to "This pull request is still a work in progress."

@rixx Shall I recreate this PR or can you review it and merge it manually?

@Nakaner Nakaner changed the title Pre-fill SubmissionInfo form of CfP with track and submission_type [ready for review] Pre-fill SubmissionInfo form of CfP with track and submission_type Mar 9, 2019

@rixx rixx marked this pull request as ready for review Mar 9, 2019

@rixx
Copy link
Member

rixx left a comment

Thank you for your PR! Let me start with a high level comment: Couldn't we use the ID fields instead of adding slug fields?

@Nakaner

This comment has been minimized.

Copy link
Contributor Author

Nakaner commented Mar 10, 2019

I thought about serial vs. string for the slug field.

Pro string:

  • It is more human readable in the URL.
  • Uniqueness is only useful within an event (it is not enforced).
  • A serial field makes porting an event into another instance with other existing events difficult.

Pro integer/serial field:

  • performance

I am happy to change that if you ask me to do so.

@rixx
Copy link
Member

rixx left a comment

Having thought about it, I'm against adding a slug field, but for using a slug: Let's do it in a mixed format, ?track=123-mytrackname. Only the 123 is relevant, but the mytrackname tells people what is going on.

So I'd request that in the tables listing all submission types / tracks, that you'd add a link button "Copy prefilled CfP URL", which shows organisers that they can link to this submission type / track directly. You can just use Django's slugify method there.

Show resolved Hide resolved doc/changelog.rst Outdated
if submission_type:
params.append(f'submission_type={submission_type}')
if len(params) > 0:
context['submit_qs'] = f'?{"&".join(params)}'

This comment has been minimized.

Copy link
@rixx

rixx Mar 10, 2019

Member

Letting params be a dict and using urllib.urlencode(params) would make sure all content is properly escaped. (Please test this with a track with special characters, such as umlauts.

for field, model_name in (('submission_type', SubmissionType), ('track', Track)):
request_value = self.request.GET.get(field)
if request_value:
queryset = model_name.objects.filter(event=self.request.event, slug=request_value)

This comment has been minimized.

Copy link
@rixx

rixx Mar 10, 2019

Member
object = model_name.objects.filter(event=self.request.event, slug=request_value).first()
if object:
…

This avoids sending the COUNT to the database.

@Nakaner Nakaner force-pushed the Nakaner:track_and_submission_type_as_qs branch from 8487f7c to 062c2a6 Mar 10, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 10, 2019

Codecov Report

Merging #628 into master will decrease coverage by <.01%.
The diff coverage is 89.65%.

@@            Coverage Diff             @@
##           master     #628      +/-   ##
==========================================
- Coverage   89.59%   89.59%   -0.01%     
==========================================
  Files         137      137              
  Lines        8100     8127      +27     
  Branches      998     1004       +6     
==========================================
+ Hits         7257     7281      +24     
- Misses        569      572       +3     
  Partials      274      274
Impacted Files Coverage Δ
src/pretalx/cfp/views/event.py 97.43% <100%> (+0.21%) ⬆️
src/pretalx/submission/models/type.py 92% <100%> (+1.52%) ⬆️
src/pretalx/submission/models/track.py 100% <100%> (ø) ⬆️
src/pretalx/cfp/views/wizard.py 90.79% <83.33%> (-1.04%) ⬇️
@Nakaner

This comment has been minimized.

Copy link
Contributor Author

Nakaner commented Mar 10, 2019

The commit I pushed a minute ago should address all your issues. I introduced a new mixin IdBasedMixin for the slug you described.

You reorganised the tests last night. Do you expect me to rebase (and merge) my changes on current master?

@rixx

This comment has been minimized.

Copy link
Member

rixx commented Mar 10, 2019

You reorganised the tests last night. Do you expect me to rebase (and merge) my changes on current master?

Nah, that's fine, a rebase would probably be painful. I'll handle it on my end, don't worry about it.

# search requested object by ID
obj = model_name.objects.filter(event=self.request.event, id=request_id).first()
# ensure that the whole slug matches, not only the ID
if obj and obj.slug() == request_value:

This comment has been minimized.

Copy link
@rixx

rixx Mar 10, 2019

Member

But we can't be sure that this is true: The slug is calculated off str(obj.name) after all, which can differ between active locales. I'd just not validate this here.

"""
Get ID from slug value.
"""
return int(slug.split(cls.slug_separator)[0])

This comment has been minimized.

Copy link
@rixx

rixx Mar 10, 2019

Member

This potentially causes a ValueError if called with a string that doesn't start with a number.

@rixx

This comment has been minimized.

Copy link
Member

rixx commented Mar 10, 2019

Thank you for this PR! If it's alright by you, I'll address the remaining issues myself.

@rixx rixx force-pushed the Nakaner:track_and_submission_type_as_qs branch from 062c2a6 to 6de7401 Apr 4, 2019

@rixx rixx dismissed their stale review Apr 4, 2019

Fixed myself

@rixx

This comment has been minimized.

Copy link
Member

rixx commented Apr 4, 2019

I've rebased and added a couple of fixes and rewrites. Going to fix tests soon, and merge afterwards.

@rixx rixx force-pushed the Nakaner:track_and_submission_type_as_qs branch from 6de7401 to e5f70d9 Apr 5, 2019

Nakaner and others added some commits Mar 9, 2019

Pre-fill SubmissionInfo form of CfP with track and submission_type
If the URL query string for the submission info form or the CfP landing
page contains a track or submission_types key, their value will be used
to pre-fill the submission info form. If the values do not match any in
the database, the fields will have their default as initial value.

The values consist of the ID of the track/submission_type and their
slugified name. Creation of this ID based slug is provided by the new
mixin IdBasedSlug. The original names of tracks/submission types are not
suitable because they are I18nCharFields which are not designed to be
queried.

@rixx rixx force-pushed the Nakaner:track_and_submission_type_as_qs branch from 3506e30 to ec7d383 Apr 5, 2019

@rixx rixx force-pushed the Nakaner:track_and_submission_type_as_qs branch from ec7d383 to 940dd83 Apr 5, 2019

@rixx rixx merged commit cb83829 into pretalx:master Apr 5, 2019

3 checks passed

codecov/patch 89.65% of diff hit (target 89.59%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +0.06% compared to 9ed458c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.