Skip to content
This repository has been archived by the owner on Nov 14, 2022. It is now read-only.

AR-1473 #83

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

AR-1473 #83

wants to merge 10 commits into from

Conversation

Dagonite
Copy link
Contributor

@Dagonite Dagonite commented Nov 12, 2021

No description provided.

@codecov
Copy link

codecov bot commented Nov 12, 2021

Codecov Report

Merging #83 (0ee0e80) into main (76f7b95) will decrease coverage by 0.63%.
The diff coverage is 65.38%.

❗ Current head 0ee0e80 differs from pull request most recent head cd9cb91. Consider uploading reports for the commit cd9cb91 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
- Coverage   79.80%   79.16%   -0.64%     
==========================================
  Files          54       54              
  Lines        1882     1901      +19     
==========================================
+ Hits         1502     1505       +3     
- Misses        380      396      +16     
Flag Coverage Δ
webapp-integration 59.70% <63.46%> (-0.29%) ⬇️
webapp-pages 76.38% <65.38%> (-0.62%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
autoreduce_frontend/autoreduce_webapp/views.py 100.00% <ø> (ø)
...toreduce_frontend/reduction_viewer/views/common.py 77.89% <50.00%> (-16.14%) ⬇️
...uce_frontend/reduction_viewer/views/run_summary.py 89.55% <85.71%> (ø)
...autoreduce_webapp/templatetags/colour_table_row.py 100.00% <100.00%> (+6.66%) ⬆️
autoreduce_frontend/reduction_viewer/view_utils.py 96.77% <100.00%> (ø)
...ntend/reduction_viewer/views/configure_new_runs.py 98.18% <100.00%> (ø)
...educe_frontend/reduction_viewer/views/runs_list.py 91.52% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76f7b95...cd9cb91. Read the comment docs.

@Dagonite Dagonite force-pushed the 1473-drop-down-menu-AR-1473 branch from 30ce2d6 to 1cc4c09 Compare December 24, 2021 13:33
@Dagonite Dagonite force-pushed the 1473-drop-down-menu-AR-1473 branch from 1cc4c09 to cd9cb91 Compare December 24, 2021 13:40
Copy link
Contributor

@dtasev dtasev left a comment

Choose a reason for hiding this comment

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

I seem to not be able to get the right remote variable syntax, in order to get it to properly render, does this look ok?

    "remote variable": {
        "url":
        "https://raw.githubusercontent.com/autoreduction/autoreduce/master/container/",
        "default": "qp_mantid_python36.D"
    }

I'm testing it locally a bit more so might leave further comments

"""
vars_kwargs = arguments.as_dict()
fetch_api_urls(vars_kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be done after the _combine_dicts calls, and called on each standard_vars and advanced_vars

@@ -92,50 +111,83 @@ def prepare_arguments_for_render(arguments: ReductionArguments, instrument: str)
return final_standard, final_advanced, variable_help


def fetch_api_urls(vars_kwargs):
"""Convert file URLs in vars_kwargs into API URL strings."""
for category, headings in vars_kwargs.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

After calling it on standard_vars and advanced_vars you'll have to remove this level of nesting and the [category] part from code below

Comment on lines +140 to +141
except Exception:
pass
Copy link
Contributor

@dtasev dtasev Dec 24, 2021

Choose a reason for hiding this comment

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

Also this should be logged as logger.error (check logger imports in other files), might be also useful to have a traceback, something like

logger.error("Failed to fetch file from GitHub: %s\n%s", str(err), traceback.format_exc())

<option value="True" {% if value.lower == 'true' %} selected {% endif %}>True</option>
<option value="False" {% if value.lower == 'false' %} selected {% endif %}>False</option>
</select>
{% elif "file" in name %}
Copy link
Contributor

@dtasev dtasev Dec 24, 2021

Choose a reason for hiding this comment

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

This will only work for variable with "file" in their name, it should be something like {% elif "url" in variable.current %}

Copy link
Contributor

@dtasev dtasev left a comment

Choose a reason for hiding this comment

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

A couple more suggested changes, I've pushed a branch containing them at 0ee0e80

Comment on lines +132 to +133
auth_token = os.environ.get("AUTOREDUCTION_GITHUB_AUTH_TOKEN", "")
req = requests.get(f"{url}/{default}", headers={"Authorization": f"Token {auth_token}"})
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't submit the request for me when the token is empty, I had to change it to this to work

auth_token = os.environ.get("AUTOREDUCTION_GITHUB_AUTH_TOKEN", None)
headers = {"Authorization": f"Token {auth_token}"} if auth_token else {}
req = requests.get(f"{url}/{default}", headers=headers)

@@ -94,56 +113,83 @@ def prepare_arguments_for_render(arguments: ReductionArguments, instrument: str)
return final_standard, final_advanced, variable_help


def fetch_api_urls(vars_kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Managed to get this function working with a few changes, I don't have a patch so I'll just paste it below.

I've also removed the second requests.get as we're not displaying the value anywhere. This saves a lot of API requests and we can just link to the github file directly if the user wants to see it!

def fetch_api_urls(vars_kwargs):
    """Convert file URLs in vars_kwargs into API URL strings."""
    for _, heading_value in vars_kwargs.items():
        if isinstance(heading_value["current"], dict) and "url" in heading_value["current"]:
            try:
                heading_value["all_files"] = {}
                base_url, _, path = heading_value["current"]["url"].partition("master")
                # TODO might want to support >1 origin, or allow different syntax
                repo = base_url.replace("https://raw.githubusercontent.com/", "")[:-1]  # :-1 drops the trailing slash
                path = path.lstrip("/")
                url = f"https://api.github.com/repos/{repo}/contents/{path}"
                auth_token = os.environ.get("AUTOREDUCTION_GITHUB_AUTH_TOKEN", None)
                headers = {"Authorization": f"Token {auth_token}"} if auth_token else {}
                req = requests.get(url, headers=headers)
                data = json.loads(req.content)

                for link in data:
                    file_name = link["name"]
                    # if this download_url is None, then it's a directory
                    if link["download_url"]:
                        url, _, default = link["download_url"].rpartition("/")
                        # req = requests.get(f"{url}/{default}", headers=headers)
                        # value = req.text
                        heading_value["all_files"][file_name] = {
                            "url": url,
                            "default": default,
                            # "value": value,
                        }
            except Exception as err:
                logger.error("Failed to fetch file from GitHub: %s\n%s", str(err), traceback.format_exc())

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants