-
Notifications
You must be signed in to change notification settings - Fork 378
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
feat: enable editorial stream adoption and balloting #5011
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5011 +/- ##
==========================================
+ Coverage 88.47% 88.49% +0.01%
==========================================
Files 296 296
Lines 39818 40019 +201
==========================================
+ Hits 35230 35414 +184
- Misses 4588 4605 +17
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Fixes #5000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor questions inline
return can_defer(user, doc) | ||
|
||
@register.filter() | ||
def can_request_rfc_publication(user, doc): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this used anywhere - is it likely to be needed (or perhaps a sign of something omitted)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's an artifact of discovering that the view is adding buttons to what's rendered late in the game. I think we should leave this, though, as those views/templates really should get refactored to put the display logic back in the template.
stream_id = 'editorial' | ||
|
||
@factory.post_generation | ||
def states(obj, create, extracted, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the linter no longer complain about having the first parameter as obj
instead of self
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen no complaints, and functionally, self makes less sense than obj given that this is a factory, and this is called by the factory engine at untypical times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - some places have pylint directives on these lines, but I guess they're obsolete
) | ||
for slug, name, order in ( | ||
("repl", "Replaced editorial stream document", 0), | ||
("active", "Active editorial stream document", 2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No order=1
state? I guess it's a side effect of creating then deleting the rsab_review
state and doesn't matter aside from the order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
order is almost a nuisance field. I think we'll have less confusion with this migration as is. We can make order look prettier (to no good effect otherwise) later.
(edited): I had earlier suggested this become a feature branch. I don't think that's needed now, and propose this just be merged into main.