Skip to content

UI improvements to the admin interface for setting project upload limits #2473

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

Merged
merged 2 commits into from
Oct 6, 2017

Conversation

theacodes
Copy link
Contributor

  • Uses a number field instead of a text field.
  • The text field now uses megabytes instead of bytes.
  • The detail page now shows the value in megabytes as well as the full value in bytes.
  • The detail page now shows "Default" as well as indicating what the default limit is in MB instead of just displaying "None".
  • The form field should prevent numbers lower than the default, but the backend also now gives an invalid request error if the value is lower than the default.
  • Simplified the flash message as the full value will be displayed on the detail page.

Unfortunately, I couldn't use "step" in the form field as it would have limited the input to multiples of the step value.

Fixes #2471

* Uses a number field instead of a text field.
* The text field now uses megabytes instead of bytes.
* The detail page now shows the value in megabytes as well as the full value in bytes.
* The detail page now shows "Default" as well as indicating what the default limit is in MB instead of just displaying "None".

Unfortunately, I couldn't use "step" in the form field as it would have limited the input to multiples of the step value.

Fixes pypi#2471
@dstufft
Copy link
Member

dstufft commented Oct 6, 2017

Unfortunately, I couldn't use "step" in the form field as it would have limited the input to multiples of the step value.

A bummer, I was hoping it would just control the arrows and not be part of the client side validation. Ah well.

@theacodes
Copy link
Contributor Author

A bummer, I was hoping it would just control the arrows and not be part of the client side validation. Ah well.

I tried my best. :)

Copy link
Member

@dstufft dstufft left a comment

Choose a reason for hiding this comment

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

Looks pretty good, small tweak on formatting in the display, but otherwise LGTM.

<td>{{ project.upload_limit }}</td>
<td>
{% if project.upload_limit %}
{{ project.upload_limit / ONE_MB }} MB ({{ "{:,.0f}".format(project.upload_limit) }} bytes)
Copy link
Member

Choose a reason for hiding this comment

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

To format the bytes, I think it'd be better to use project.upload_limit|format_number(), it's more comprehensible then the terrible Python mini language :D

I'm torn on the display of the MB itself, we have filesizeformat(binary=True) which will do the same thing, except work nicer if we go > 1GB... but I don't think that we're likely to do that so I don't know how much it matters. Since I'm already asking for you to fix the number formatting for the bytes, I think unless you feel strongly about it using {{ project.upload_limit|filesizeformat(binary=True) }} would be my preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

<input type="text" name="upload_limit" class="form-control" id="uploadLimit" rows="3" value="{{ project.upload_limit | default('', True)}}">
<label for="uploadLimit">Upload limit (in Megabytes)</label>
{% if project.upload_limit %}
{% set upload_limit_value = project.upload_limit / ONE_MB %}
Copy link
Member

Choose a reason for hiding this comment

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

Unlike the display issue, this needs to use the same logic as the python code to go from bytes to MB, so this makes sense to keep this regardless of what happens above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted.

@dstufft dstufft merged commit edc0b87 into pypi:master Oct 6, 2017
@dstufft
Copy link
Member

dstufft commented Oct 6, 2017

Great, thanks a lot!

@theacodes
Copy link
Contributor Author

theacodes commented Oct 6, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants