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

Add Osmose Q/A layer #7095

Merged
merged 38 commits into from
Feb 4, 2020
Merged

Add Osmose Q/A layer #7095

merged 38 commits into from
Feb 4, 2020

Conversation

kymckay
Copy link
Collaborator

@kymckay kymckay commented Dec 7, 2019

Closes #5682

This is a functional Q/A layer with support for a reasonable number of issue types. Supporting new issues is as simple as giving them an icon entry in the qa_errors.json data file.

Some implementation notes:

  • Osmose issues are uniquely identified by a unique item and class combination (both integer values). To identify specific issues we concatenate as a string item-class.
  • Adding a new issue type to iD is as simple as adding an icon entry for the item-class in data/qa_errors.json.
  • When requesting issues for a tile, we use the item= filter since Osmose tiles can return a maximum number of issues (we want to show the user as many as possible).
  • All issue colors and static translation strings (besides UI headings) are pulled in from the Osmose API when the layer is first enabled. To reduce the overall data being requested, this is done using a langs= filter and as individual requests for each item-class (as Promises).
    • The API automatically falls back to English if a string is untranslated or if the requested locale doesn't exist in Osmose
  • The list of elements and a dynamic details string (contains information like keys and values of the issue) associated with each issue has to be requested individually - this is done upon UI creation.
  • The UI subsections (other than the issue description) are only created as needed, in order:
    • Description (always created)
    • Issue Details (the dynamic string)
    • Issue Elements (list of element links)
    • Fix Guidelines (string giving direction on how to fix the issue)
    • Common Mistakes (string giving information on traps to avoid while fixing)

Preview of the UI in action:
image

@kymckay kymckay added validation An issue with the validation or Q/A code wip Work in progress labels Dec 7, 2019
@kymckay

This comment has been minimized.

@quincylvania

This comment has been minimized.

@kymckay

This comment has been minimized.

@quincylvania

This comment has been minimized.

@kymckay

This comment has been minimized.

@kymckay

This comment has been minimized.

@kymckay kymckay changed the base branch from master to 2.x December 28, 2019 19:04
@kymckay

This comment has been minimized.

@quincylvania
Copy link
Collaborator

@SilentSpike Thanks for your continued work on this! Let us know if you need any advice/assistance and when this is ready for review.

There's also potential to use the provided translation strings for titles and descriptions - although I'm not sure they're all written in a way iD would typically present information to users.

Hmm we don't really have any other external translation integrations like this but it'd be worth looking into if there are are lots of strings.

@frodrigo
Copy link

For now the Osmose analyzers (backend) have more than 500 strings. But I am on integrating doc to transifex. Each class of issue will have a title, a description, a how to fix, a tips on trap to avoid and example plus a contextual message with placeholder. For now only title and contextual message are present. The process of adding the others is ongoing. The text with placeholder are already replaced and translated on Osmose API, the placeholder are not available at the API level.

Osmose analyzers also run JOSM validator and reuse the JOSM translation.

The Osmose API aka Frontend aka Website also contains issues reported by other parties. We have no the hand over thus texts and availability of translations.

@frodrigo
Copy link

@kymckay kymckay removed the wip Work in progress label Dec 31, 2019
@kymckay

This comment has been minimized.

@kymckay

This comment has been minimized.

@frodrigo
Copy link

frodrigo commented Jan 1, 2020

It is better to use "issue" than "error" on buttons.

The ignore button is not well named. It is not an ignore action, but flag the issue as false positive for the current user, but all the users. Then no more displayed.

@kymckay
Copy link
Collaborator Author

kymckay commented Jan 1, 2020

Easily changed, those are just reused strings from the other QA services. We actually seem to interchange "error" and "issue" in a few places would be good to make those all consistent.

@quincylvania

This comment has been minimized.

@frodrigo
Copy link

frodrigo commented Jan 2, 2020

@SilentSpike Okay, that's not too many. Let's continue with separate translations since it's easiest and allows us to use wording suited to iD mappers.

I am interested in improving the wording directly into Osmose. Plus it can help to avoid re-wording/translation in iD.

@frodrigo Thanks for your continued advice. When do you expect the v0.3 API to be stable?

When it will be ready! The API 0.3 was started in summer '18, so I do not know I will be completed. Sure, it will take some more months. But if you choose to use the 0.3beta we will pay attention to it. Some other few softwares already use it.

@kymckay

This comment has been minimized.

@frodrigo

This comment has been minimized.

@kymckay kymckay added the waitfor-upstream Waiting for something in an upstream project label Jan 8, 2020
@kymckay
Copy link
Collaborator Author

kymckay commented Jan 8, 2020

@frodrigo Had a quick discussion and we agree that it'd be ideal to use the Osmose strings directly and align our interests.

In terms of the API "wants" to make this possible for iD:

  1. Language filter to reduce data when requesting static strings for caching
  2. Long descriptions available via API - will these be dynamic strings or also static?

In the meantime I've forked the backend and will see about updating some of the English titles

@frodrigo

This comment has been minimized.

@jocelynj

This comment has been minimized.

@kymckay

This comment has been minimized.

@frodrigo

This comment has been minimized.

Also only add sections where appropriate
Providing a `langs` param that's currently not implemented so these will
all be English until that is done.
- Only requesting the data we need (Osmose has many more issue types
than iD will ever display)
Improves structure of the details UI and introduces the use of flexboxes
for this. Does not break UI for other error services with shared classes.
@quincylvania quincylvania self-requested a review February 4, 2020 18:41
Copy link
Collaborator

@quincylvania quincylvania left a comment

Choose a reason for hiding this comment

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

Thanks again, @SilentSpike, this looks awesome. I made a few nitpicky comments about UI stuff but the underlying functionality appears to work fine.

The one thing I'd like to hold back on for the moment is adding the closed issue changeset tags. Other than that we can address the UI stuff before or after merging.

data/core.yaml Outdated
elems_title: Issue Elements
fix_title: Fix Guidelines
trap_title: Common Mistakes
translation: Translations supplied by Osmose
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might just say "Translate", and maybe hide the button unless there are untranslated strings onscreen.

Separately, we should add text to iD's documentation about where you can translate the Osmose strings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe there's no way for us to check whether untranslated strings are present (since the API automatically falls back to English strings and they all get cached under the current language code).

My thinking with not using a "call to action" here is that if there are issues with the strings or missing translations then a user inclined to improve them knows where they come from - and if there aren't then it's still nice to acknowledge they're supplied by a 3rd party.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm we could fetch the English strings too and see if they're equivalent? The issue for me is that the string is sort of long and overly draws my attention. "Translations by Osmose" might be a small improvement. It'd also help to put the button under the grey box or to at least give it a greater margin.

Another issue is that I think we use a verb phrase everywhere else we use the outlink icon. Something like "View translations" would keep that while being less "asky". We could also use a different icon.

@quincylvania quincylvania added the new-feature A new feature for iD label Feb 4, 2020
@kymckay kymckay merged commit 4c843f6 into 2.x Feb 4, 2020
@kymckay kymckay deleted the osmose branch February 4, 2020 21:47
@quincylvania quincylvania added this to the Next Release milestone Feb 4, 2020
@quincylvania quincylvania modified the milestones: 3.0.0, 2.18.0 May 5, 2020
@frodrigo
Copy link

Just note that the API 0.3 of Osmose is now released. No change in the API, just it is now 0.3 in place of 0.3beta. The 0.3beta is still valid URL path and redirect to 0.3.

@kymckay
Copy link
Collaborator Author

kymckay commented Jul 17, 2020

Just note that the API 0.3 of Osmose is now released. No change in the API, just it is now 0.3 in place of 0.3beta. The 0.3beta is still valid URL path and redirect to 0.3.

Great to hear! I'll update the iD source to use the new path 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature A new feature for iD validation An issue with the validation or Q/A code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Osmose Q/A layer
5 participants