-
Notifications
You must be signed in to change notification settings - Fork 0
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
Additional checks interface (add quotes to string values, make result table columns consistent) #13
Comments
I've put on board and we'll see how much we can do - even just grouping them would be good. |
A first go at grouping is now in - as each type now has separate tables, this also allows someone to start checking which columns come up for each type of error and to specify which columns should come up. Try uploading the files in https://github.com/Open-Telecoms-Data/lib-cove-ofds/tree/8db98fd56cf2a5fe8a2563160c27ec3209bbba04/tests/fixtures/0_1_0_alpha |
Thanks! I like the approach of having separate tables for checks that can relate to multiple objects. I think that's better than the 'Object identifier' approach in the issue description: I've specified the columns that should appear in the results table for each check type and the values for each column in the additional checks spreadsheet. I've also noted some things that need updating:
I've opened a couple of other issues related to the results interface: |
Show *_reference_name_set_but_not_in_original in the same block as *_name_does_not_match errors #13 (comment)
Please can you add the following introductory sentence to the additional checks box:
bf7f167: please can you include the link to the schema docs as part of the feedback description paragraph, e.g. for the Node location type check:
I've updated the feedback description column in the additional checks spreadsheet with the exact text to use for each check. |
#13 Also fix broken if clause - some errors would not have been shown Not sure how to do trans blocks correctly but it's not important for now so leaving for later
#13 Also fix broken if clause - some errors would not have been shown Not sure how to do trans blocks correctly but it's not important for now so leaving for later
all on 2022-11-08
Getting this exactly right really depends on some good design work that should really be done for the whole site - for now I've just changed h3's to h4's and it kinda works. on 2022-11-08 |
Looks good on 2022-11-08
Can't test due to Open-Telecoms-Data/lib-cove-ofds#5 (comment)
Good enough for now, but agree that design work is needed outside of this sprint. |
The priority outstanding action for this issue is adding the necessary columns to the results tables for each check, from #13 (comment):
|
Next version of library will have everything but path - that's a harder one to add. EDIT this is now on live! (Whoops, I have phase ref name and organsation ref name but I've missed phase name and organsation name - I'll come back to those) |
Are the columns in the correct order? eg span_start_node_not_found says "Network identifier (.id) / Span identifier (Span.id) / Path (e.g. /spans/0/start) / Node reference (Span.start | Span.end)" and that's the order the 4 columns should be in? I'm not going to re-order them today, but it's just HTML tables in cove_ofds/templates/cove_ofds/additional_checks_table.html so this is an easy task for me or someone to pick up later. |
Looking good so far!
Yep, please order the columns as specified in the spreadsheet. |
Columns are in correct order and all columns specified in spreadsheet should now be on live - please test! |
Looks good, thanks! A couple of outstanding tasks from earlier comments:
And some minor changes for consistency:
|
Ticking backticks off as that already has a separate issue: #35 |
Ticking off show/hide comment, as that is moved to #65 |
All the label changes are ready for testing on live |
The changes look good. Still need to add quotes to string values. Also, whilst testing I noticed that the columns for the result tables for the organisation references check are inconsistent: Please can you remove the 'field' column from the results tables for nodes and spans? Edit: I've renamed the issue to reflect the outstanding tasks. |
Ready for testing on live |
Looks good. Thanks! |
Building on the check documentation in Open-Telecoms-Data/lib-cove-ofds#8 (comment), I've been thinking about how we display additional check results to users and have sketched up the following interface (colours, fonts, sizes are all placeholders, but hopefully it gives an idea):
My thinking is:
@odscjames is it feasible to implement something like this as part of the current sprint?
@lgs85 please take a look and share your feedback/suggestions (I'm definitely not a UI expert!)
The text was updated successfully, but these errors were encountered: