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

Implement a page object accessibility dialog #17225

Merged
merged 2 commits into from
Apr 17, 2024

Conversation

jmchilton
Copy link
Member

This is trickier than it sounds in some ways because objects may be sources of permission in different ways. Histories need to be accessible for invocations or collections or jobs - datasets may be referenced in different ways. The APIs for histories and workflows is pretty different than for datasets also. I think I've managed to give it a unified feel though.

Screenshot 2023-12-21 at 2 43 46 PM

xref #13926 - it doesn't fix the issue yet but it is a component I want to foreground after creating a page from an invocation. I think this PR is 90% of the work.

Builds on all my markdown working branch.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@jmchilton jmchilton force-pushed the page_permissions branch 2 times, most recently from 1d78c78 to 09cd46a Compare December 22, 2023 22:24
@jmchilton jmchilton force-pushed the page_permissions branch 4 times, most recently from f572080 to afe80ec Compare January 4, 2024 17:04
@jmchilton jmchilton force-pushed the page_permissions branch 3 times, most recently from ca110bc to 2d12480 Compare January 15, 2024 18:45
@jmchilton jmchilton marked this pull request as ready for review January 15, 2024 18:49
@github-actions github-actions bot added this to the 24.0 milestone Jan 15, 2024
@jdavcs
Copy link
Member

jdavcs commented Feb 26, 2024

@jmchilton, can you resolve the conflict, please?

@jmchilton jmchilton modified the milestones: 24.0, 24.1 Mar 5, 2024
@jmchilton jmchilton force-pushed the page_permissions branch 2 times, most recently from dea1d92 to df71f29 Compare March 28, 2024 22:45
@jmchilton jmchilton force-pushed the page_permissions branch 2 times, most recently from 09bd415 to 554c868 Compare April 2, 2024 18:10
Copy link
Contributor

@davelopez davelopez left a comment

Choose a reason for hiding this comment

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

Looks amazing! but unfortunately, I found an issue with datasets.

I created a page out of a workflow report and included a couple of markdown directives, but it seems to be an issue retrieving the dataset's name (although they show up correctly in the modal) 🤔

MarkdownObjectPermissionsDatasetBug

client/src/api/datasetCollections.ts Show resolved Hide resolved
@jmchilton jmchilton marked this pull request as draft April 15, 2024 16:11
@jmchilton
Copy link
Member Author

I've tried deleted, purged, datasets in a different history, etc... and I cannot reproduce this locally. I get a very fun error if I don't own the dataset - but it is the infinite loop that I think is being worked on elsewhere - https://github.com/galaxyproject/galaxy/pull/17818/files.

The problem with name is not the history dataset name but the name of a permission object. Your API isn't returning what mine is for some reason. The relevant block is:

function initHistoryDatasetData() {
    for (const historyDatasetId of referencedHistoryDatasetIds.value) {
        fetchDataset({ id: historyDatasetId });
        if (historyDatasetId && !(historyDatasetId in historyDatasetAccessible.value)) {
            axios
                .get(withPrefix(`/dataset/get_edit?dataset_id=${historyDatasetId}`))
                .then((response) => {
                    const permissionInputs = response.data.permission_inputs;
                    const accessPermissionInput = permissionInputs[1];
                    if (accessPermissionInput.name != "DATASET_ACCESS") {
                        throw Error("Galaxy Bug");
                    }
                    const accessible = (accessPermissionInput.value || []).length == 0;
                    Vue.set(historyDatasetAccessible.value, historyDatasetId, accessible);
                })
                .catch((e) => {
                    const errorMessage = errorMessageAsString(e);
                    const title = "Failed to fetch dataset metadata.";
                    toast.error(errorMessage, title);
                    Vue.set(historyDatasetAccessible.value, historyDatasetId, `${title} Reason: ${errorMessage}.`);
                });
        }
    }
}

The exception is being thrown here I think: if (accessPermissionInput.name != "DATASET_ACCESS") {

Is there any chance I can get you to tell me what the console is reporting the API response is for offending axios request?

axios.get(withPrefix(`/dataset/get_edit?dataset_id=${historyDatasetId}`))

Does the edit permissions page when you click the pencil in the history work for the offending datasets in your case?

@davelopez
Copy link
Contributor

davelopez commented Apr 16, 2024

Unfortunately, I purged that history when testing, however I could get the response to /dataset/get_edit for one of the datasets:

{
    "display_name": "Dataset A",
    "message": null,
    "status": null,
    "dataset_id": "40002f5e6ec8c7e7",
    "metadata_disable": false,
    "attribute_inputs": [
        {
            "name": "name",
            "type": "text",
            "label": "Name",
            "value": "Dataset A"
        },
        {
            "name": "info",
            "type": "text",
            "area": true,
            "label": "Info",
            "value": "uploaded txt file"
        },
        {
            "name": "annotation",
            "type": "text",
            "area": true,
            "label": "Annotation",
            "optional": true,
            "value": null,
            "help": "Add an annotation or notes to a dataset; annotations are available when a history is viewed."
        },
        {
            "type": "select",
            "multiple": false,
            "optional": true,
            "name": "dbkey",
            "label": "Database/Build",
            "options": [
                [
                    "unspecified (?)",
                    "?",
                    true
                ],
                [
                    "A. gambiae Feb. 2003 (IAGEC MOZ2/anoGam1) (anoGam1)",
                    "anoGam1",
                    false
                ],
               ... // Trimming huge list of options for readability
            ],
            "value": "txt",
            "help": "This will change the datatype of the existing dataset but not modify its contents. Use this if Galaxy has incorrectly guessed the type of your dataset."
        }
    ],
    "datatype_disable": false,
    "permission_inputs": [
        {
            "name": "not_shareable",
            "type": "hidden",
            "label": "The dataset is stored on private storage to you and cannot be shared.",
            "readonly": true
        }
    ],
    "permission_disable": true
}

Trying with a new history it seems to work as expected, so maybe I was so unlucky I tried with a history in a "bad state"? Or maybe is the permissions_inputs because that dataset was stored in a private object store 🤔

Let me know if I can provide more info, but I don't think this should hold the PR.

Update:

That was it, relocating one of the datasets to a private object store makes the error reproducible. Which kind of makes sense 😅

@jmchilton
Copy link
Member Author

Perfect - thanks for tracking that down... and frankly thanks for running with a obscure object store that should help find a lot of things. I'll revise this with a fix!

@davelopez
Copy link
Contributor

and frankly thanks for running with a obscure object store that should help find a lot of things.

It was just a coincidence, but I will use it more often to test stuff 😆

@jmchilton
Copy link
Member Author

Screenshot 2024-04-16 at 12 10 33 PM

@jmchilton jmchilton marked this pull request as ready for review April 16, 2024 18:37
Copy link
Contributor

@davelopez davelopez left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you very much!

@davelopez davelopez merged commit 8e900c5 into galaxyproject:dev Apr 17, 2024
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants