-
Notifications
You must be signed in to change notification settings - Fork 34
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
fix the api part of #531 by providing an api call to export all CREs … #532
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
import io | ||
import logging | ||
from copy import deepcopy | ||
from typing import Any, Dict, List, Optional | ||
from typing import Any, Dict, List, Optional, Set | ||
import os | ||
import gspread | ||
import yaml | ||
|
@@ -237,3 +237,63 @@ def write_spreadsheet(title: str, docs: List[Dict[str, Any]], emails: List[str]) | |
for email in emails: | ||
sh.share(email, perm_type="user", role="writer") | ||
return "https://docs.google.com/spreadsheets/d/%s" % sh.id | ||
|
||
|
||
def generate_mapping_template_file( | ||
database: db.Node_collection, docs: List[defs.CRE] | ||
) -> str: | ||
maxOffset = 0 | ||
related = set() | ||
|
||
def add_offset_cre( | ||
cre: defs.CRE, database: db.Node_collection, offset: int, visited_cres: Set | ||
) -> List[Dict[str, str]]: | ||
nonlocal maxOffset, related | ||
maxOffset = max(maxOffset, offset) | ||
rows = [] | ||
|
||
rows.append( | ||
{f"CRE {offset}": f"{cre.id}{defs.ExportFormat.separator.value}{cre.name}"} | ||
) | ||
visited_cres.add(cre.id) | ||
dbcre = database.get_CREs(external_id=cre.id) | ||
if not dbcre: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is not dbcre true if dbcre is empty or if there has been an error in get_CREs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dbcre should be None in case of error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does not None resolve to true? |
||
raise ValueError(f"CRE with id {cre.id} not found in the database") | ||
cre = dbcre[0] | ||
for link in cre.links: | ||
if ( | ||
link.document.doctype == defs.Credoctypes.CRE | ||
and link.document.id not in visited_cres | ||
): | ||
if link.ltype == defs.LinkTypes.Contains: | ||
rows.extend( | ||
add_offset_cre( | ||
cre=link.document, | ||
database=database, | ||
offset=offset + 1, | ||
visited_cres=visited_cres, | ||
) | ||
) | ||
elif link.ltype == defs.LinkTypes.Related: | ||
related.add(link.document.id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently this code is doing nothing with related CRES, but we need to have them in the template at some point, for the template to be usable for changing the catalog. We have two options: add them as id|name list in one extra column, or add them as rows, just like we're adding the children (contains) as rows. I think the latter is easier to read but more confusing. So I vote for one column for the related CRES. This would require building up a dictionary of the related id|name pairs, and then adding them to rows, together with the CRE in a dictionary of two (or one if there is no related CRES. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree
We could have a column named "Related CRE" sure, Since we want to be capturing more complex relationships in a 2 dimensional matrix I think we should consider a more verbose and less context-based format. e.g.
In any case, this is beyond the scope of this particular pull request, let's think about this a bit more, let me know when you are free for a brainstorming session There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mapping template specifies with which CRE to link standard entries. The related CREs will not get the standard entries aside. I see your point. I guess it can be an option that is off by default to include the related CREs column in the template. So yes: two formats. and indeed not for this PR. |
||
return rows | ||
|
||
visited_cres = set() | ||
csv: List[Dict[str, str]] = [] | ||
|
||
for cre in docs: | ||
csv.extend( | ||
add_offset_cre( | ||
cre=cre, database=database, offset=0, visited_cres=visited_cres | ||
) | ||
) | ||
result = [{f"CRE {offset}": "" for offset in range(0, maxOffset + 1)}] | ||
result.extend(csv) | ||
|
||
orphaned_documents = [doc for doc in related if doc not in visited_cres] | ||
if len(orphaned_documents): | ||
raise ValueError( | ||
"found CREs with only related links not provided in the root_cre list, unless you are really sure for this use case, this is a bug" | ||
) | ||
|
||
return result |
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 name get_CREs is unclear about what you're getting and so is dbcre. You're looking for linked CREs, right? Rename?