Skip to content

feat: Add list page as requested in #4242 - #4259

Closed
kivinen wants to merge 6 commits into
ietf-tools:mainfrom
kivinen:ad-list-page
Closed

feat: Add list page as requested in #4242#4259
kivinen wants to merge 6 commits into
ietf-tools:mainfrom
kivinen:ad-list-page

Conversation

@kivinen

@kivinen kivinen commented Jul 23, 2022

Copy link
Copy Markdown
Contributor

Add new page to list all area directors and their current workload. Include links to the specific dashboards
for each area director. This new page is in doc/ad/.

kivinen added 5 commits July 23, 2022 18:23
and their current workload. Include links to the specific dashboards
for each area director. This new page is in doc/ad/.
and their current workload. Include links to the specific dashboards
for each area director. This new page is in doc/ad/.
Comment thread ietf/doc/views_search.py

return cached_redirect(cache_key, urlreverse('ietf.doc.views_search.search') + search_args)

def ad_dashboard_group_type(doc):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This adds a new point of maintenance when new document types are created. Instead of literal strings, why isn't this using DocTypeName.name? (see

content = markdown.markdown(doc.text_or_error())
) and, for instance, at #L315, just iterating over the DocTypeNames that are used?

Comment thread ietf/doc/views_search.py Outdated
if doc.type.slug=='draft':
if doc.get_state_slug('draft') == 'rfc':
return 'RFC'
elif doc.get_state_slug('draft') == 'active' and doc.get_state_slug('draft-iesg') and doc.get_state('draft-iesg').name in ('RFC Ed Queue Internet-Draft'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should that be ...in ('RFC Ed Queue', 'Internet-Draft')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like a real bug. Please address it, and consider the other feedback.

return True

@register.filter
def get_hash(h, key):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggest naming this get_from_dict or something that better matches the dict Python data type. (There's a similar filter lookup in agenda_custom_tags.py that could perhaps be moved and reused - this is a better place for it.)

Comment thread ietf/doc/views_search.py
return None
else:
return 'I-D'
elif doc.type.slug=='conflrev':

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Similar to @rjsparks's comment above would be better to make use of the DocTypeName data rather than repeating it here. A single doc.type.slug in [] test could replace a bunch of these cases

@rjsparks
rjsparks self-requested a review July 24, 2022 16:38

@rjsparks rjsparks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(See previous comments for requested change)

@kivinen

kivinen commented Jul 24, 2022 via email

Copy link
Copy Markdown
Contributor Author

@rjsparks

Copy link
Copy Markdown
Member

There are other comments to consider, but I wanted to point out:
the change at 76df542 should just be to use == instead of in.
You are getting lucky that a string in parenthesis is a string, not a tuple, and it leaves a trap for the maintainer:

>>> 'foo' in 'foobar'
True
>>> 'foo' in ('foobar')
True
>>> 'foo' in ('foobar',)
False

@NGPixel NGPixel changed the title feat: Ad list page as requested in #4242 feat: Add list page as requested in #4242 Jul 26, 2022
@rjsparks

rjsparks commented Aug 5, 2022

Copy link
Copy Markdown
Member

Rather than push into Tero's branch, I'm working in https://github.com/rjsparks/datatracker/tree/ad-list-page. I'll eventually make a PR that supersedes this one.

@rjsparks

Copy link
Copy Markdown
Member

Replaced by #4315.

@rjsparks rjsparks closed this Aug 11, 2022
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants