fix the api part of #531 by providing an api call to export all CREs … #532
Conversation
northdpole
force-pushed
the
feature-531-offseted-cre-csv
branch
from
a1e4b9c to
090ba95
Compare
|
|
||
| rows.append({f"CRE{offset}-ID": cre.id}) | ||
| visited_cres.add(cre.id) | ||
| dbcre = database.get_CREs(external_id=cre.id) |
There was a problem hiding this comment.
this name get_CREs is unclear about what you're getting and so is dbcre. You're looking for linked CREs, right? Rename?
| rows.append({f"CRE{offset}-ID": cre.id}) | ||
| visited_cres.add(cre.id) | ||
| dbcre = database.get_CREs(external_id=cre.id) | ||
| if not dbcre: |
There was a problem hiding this comment.
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.
dbcre should be None in case of error
There was a problem hiding this comment.
Does not None resolve to true?
| ) | ||
| ) | ||
| elif link.ltype == defs.LinkTypes.Related: | ||
| related.add(link.document.id) |
There was a problem hiding this comment.
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.
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.
I agree
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
We could have a column named "Related CRE" sure,
then we can add the CRE listed in this column with a related type link,
During importing tehn we have the hierarchy on the left, then one column with related CREs, then an arbitrary number of standard entries. Do we link each standard to both the normal cre and the related one? This seems a bit confusing for people.
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.
- perhaps there should be 2 formats, 1 for mapping standards and one for links between cres
- alternatively perhaps everything should be considered a standard, CREs-included, we forget the hierarchy and just support a link between only 2 standards for each CSV, these 2 standards are CRE and any standard, for this we need to explicitly include the relationship type in a column.
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.
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.
northdpole
force-pushed
the
feature-531-offseted-cre-csv
branch
from
9212b92 to
a139636
Compare
…in an instance in a visually pleasing format
northdpole
force-pushed
the
feature-531-offseted-cre-csv
branch
from
a139636 to
173e7d6
Compare
…in an instance in a visually pleasing format
This produces spreadsheets like this one https://docs.google.com/spreadsheets/d/10jrVWRRw-zCVFVgzumnO7xgNTjZYU6M6B9EDqss9zxY/edit?usp=sharing
Todo: