-
Notifications
You must be signed in to change notification settings - Fork 54
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
changes for EDGAR 24.0.1 operation #607
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
@@ -269,10 +290,17 @@ def getLocalFile(self, file, relpath, request): | |||
|
|||
|
|||
def guiRun(cntlr, modelXbrl, attach, *args, **kwargs): | |||
for generateOnCall in pluginClassMethods("iXBRLViewer.GenerateOnCall"): |
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.
This seems problematic as another plugin essentially disables the ixbrl-viewer with this hook. The ixbrl-viewer tools menu and its settings no longer work as expected (Launch on Load, etc.) for users.
If I'm understanding correctly there's the need for 4 things.
- for another plugin to generate a viewer on demand (with potentially its own customized/older javascript).
- security hooks for another plugin to control how the viewer files are written to disk.
- the option to only write the stub file to disk.
- the ability to enable the ixbrl-viewer plugin, but only have it generate viewers on demand as requested by another plugin.
It seems reasonable that a user may want both the viewer that's generated by another plugin that depends on ixbrl-viewer (with whatever js it's using) along with the default ixbrl-viewer. In that case we need to handle requirement 4 through configuration and not universal control by another plugin.
The GUI can already be disabled by unselecting Launch on Load
from the tools menu and I think we can update the CLI hook to exit if --save-viewer
isn't used (saveViewerDest
isn't set). Would that satisfy your requirements?
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.
EdgarRenderer has a browser menu with multiple options for the launched viewer, such as redline behavior, etc. I think in that configuration we don't want the regular plugin tools menu addition as well.
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.
If you enable EdgarRenderer there's no longer anyway to get the latest Arelle viewer js? That will come as a surprise for users that enable both the EdgarRenderer and Arelle viewers (there are many).
If we go forward with this path (I don't think we should) we would need a notice in the GUI and CLI that enabling the EdgarRenderer overrides the Arelle viewer and if you want the latest and greatest Arelle viewer you must disable the EdgarRenderer.
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.
View (menu ideas)
EdgarRenderer
Show Filing Data (launch ixviewer-plus)
other options
Arelle Viewer
Show Filing Data (launch arelle viewer)
Edgar 23.4 version (1.14.09)
Edgar preview version (1.14.10)
Latest public version (1.99.99)
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 think we can update the CLI hook to exit if
--save-viewer
isn't used (saveViewerDest
isn't set).
I think that's already the case - if you don't specify --save-viewer
it doesn't create a viewer (see
https://github.com/Arelle/ixbrl-viewer/blob/master/iXBRLViewerPlugin/__init__.py#L146)
EdgarRenderer has a browser menu with multiple options for the launched viewer, such as redline behavior, etc. I think in that configuration we don't want the regular plugin tools menu addition as well.
I wonder if we could separate the viewer into a common module and a plugin? The common module could then be used by both the standard CLI/GUI plugin, and the EDGAR plugin. The EDGAR code could use the viewer code without getting the standard viewer CLI and GUI menus. I don't know if there's an existing mechanism for plugin dependencies, but the common module could just be an ixbrl-viewer-common
plugin that imports the code, but doesn't define any hooks.
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.
It would need to be dynamic, as some GUI users (and possibly cmd line workflows) interweave Edgar submissions with ESEF work, and wouldn't want to have a lot of clicks and the cmd line workflow may have to sequence through different disclosureSystems without reloading or reconfiguring.
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.
That's fine. If they want both EDGAR and standard iXBRL viewer functionality, they can load both plugins.
Reason for change
Description of change
iXBRLViewer.GenerateOnCall
returningTrue
blocks iXBRLViewer from the default workflow of generating on plugin methodCntlrWinMain.Xbrl.Loaded
orCntlrCmdLine.Filing.End
. Instead iXBRLViewer implements plugin methodiXBRLViewer.Generate
which application invokes to generate according to its workflow.iXBRLViewer.GenerateOnCall
iXBRLViewer.Generate
providing override file name for stub file (with .xhtml suffix) and blocking output of any iXBRL or other documents, twice as needed for private and public redacted outputs in same workflow (in different directory paths with different stub file names).CntlrWinMain.Xbrl.Loaded
operation.Steps to Test
Run Arelle with edgar24.0.1-preview branches. Check for presence of stub .xhtml files.
review:
@Arelle/arelle
@paulwarren-wk