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

Job list page a bit broken #3436

Open
jmsmkn opened this issue Jul 11, 2024 · 18 comments · May be fixed by #3767
Open

Job list page a bit broken #3436

jmsmkn opened this issue Jul 11, 2024 · 18 comments · May be fixed by #3767
Assignees

Comments

@jmsmkn
Copy link
Member

jmsmkn commented Jul 11, 2024

The warning from the job (the last line of stderr) is no longer visible to the user who created the algorithm job (unless they have the view_logs permission, which they do not). This is used by algorithms to pass warnings such as, I completed, but the voxel size is outside the range this was trained on, etc.

@jmsmkn
Copy link
Member Author

jmsmkn commented Aug 14, 2024

I think this was caused by the changes to the CIV rendering, so assigning @chrisvanrun.

@chrisvanrun
Copy link
Contributor

On which page should this be visible: I am a bit confused about the described case.

AFAIK one should have view_logs to view any of the stderr/stdout and I cannot find a trace on the detail page on special filtering on the stderr. There is the special Job.error_message: that should already be visible.

@chrisvanrun
Copy link
Contributor

This was the detail page rework:

@jmsmkn
Copy link
Member Author

jmsmkn commented Aug 19, 2024

It should be visible on the list views. It was added in #1606 .

@chrisvanrun
Copy link
Contributor

I am truly sorry. Looking over the original introduction PR I can't find anything that I might call a warning or last line of stderr. I feel stupid for asking but could you link to the lines that introduced it?

@jmsmkn
Copy link
Member Author

jmsmkn commented Aug 19, 2024

I don't know. Does it really matter? It used to behave like this, it now doesn't.

@chrisvanrun
Copy link
Contributor

chrisvanrun commented Aug 19, 2024

It matters in such that I am not sure what is needed to address the issue or what the behaviour should be. The old implementation would tell me what is now missing.

I will implement something that will always show the last line of the stderr under the condition that the job has a SUCCESS state. Independent of view_logs permissions.

@jmsmkn
Copy link
Member Author

jmsmkn commented Aug 19, 2024

You can run an old version of Grand Challenge locally to see the old behaviour.

@jmsmkn
Copy link
Member Author

jmsmkn commented Aug 20, 2024

I think you're right. Lets not do it. However:

2024-08-20.09-28-08.mp4

@jmsmkn jmsmkn changed the title Job warnings are not visible to users Job list page a bit broken Aug 20, 2024
@chrisvanrun
Copy link
Contributor

The failure message modal is broken (at least on Safari). This is kind of crucial as it breaks the page.

Oh, that isn't very nice! I'll have a look into why it breaks. Pretty standard modal, but maybe the way the data table collapses causes it to not work. Yep! Confirmed.

The 'Result' column is intentionally showing the result, as was requested in #3495. It being so blown up is a direct result of not configuring the template to render something concisely:

<pre>{{ results|tojson(indent=2) }}</pre>

Not a huge fan of hiding the results behind a modal. They can click on the detail to get to actual results, so we might as well remove the results altogether then. Which I think is not something we'd want.

The 'new' way of showing results.json are Int and Float output interfaces which are nicely previewed in the results:

image

@jmsmkn
Copy link
Member Author

jmsmkn commented Aug 22, 2024

<pre>{{ results|tojson(indent=2) }}</pre>

That is exactly the template used by that algorithm, which causes the blow up. IIRC previously the results were in a modal. Opening a new page takes the user away from the table, they lose all their sorting, filtering and page. Putting it in a modal means that they could quickly compare multiple results, and the algorithm editors could present more detail about a result than would fit in a tiny column.

@chrisvanrun
Copy link
Contributor

chrisvanrun commented Aug 29, 2024

\<pre\>{{ results|tojson(indent=2) }}\</pre\>

That is exactly the template used by that algorithm, which causes the blow up. IIRC previously the results were in a modal. Opening a new page takes the user away from the table, they lose all their sorting, filtering and page. Putting it in a modal means that they could quickly compare multiple results, and the algorithm editors could present more detail about a result than would fit in a tiny column.

The results were always shown on top of the status badge, the status badge would have a modal that would have a strange mix of errors, CIVs etc.

I'll whip together a changeset that tries to address these issues:

  • Modal not working when collapsed
  • Error modal access in earlier column
  • Result template behind modal + access in an earlier column

I'll likely just add them to the state column.

@chrisvanrun
Copy link
Contributor

Modal isn't showing because the column' HTML-content is copied and then hidden via display: none;. This messes up the modal IDs since there are now two targets.

Quick fix, possibly a setting that has Datatables remove the column from the DOM. Or alt, implement the scrolling. Or alt, dirty fix, define the modal in the first column (won't work for the CIV representations).

@jmsmkn
Copy link
Member Author

jmsmkn commented Aug 29, 2024

Stuffing things in earlier columns is hacky. Needs a proper fix.

@chrisvanrun
Copy link
Contributor

Cycle work started. Going to poste some intermediate constructs I made here:

$("#GCModal").on("show.bs.modal", function (event) {
    console.log("GC MODAL")
    const button = $(event.relatedTarget);
    const modal = $(this);

    function copyFromTemplate(sourceAttr, targetClass) {
        const modalEl = modal.find(`.${targetClass}`);

        // Clear the modal element before adding new elements
        modalEl.empty();

        const source = button.find(`[${sourceAttr}]`).first();
        if (typeof source !== "undefined") {

            const children = source.children();
            const deep = true;

            children.each(function () {
                const newEl = this.cloneNode(deep);
                modalEl.append(newEl)
            });
        }
    }

    copyFromTemplate("data-modal-title", "modal-title");
    copyFromTemplate("data-modal-body", "modal-body");
    copyFromTemplate("data-modal-footer", "modal-footer");
});

The HTML for modal_base.html, from CIV representations:

<a href="GCModal"
   data-toggle="modal"
   data-target="#GCModal"
   class="text-decoration-none"
   role="button"
   title="View {{ civ.interface.title }}"
>
    <div class="badge badge-primary">
        <i
                class="fas fa-fw fa-eye"
        ></i>
        {{ civ.interface.title|truncatechars:64 }}
    </div>
    {% block preview %}{% endblock %}

    <div class="d-none">
        <div data-modal-title>
            {{ civ.interface.title }}
        </div>
        <div data-modal-body>
            {% block content %}{% endblock %}
        </div>
    </div>
</a> 

The partial template:

<div class="modal" id="GCModal" tabindex="-1" role="dialog"
     aria-hidden="true">
    <div class="modal-dialog modal-dialog-centered modal-lg" role="document">
        <div class="modal-content">
            <div class="modal-header">
                <h5 class="modal-title">
                </h5>
                <button type="button" class="close" data-dismiss="modal"
                        aria-label="Close">
                    <span aria-hidden="true">&times;</span>
                </button>
            </div>
            <div class="text-left modal-body"></div>
        </div>
    </div>
</div>

@chrisvanrun
Copy link
Contributor

Summary: the initial PR with the solution entailed a rework of how the datatables handle overflow, it introduced a small JS dependency to have the horizontal scrollbar render on the bottom of the current view port of the table (instead of way at the bottom, out of sight).

The reason for doing that is that the Datables JS library currently handles overflow strangely by copying instead of moving the HTML nodes, causing havoc with element identifiers on which some other JS libraries depend (e.g. modals).

The initial PR got shot down though, partially because of the turbulent cycle. I still think it's the better solution to change the Datatables behaviour, rather than patching modals to have them work with duplicate node IDs.

If the opinion is however still to patch the modals, I'll pick that up again.

@jmsmkn
Copy link
Member Author

jmsmkn commented Nov 28, 2024

They're two separate issues. The visible scrollbar could be a solution for #3208, but a PR needs to be made just for that issue as it affects almost every table on the site, so each one would need to be checked (we have no Playwright tests for those). Once that is done then come back to this, it may be solved at the same time but decide whether you want to spend time on fixing the model here, or manually reviewing each table.

@chrisvanrun
Copy link
Contributor

How does this look? I am not sure about having the truncated error message in the error.

Screen.Recording.2024-12-24.at.16.28.39.mov

@chrisvanrun chrisvanrun linked a pull request Dec 27, 2024 that will close this issue
@chrisvanrun chrisvanrun linked a pull request Dec 27, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants