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

fix for 4146 - revision page edit button only shows with correct perms #4850

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@FedorSelitsky
Contributor

FedorSelitsky commented Oct 27, 2018

Hello,

Fixes issue #4146

There was already a pull request - #4189, but it has not been updated for a long time.

@m1kola

m1kola approved these changes Oct 27, 2018 edited

Looks good to me.

@gasman will probably want to have a look as he reviewed the original PR (#4189)

@m1kola

This comment has been minimized.

Member

m1kola commented Oct 27, 2018

The TOXENV=py36-djmaster-postgres-noelasticsearch test job fails, but I don't think it's related to the changes in this PR. It is failing even in the master branch, at the moment (see this example).

@gasman

This comment has been minimized.

Collaborator

gasman commented Oct 27, 2018

Thanks @FedorSelitsky! I'm happy with this approach.

It looks like @kcleong's feedback on the original PR (#4189 (comment)) still applies here - we also need to do something about the link on the page title. I'd suggest linking to the preview action instead.

Also, this will need a test - I'm not sure if we currently have any tests for the moderation list, so I'm happy to pick this up if necessary.

@m1kola Yes, the test against Django master is a known expected failure and can be ignored here. (Hoping I'll get chance to work on that this weekend...)

@FedorSelitsky FedorSelitsky force-pushed the FedorSelitsky:4146_permission_edit_button branch from 247a8f7 to 5b24622 Oct 28, 2018

{% if page_perms.can_edit %}
<li><a href="{% url 'wagtailadmin_pages:edit' revision.page.id %}" class="button button-small button-secondary">{% trans 'Edit' %}</a></li>
{% endif %}
<li><a href="{% url 'wagtailadmin_pages:preview_for_moderation' revision.id %}" class="button button-small button-secondary" target="_blank">{% trans 'Preview' %}</a></li>

This comment has been minimized.

@m1kola

m1kola Oct 28, 2018

Member

You missed rel="noopener noreferrer" while resolving conflicts. It was added recently in 303ee0f (see this).

@FedorSelitsky

This comment has been minimized.

Contributor

FedorSelitsky commented Oct 28, 2018

@gasman Please, have another look

@gasman

gasman approved these changes Oct 28, 2018

m1kola added a commit that referenced this pull request Oct 28, 2018

@m1kola

This comment has been minimized.

Member

m1kola commented Oct 28, 2018

Merged in 86865ba. Release notes in a0a58d8

Thanks, Fedor and everyone involved in fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment