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

Add customizable announcement text on home,login,logout,spawn #1913

Merged
merged 5 commits into from
Jun 12, 2018

Conversation

rkdarst
Copy link
Contributor

@rkdarst rkdarst commented May 22, 2018

  • Using the new template_vars setting (Allow extra variables to be passed into templates #1872), allow the variable
    announcement to create a header message on all the pages in the
    title, or the variables announcement_{home,login,logout,spawn} to
    set variables on these single pages.
  • This is not the most powerful method of putting an announcement into
    the templates, because it requires a server restart to change. But
    the invasiveness is very low, and allows minimal message
    without having to touch the templates themselves.
  • Closes: Add short announcement text in templates #1836

Usage:

c.JupyterHub.template_vars = {'announcement_spawn': 'this is a message',
                              'announcement': 'blah'}

This at least needs testing and documentation before it can be used. More importantly, needs consideration if it is a good idea. Also some designer could make it prettier and fit better. This idea could be in other places in the spirit of "make the default templates more customizable".

My big picture opinion is that this, or something like it, would be useful to include, but if people start going deep enough they would instead directly modify templates to do this. https://jupyterhub.readthedocs.io/en/latest/reference/templates.html#example is the alternative and I think it is clear enough now.

@@ -2,6 +2,13 @@

{% block main %}

{% if announcement_home or announcement %}
Copy link
Member

Choose a reason for hiding this comment

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

Since this is on every page, perhaps it should be put in the page.html template? I'm not 100% sure how to specify the extra variables, but perhaps:

{% if announcement_home %}
    {% set announcement = announcement_home %}
{% endif %}
{{ super() }}

would work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice idea. I was able to simplify a bit more even and it seems to work (pending tests). Another advantage is that there is now an announcement block which can be inherited separately in other templates - so that option (0) using your own templates becomes even easier. Option (0) should also allow you to easily change the message without restarting!

@rkdarst
Copy link
Contributor Author

rkdarst commented May 23, 2018

I think (untested) these latest changes should allow you to do the following, which is probably the easiest way to set a simple message using a template:

{% extends "page.html" %}
{% set announcement = "this is some custom local announcement"%}

Before I go forward too much more, does this look good?


{% block announcement %}
{% if announcement %}
<div class="container text-center">
Copy link
Member

Choose a reason for hiding this comment

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

Can this get the class 'announcement', too, for easier CSS? Then I think this is good to go.

@rkdarst
Copy link
Contributor Author

rkdarst commented May 25, 2018

I updated the PR. I started some tests (using the framework in test_pages.py), but am having trouble making the tests actually work (the get_page() function not doing what I expect). Should I forget them, or push and you can comment on what is wrong?

@rkdarst
Copy link
Contributor Author

rkdarst commented May 25, 2018

Oh, also let me add documentation first... obviously.

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Approved, pending docs. Thanks!

@rkdarst
Copy link
Contributor Author

rkdarst commented May 27, 2018

Re-pushed. The second commit adds tests, but the tests just don't work - I'm not doing the HTTP simulation correctly somehow. If you have hints, I can fix them, or I can just remove the tests.

ann01 = 'ANNOUNCE01'
ann02 = 'ANNOUNCE02'
with mock.patch.dict(app.users.settings,
{'template_vars': {'announcement': ann01}}):
Copy link
Member

Choose a reason for hiding this comment

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

Add the FormSpawner to the patch so that requesting spawn renders the spawn page instead of triggering an automatic launch:

    with mock.patch.dict(
        app.users.settings,
        {
            'template_vars': {
                'announcement': ann01,
            },
            'spawner_class': FormSpawner,
        }
    ):

Then I think your tests will pass.

- Using the new template_vars setting (jupyterhub#1872), allow the variable
  `announcement` to create a header message on all the pages in the
  title, or the variables `announcement_{home,login,logout,spawn}` to
  set variables on these single pages.
- This is not the most powerful method of putting an announcement into
  the templates, because it requires a server restart to change.  But
  the invasiveness is very low, and allows minimal message
  without having to touch the templates themselves.
- Closes: jupyterhub#1836
- Adds test_pages.py:test_page_contents, which currently tests just
  the page annoucement variables.
@rkdarst
Copy link
Contributor Author

rkdarst commented Jun 2, 2018

Finally got around to making the tests pass. It's a relatively large number of lines for a small change, but maybe this block can be reused for other template tests, too.

Please also see if the docs updates are good - it's late and I should check them once more...

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Looks good @rkdarst. A suggestion on docs and tests. Thank you ☀️

@@ -1,4 +1,5 @@
{% extends "page.html" %}
{% if announcement_home %}{% set announcement = announcement_home %}{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit easier to read if these are broken into 3 lines.



@pytest.mark.gen_test
def test_page_contents(app):
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably more maintainable to break this into multiple tests.

@@ -59,3 +59,33 @@ text about the server starting up, place this content in a file named
<p>Patience is a virtue.</p>
{% endblock %}
```

## Simple configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

## Page Announcements

To add announcements to be displayed on a page, you have two options:

- Extend the page templates as described above
- Use configuration variables

### Announcement Configuration Variables

@minrk
Copy link
Member

minrk commented Jun 12, 2018

@willingc thanks for the review! I added some changes to address it.

@rkdarst thanks! This is going to probably be the last code change for 0.9.

@minrk minrk merged commit aa9676e into jupyterhub:master Jun 12, 2018
@willingc
Copy link
Contributor

Thanks @rkdarst for all your work on this 🎉

@rkdarst
Copy link
Contributor Author

rkdarst commented Jun 12, 2018 via email

@willingc
Copy link
Contributor

Thanks you @rkdarst. Your effort got us 90% there ☀️

@rkdarst rkdarst deleted the announcement_text branch December 4, 2018 21:45
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.

None yet

3 participants