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

Add support for hidden attachment folders #3181

Merged
merged 4 commits into from Feb 12, 2018

Conversation

Projects
None yet
4 participants
@bpedersen2
Contributor

bpedersen2 commented Dec 11, 2017

Not all uploaded files need to be shown in the materials listing
on the event front page, sometimes it should only be used for links
at some place in the event. Allow to always hide folders in the
frontend indepently from the access rights.

Fixes: #3180

# ### commands auto generated by Alembic ###
op.add_column('folders', sa.Column('is_always_hidden_in_fe', sa.Boolean(), nullable=False, server_default='false'),
schema='attachments')
op.alter_column('folders', 'is_always_hidden_in_fe', server_default=None, schema='attachments')

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 11, 2017

Contributor

Question: Is there something to be done to force the mutual exclusive setting of hide_in_fe and always_visible at the database level?

def upgrade():
# ### commands auto generated by Alembic ###

This comment has been minimized.

@mic4ael

mic4ael Dec 11, 2017

Member

You can remove the comments from upgrade and downgrade functions.

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 11, 2017

Contributor

Done

@mic4ael

This comment has been minimized.

Member

mic4ael commented Dec 11, 2017

On a side note you don't have to create a separate commit for each fix/change which is a result of a comment. You could easily use either git commit --amend or interactive rebase along with squash operation.

@@ -91,6 +91,14 @@ def __table_args__(cls):
default=True
)
#: If the folder is never shown in the frontend (even if you can access it)
is_always_hidden_in_fe = db.Column(

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

I kind of dislike this name. It's not very clear without reading the comment, and also too verbose. IMO is_hidden is a much better name for it. (also, we don't put an empty line between column definitions, and only one empty line between columns and relationships)

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 11, 2017

Contributor

Changed to is_hidden_in_fe .

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

I think the _in_fe is not very useful either; that's something worth mentioning in the docstring on top of it (like it already does)

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 11, 2017

Contributor

Ok, using is_hidden now

@@ -36,7 +36,7 @@ def get_attached_folders(linked_object, include_empty=True, include_hidden=True,
folders = AttachmentFolder.get_for_linked_object(linked_object, preload_event=preload_event)
if not include_hidden:
folders = [f for f in folders if f.can_view(session.user)]
folders = [f for f in folders if (f.can_view(session.user) and not f.is_always_hidden_in_fe)]

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

This check can be done in can_view

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 11, 2017

Contributor

Done

description=_("By default, folders are always visible, even if a user cannot "
"access them. You can disable this behavior here, hiding the folder "
"for anyone who does not have permission to access it."))
is_always_hidden_in_fe = BooleanField(_("Always Hidden in Frontend"),
[HiddenUnless('is_always_visible', value=False), ],

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

no need for the , at the end of the list - only single-element tuples need this.

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 11, 2017

Contributor

Done

@bpedersen2 bpedersen2 force-pushed the bpedersen2:hiddenfolders branch 2 times, most recently from 52c3c32 to b1422b0 Dec 11, 2017

description=_("By default, folders are always visible, even if a user cannot "
"access them. You can disable this behavior here, hiding the folder "
"for anyone who does not have permission to access it."))
is_hidden = BooleanField(_("Always Hidden in Frontend"),

This comment has been minimized.

@pferreir

pferreir Dec 11, 2017

Member

Hi,
I'm not that sure about the wording here. "Frontend" is not a term that a end user will necessarily understand.

This comment has been minimized.

@pferreir

pferreir Dec 11, 2017

Member

I discussed it briefly with @ThiefMaster, "Hide in event page" or similar could be an option.

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 12, 2017

Contributor

Done

@@ -64,6 +64,14 @@
title="{% trans %}The access to this folder is restricted, regardless of the protection scheme of
the parent.{% endtrans %}"></i>
{% endif %}
{% if folder.is_hidden and management%}

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

missing space before %}

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 12, 2017

Contributor

Done

@@ -64,6 +64,14 @@
title="{% trans %}The access to this folder is restricted, regardless of the protection scheme of
the parent.{% endtrans %}"></i>
{% endif %}
{% if folder.is_hidden and management%}
<i class=" icon-eye-blocked"
title="{% trans %}This folder is always hidden in the frontend.{% endtrans %}"></i>

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

not enough indentation (attrs should be aligned)

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 12, 2017

Contributor

Done

@@ -64,6 +64,14 @@
title="{% trans %}The access to this folder is restricted, regardless of the protection scheme of
the parent.{% endtrans %}"></i>
{% endif %}
{% if folder.is_hidden and management%}
<i class=" icon-eye-blocked"

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

there shouldn't be a space right after the opening quote

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 12, 2017

Contributor

Done

{% endif %}
{% if folder.is_always_visible and management%}
<i class=" icon-eye"
title="{% trans %}This folder is always visible in the frontend.{% endtrans %}"></i>

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

Maybe this is a bit noisy since it's enabled by default? Explicitly showing the hidden flag makes sense but for this one I'm not sure if there's any benefit

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 12, 2017

Contributor

Well, it is only shown in managment views. And for symmetry ( all other flags one can set on a folder are indicated) I think it is helpful.

nullable=False,
default=False
)

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

one linebreak too much (there should be one empty line between this and the the relationships)

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 12, 2017

Contributor

Done

op.add_column('folders', sa.Column('is_hidden', sa.Boolean(), nullable=False, server_default='false'),
schema='attachments')
op.alter_column('folders', 'is_hidden', server_default=None, schema='attachments')
op.create_check_constraint('ck_folders_exclusive_is_hidden_is_always_visible', 'folders',

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

Not sure if a DB constraint is really important here - there is no harm if both are set for some reason.

Anyway, if you want to keep it, make sure to add it to the model as well!

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 12, 2017

Contributor

Added constraint to model as well. I think this is needed, as both switches would get hidden if both flags are set in the db. Then the only way to recover would be to edit values in the database by hand.

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

Good point indeed!

BTW: This should be validated on the Form level as well; i'll add a snippet to the comment once i'm in the office.

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member
def validate_is_always_visible(self, field):
    if self.is_always_visible.data and self.is_hidden.data:
        raise ValidationError('These two options cannot be used at the same time')

validate_is_hidden = validate_is_always_visible

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 12, 2017

Contributor

Done

@bpedersen2 bpedersen2 force-pushed the bpedersen2:hiddenfolders branch 2 times, most recently from 700c81a to 46bb658 Dec 12, 2017

@@ -50,7 +50,7 @@ def __auto_table_args():
return (db.CheckConstraint(default_inheriting, 'default_inheriting'),
db.CheckConstraint('is_default = (title IS NULL)', 'default_or_title'),
db.CheckConstraint('not (is_default and is_deleted)', 'default_not_deleted'),
{'schema': 'attachments'})

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

what happened to this?!

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 12, 2017

Contributor

Lost due to mis-reading braces..., readded

@@ -161,6 +167,8 @@ def can_view(self, user):
"""
if not self.object.can_access(user):
return False
if self.is_hidden:

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

do this before the access check; it's much cheaper!

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 12, 2017

Contributor

done

@bpedersen2 bpedersen2 force-pushed the bpedersen2:hiddenfolders branch from 46bb658 to d159639 Dec 12, 2017

@ThiefMaster ThiefMaster changed the base branch from v2.1-dev to master Jan 12, 2018

@bpedersen2 bpedersen2 force-pushed the bpedersen2:hiddenfolders branch from d159639 to b984a98 Jan 29, 2018

@bpedersen2

This comment has been minimized.

Contributor

bpedersen2 commented Jan 29, 2018

Bumped migrations

@bpedersen2 bpedersen2 force-pushed the bpedersen2:hiddenfolders branch from b984a98 to 1106aad Jan 29, 2018

'NOT (is_hidden AND is_always_visible)', schema='attachments')
def validate_is_always_visible(self, field):

This comment has been minimized.

@ThiefMaster

ThiefMaster Feb 12, 2018

Member

this doesn't belong in here

bpedersen2 and others added some commits Dec 11, 2017

Add support for hidden attachment folders
Not all uploaded files need to be shown in the materials listing
on the event front page, sometimes it should only be used for links
at some place in the event. Allow to always hide folders in the
frontend indepently from the access rights.

Fixes: #3180
Add folder visibility indicator for attachments
If in management mode, indicate forced visibiliy/invisibility
using an (striked-out) eye-icon.

@ThiefMaster ThiefMaster force-pushed the bpedersen2:hiddenfolders branch from 1106aad to db3666f Feb 12, 2018

@ThiefMaster ThiefMaster merged commit 9341a90 into indico:master Feb 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bpedersen2 bpedersen2 deleted the bpedersen2:hiddenfolders branch Feb 13, 2018

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