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

fix: restore ancestors and decendants on document graph #6646

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

rjsparks
Copy link
Member

No description provided.

@rjsparks rjsparks changed the title fix: restore rfcs on document graph when looking at rfc predecesors fix: restore ancestors and decendants on document graph Nov 16, 2023
Copy link
Member

@jennifer-richards jennifer-richards left a comment

Choose a reason for hiding this comment

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

This looks fine to me - a couple small questions inline

history[url] = {
"name": d.name,
"rev": d.name,
"published": e and e.time.isoformat(),
Copy link
Member

Choose a reason for hiding this comment

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

Did you check whether the consumer of this data can handle a null in the JSON here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not, but it would mean we have an RFC without a published event, which is an anomaly and the consequence would be that the graph doesn't display.

'url': url,
}
if d.history_set.filter(rev=e.newrevisiondocevent.rev).exists():
history[url]['pages'] = d.history_set.filter(rev=e.newrevisiondocevent.rev).first().pages
Copy link
Member

Choose a reason for hiding this comment

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

Should there be an analogous lookup of pages for the RFC case?

Copy link
Member Author

Choose a reason for hiding this comment

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

good question - I'm not sure where it's used. It isn't used for this particular graph.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see that we use it, though we do describe using this endpoint as part of the API so outsiders might be using it.

@jennifer-richards jennifer-richards merged commit 9fedce0 into ietf-tools:feat/rfc Nov 16, 2023
4 of 7 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2023
@rjsparks rjsparks deleted the fixhistorygraph branch November 29, 2023 20:04
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.

2 participants