Dev

[sarraha786]

Before the 13th of January, Marc wants the CLOSUP website to be able to handle City of Brea (attached) template and make sure the empty template, conversion process, iXBRL viewer has no issues. I believe there are no major issues on the dev branch and want to see if we can merge it into main before this deadline.

The CLOSUP website has been updated so that instead of being embedded, you click on the 'Get Started' button and it opens a new tab with the main branch deployment on Heroku.

The following conversion works on dev branch
City of Brea, CA.xlsx

What we want to review (on different browsers too) @kwheelan @sarraha786

• UI/UX is right (following conversion)
• Downloading button, uploading, converting, deleting files, opening iXBRL viewer all work
  • KW: Yes, but download does not work for all versions of Chrome.
• Make sure CLOSUP website loads the tool properly
  • KW: Links to main deployment but this will work once this PR is merged.
• Make sure the empty ACFR template is up to date

Changes

• Closes Hide messages when starting new upload #132
• Closes New tab opens blank #134
• Closes Blocked pop-up #140
• Closes Viewer not fully loading #161
• Removes submodules and sets arelle and ixbrl-viewer to be static releases.
• Fixes an issue with negatives in the reconciliation table without breaking XBRL parsing of negative numbers or dollar signs

Remaining issues

• Multiple document order error #133
• Download blocked #139
• Package versions #163
• Empty lines #164
• Domain issues #165
• Convert prints to error statements #166

[lucakato]

@kwheelan Please check again, with the latest iXBRL viewer ver it should work.
Note: I think something somehow broke with main too even though we haven't touched it, which I find odd.

[kwheelan]

@lucakato

Ixbrl viewer works for me now.

The issue with both main and dev is that they are set to download the most recent release of both ixbrl-viewer and arelle, which can break our code.

I will add this to the review, but we should instead set ixbrl-viewer and arelle to a static version. At one point I had fixed the versions to

arelle_version = "2.17.5"
ixbrl_viewer_version = "1.4.9"

But if it now works with the most recent release of both, let's just set the versions to those release numbers.

I would suggest doing this in the .gitmodules file, which will automatically download the correct version as submodules for Heroku. I see that you added and changed the versions in requirements.txt in e41b99a and 84b742b. Having arrelle-release in there will download the most recent release of the package. I think you should be able to delete that from the requirements file completely. Just make sure you manually clone and initialize the same versions of Arelle and ixbrl-viewer in your personal environment by running git submodule update --init --recursive. Let me know if you have any issues with that.

In Chrome, the download still doesn't work for me (and I don't have any 3rd party pop-up blockers, just the most recent version of Chrome). Safari gives me an warning popup but allows me to download. Basically #139 from November is still unfixed.

When I upload multiple files, they still don't get reordered according to the list order when I drag and drop. (Ie. #133 is still an issue).

A new (minor) issue:
There are some extra empty lines that render where they shouldn't, especially before the 'Notes' and in the reconciliation tables:

I think we probably don't want a "Note:" to render at all unless there is some text in the note, but that's something to double-check with Marc.

[kwheelan]

@lucakato

Most of the above comments are issues with main as well, so I'll finish a quick code review this afternoon and go ahead and merge even without all of these issues resolved.

[lucakato]

@kwheelan
where are you getting the following versions?

arelle_version = "2.17.5"
ixbrl_viewer_version = "1.4.9"

The versions I have installed in my venv are:

Arelle Version: 2.36.0
iXBRL viewer version: 1.4.44

but when I create my iXBRL viewer in xbrl_processing.py I am using the 1.4.48 as follows:

viewer_url = "https://cdn.jsdelivr.net/npm/[email protected]/iXBRLViewerPlugin/viewer/dist/ixbrlviewer.js"

[kwheelan]

@lucakato

Before I merge, please revert changes to the Cell class and the table.html template file that cause XBRL parsing errors for negative values and values with dollar signs (see my comments).

The other edits and my comments from before are less essential, so I'll create issues for them. Just let me know when you fix the cell error and I will merge into main.

[kwheelan]

@kwheelan where are you getting the following versions?

arelle_version = "2.17.5"
ixbrl_viewer_version = "1.4.9"

The versions I have installed in my venv are:

Arelle Version: 2.36.0
iXBRL viewer version: 1.4.44

but when I create my iXBRL viewer in xbrl_processing.py I am using the 1.4.48 as follows:

viewer_url = "https://cdn.jsdelivr.net/npm/[email protected]/iXBRLViewerPlugin/viewer/dist/ixbrlviewer.js"

From a very old version of the code: 05358d4#diff-b10564ab7d2c520cdd0243874879fb0a782862c3c902ab535faabe57d5a505e1L145. I maintained those versions by deploying the Heroku branch from my CLI and initializing the submodules from there. (The main branch Heroku deployment should still work because I think it's running these old versions).

Notice that the repos in the dependencies folder are older commits: https://github.com/closup/process-xbrl/tree/main/dependencies. Looks like ixbrl-viewer got updated in commit 1120042 but arelle is still a version from 11 months ago).

Your new version of arelle is probably because you downloaded the arelle package using pip rather than cloning the repo. If this automatically downloads ixbrl-viewer, maybe this is a better strategy than having submodules at all? I will mess with it in the branch I just created for #163

[kwheelan]

@lucakato I fixed the versioning issue -- your strategy of just downloading the python packages of arelle and ixbrl-viewer is much cleaner than dealing with the submodules. I just deleted all the submodule stuff and set the versions for the two packages in requirements.txt. We eventually need to make sure we specify versions for all the package requirements in that file. (See issue #163).

For this PR, let's just revert the cell.py error and merge. I made issues for everything else.

[lucakato]

@kwheelan I reverted to using show_value() on branch #158. I believe the main reason I did this was because when we were testing it out with a bunch of new templates, there was an issue where some negative values did not have a bracket around it. Like for the table below it's not clickable, and the value (918,156) is not appearing in brackets as expected. So I made changes but didn't notice it affected fact-value.

I have a new branch #158. Also here's the TX file (not super up to date since Ram did not edit Sarrah's abs latest file
Pittsburg.Excel Final.xlsx

If I revert I believe the fact-value on iXBRL viewer should work. But it doesn't handle cases such as the above.

[kwheelan]

@kwheelan I reverted to using show_value() on branch #158. I believe the main reason I did this was because when we were testing it out with a bunch of new templates, there was an issue where some negative values did not have a bracket around it. Like for the table below it's not clickable, and the value (918,156) is not appearing in brackets as expected. So I made changes but didn't notice it affected fact-value.

I have a new branch #158. Also here's the TX file (not super up to date since Ram did not edit Sarrah's abs latest file Pittsburg.Excel Final.xlsx

If I revert I believe the fact-value on iXBRL viewer should work. But it doesn't handle cases such as the above.

Got it. The issue is because the reconciliation tables aren't taggable.

Both issues should be fixed if you adjust this section of app/templates/xbrl/table.html.

    <td id="{{ cell.id }}" class="{{ cell.td_class() }}">
        {% if cell.needs_ix_tag() %}
        {{ cell.prefix() }}<ix:nonFraction contextRef="{{ cell.context_ref() }}" name="{{ cell.xbrl_tag() }}" unitRef="USD" id="{{ cell.id }}" decimals="0" format="{{ cell.format() }}" {% if cell.sign() == 'negative' %} sign="-" {% endif %}>{{ cell.show_value() }}</ix:nonFraction>{{ cell.suffix() }}
        {% else %}
            {{ cell.show_value() }}
        {% endif %}

so that the last part (which deals with the reconciliation cells) is instead:

        {% else %}
            {{ cell.formatted_value() }}
        {% endif %}

but keep the reverted {{ cell.show_value() }} in line 18.

[lucakato]

@kwheelan I reverted to using show_value() on branch #158. I believe the main reason I did this was because when we were testing it out with a bunch of new templates, there was an issue where some negative values did not have a bracket around it. Like for the table below it's not clickable, and the value (918,156) is not appearing in brackets as expected. So I made changes but didn't notice it affected fact-value.
I have a new branch #158. Also here's the TX file (not super up to date since Ram did not edit Sarrah's abs latest file Pittsburg.Excel Final.xlsx
If I revert I believe the fact-value on iXBRL viewer should work. But it doesn't handle cases such as the above.

Got it. The issue is because the reconciliation tables aren't taggable.

Both issues should be fixed if you adjust this section of app/templates/xbrl/table.html.

    <td id="{{ cell.id }}" class="{{ cell.td_class() }}">
        {% if cell.needs_ix_tag() %}
        {{ cell.prefix() }}<ix:nonFraction contextRef="{{ cell.context_ref() }}" name="{{ cell.xbrl_tag() }}" unitRef="USD" id="{{ cell.id }}" decimals="0" format="{{ cell.format() }}" {% if cell.sign() == 'negative' %} sign="-" {% endif %}>{{ cell.show_value() }}</ix:nonFraction>{{ cell.suffix() }}
        {% else %}
            {{ cell.show_value() }}
        {% endif %}

so that the last part (which deals with the reconciliation cells) is instead:

        {% else %}
            {{ cell.formatted_value() }}
        {% endif %}

but keep the reverted {{ cell.show_value() }} in line 18.

thanks that worked and I pushed to #158-dev-review branch. I'll merge it with dev