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

Highlight certain dates in the calendar datepicker #259

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

mattpitkin
Copy link
Contributor

I'm using gw_summary to show summary pages that will only be produced on certain dates. This PR allows you to specify in the configuration file either a selection of specific dates to highlight, or whether the page will check URLs for dates that exist (non-highlighted dates will be disabled).

In both methods I have it so that the requested dates are highlighted and other dates are not enabled. The latter method, which checks the URLs can be a bit slow in rendering the page, as it has to check the URLs synchronously before rendering.

To select the former option you would add e.g. (the dates can be with or without the hyphens)

[calendar]
highlighted-dates = 2019-01-02,2019-01-05

and for the latter option you would add

[calendar]
highlight-available = True

If you have the highlighted-dates option, then that is favoured over the highlight-available option. If you have neither of these then things should render as normal.

Currently, the options in the calendar section are read in in the from_ini class method of the BaseTab, as I couldn't find a better way to do it. I'd tried adding a from_ini method to the IntervalTab in which to read in the values, but couldn't get that to work.

Does this patch look ok/useful?

@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

Merging #259 into master will decrease coverage by <.01%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
- Coverage   38.84%   38.84%   -<.01%     
==========================================
  Files          47       47              
  Lines        7420     7444      +24     
==========================================
+ Hits         2882     2891       +9     
- Misses       4538     4553      +15
Flag Coverage Δ
#Linux 38.84% <25%> (ø) ⬇️
#python27 38.81% <25%> (ø) ⬇️
#python36 38.84% <25%> (ø) ⬇️
#python37 38.84% <25%> (ø) ⬇️
Impacted Files Coverage Δ
gwsumm/tabs/core.py 30.8% <0%> (-1.14%) ⬇️
gwsumm/html/bootstrap.py 100% <100%> (ø) ⬆️
gwsumm/tabs/management.py 21.05% <0%> (ø) ⬆️
gwsumm/tabs/etg.py 15.12% <0%> (ø) ⬆️
gwsumm/plot/mixins.py 15.79% <0%> (ø) ⬆️
gwsumm/archive.py 76.27% <0%> (+0.13%) ⬆️
gwsumm/plot/__init__.py 94.44% <0%> (+0.33%) ⬆️
gwsumm/tabs/sei.py 15.18% <0%> (+0.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29fdde7...224cbef. Read the comment docs.

@mattpitkin
Copy link
Contributor Author

Regarding checking if certain dates URLs are available to highlight - it's probably better for this to be done server side using PHP and then passing the variable to the JavaScript. I'll have a loot into making this change.

@alurban alurban self-assigned this Apr 26, 2019
@alurban alurban added this to the 0.2.0 milestone Apr 26, 2019
@alurban alurban self-requested a review April 26, 2019 14:26
Copy link

@alurban alurban left a comment

Choose a reason for hiding this comment

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

@mattpitkin, just a couple of minor things to change, otherwise this looks awesome, thanks for putting it together. I do think this will be useful, especially for people using GWSumm outside of the production summary pages, which I'm on a mission to put more effort into supporting.

A couple of general thoughts:

  • Can you post a working example of this new feature?
  • Can you also update the (brand-new) unit test for gwsumm.html.bootstrap.calendar?
  • Since you're starting to become a regular contributor (which is awesome), in the future it might be worthwhile to post feature requests like this one as issue tickets so that we can coordinate your contributions with the release cycle.

gwsumm/html/bootstrap.py Outdated Show resolved Hide resolved
gwsumm/tabs/core.py Outdated Show resolved Hide resolved
gwsumm/tabs/core.py Outdated Show resolved Hide resolved
@alurban
Copy link

alurban commented Apr 26, 2019

@mattpitkin, we currently don't use PHP on the summary pages, so JavaScript is actually the preferred method of implementation, at least for now. I think once we have several months of O3 under our belt we can return to the question of whether PHP would be a worthwhile thing to support.

@mattpitkin mattpitkin changed the title Highlight certain dates in the calendar datepicker WIP: Highlight certain dates in the calendar datepicker Apr 26, 2019
@mattpitkin
Copy link
Contributor Author

@alurban just saw your comment about PHP, although in the latest commit I've actually added a PHP file that lists the directories that exist. This is a lot quicker than my previous JavaScript-only alternative. I've not checked it works fully yet, but let me know what you think.

@alurban
Copy link

alurban commented Apr 26, 2019

@mattpitkin, I think that in general we shouldn't move to support PHP. However, since I'm wholly ignorant of how PHP works, I'm going to request a review from @duncanmmacleod.

@mattpitkin
Copy link
Contributor Author

@alurban after thinking a bit more, I can probably get rid of the PHP again and achieve the same thing, by getting gw_summary to create/update a list of available directories that is available via an XMLHtmlRequest. I'll make the change after the weekend.

@mattpitkin mattpitkin changed the title WIP: Highlight certain dates in the calendar datepicker Highlight certain dates in the calendar datepicker Apr 27, 2019
@mattpitkin
Copy link
Contributor Author

@alurban I've now removed the need for the PHP, which is good 😄

I've also updated the unit test for calendar in test_bootstrap.py.

I'll add a working example soon.

@mattpitkin
Copy link
Contributor Author

For completeness I've just added an issue #262 that goes with this PR.

@mattpitkin
Copy link
Contributor Author

An example ini file (summary.ini) for running this and automatically highlighting available dates could be:

[calendar]
start-of-week = sunday
start-date = 2019-04-01
highlight-available = True

[tab-summary]
type = external
name = Summary
url = sometext.txt
success = $("#content").html('<div class="scaffold well">'+data+'</div>');

with the file sometext.txt containing whatever text you want. This could be run twice with, e.g.:

gw_summary day 20190423 -o . --config-file summary.ini --verbose

and

gw_summary day 20190424 -o . --config-file summary.ini --verbose

and given something that looks like:

FireShot Capture 018 - Summary - ldas-jobs ligo caltech edu

Copy link

@alurban alurban left a comment

Choose a reason for hiding this comment

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

@mattpitkin, this is looking really good, sorry for the delay getting back to you (we've been busy trying to upgrade the summary pages to conda). A few more comments are posted below, let me know what you think, and thanks for working on this!

gwsumm/html/static.py Outdated Show resolved Hide resolved
gwsumm/html/tests/test_bootstrap.py Outdated Show resolved Hide resolved
attributekwargs['highlight-dates'] = highlighteddates.replace('-', '')
elif highlightavailable:
# set location of dates file
datefile = '{}/list-dirs.txt'.format(
Copy link

@alurban alurban May 1, 2019

Choose a reason for hiding this comment

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

@mattpitkin, see my comment to gw_summary below. If we decide to keep this file rather than the raw list of dates, then this variable should actually be passed as a keyword argument with a default of None, i.e.

def calendar(date, tag='a', class_='navbar-brand dropdown-toggle',
             id_='calendar', dateformat=None, mode=None, datefile=None,
             highlighteddates=None, highlightavailable=False):

That would make this a lot more robust as it would enforce that the file expected here be the same as the one defined in gw_summary. Any calls to gwsumm.html.bootstrap.calendar would have to be updated along with this function's docstring and unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made changes to pass a datefile argument as suggested. In gw_summary I get it to add the file name to the config variable.

bin/gw_summary Show resolved Hide resolved
Copy link

@alurban alurban left a comment

Choose a reason for hiding this comment

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

@mattpitkin, a couple more comments and one request: after you make these changes, can you post a link to a simple working example? Otherwise, these changes look great.

bin/gw_summary Outdated Show resolved Hide resolved
bin/gw_summary Outdated
datedirs = sorted([thisdir for thisdir in os.listdir(dirpath)
if os.path.isdir(os.path.join(dirpath, thisdir))])
dirfile = os.path.join(dirpath, 'list-dirs.txt')
config.set('calendar', 'date-file', dirfile)
Copy link

Choose a reason for hiding this comment

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

@mattpitkin, my one concern here is that users won't be able to pass a date-file in the INI configuration because it will be overwritten by the call to config.set. I think you can work around this pretty easily though, by changing from set to get:

Suggested change
config.set('calendar', 'date-file', dirfile)
config.get('calendar', 'date-file', dirfile)

I haven't tested this though, so you'll probably want to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 224cbef I've made a change to check if the user is passing a date-file themselves.

Alex L. Urban and others added 2 commits May 2, 2019 09:20
@eagoetz
Copy link
Collaborator

eagoetz commented Jul 22, 2022

@mattpitkin Sorry that this PR has been left to linger for so long. I am unsure what the status of it was before @alurban left the collaboration to work in the green fields of industry. It appears that we won't be able to merge it since some of this PR is impacted by #289. However, maybe it can still be deployed but it will take a PR first over on gwdetchar/bootstrap and then possibly some changes in gwsumm.

How do you want to move forward with this? We could close the PR so that you can split up the PR into the gwdetchar PR and a new gwsumm PR. What do you think?

Thanks, and sorry again for the way-to-long delay

@mattpitkin
Copy link
Contributor Author

@eagoetz no problem, I'd completely forgotten about this. I'll have a look and see how easy this is to split into two new PRs. If it's not too much work I'll do that, but otherwise we'll just have to close this.

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

Successfully merging this pull request may close these issues.

3 participants