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

Maproulette integration #1369

Merged
merged 23 commits into from
May 3, 2024
Merged

Maproulette integration #1369

merged 23 commits into from
May 3, 2024

Conversation

tannerwuster
Copy link
Contributor

@tannerwuster tannerwuster commented Apr 10, 2024

A revamp of the great work that @voscarmv accomplished on the PR

This PR is a solution to #1150 and will allow users to view and edit Map Roulette challenges within Rapid.
Screenshot 2024-04-10 at 10 17 53
Still to do:

  • Highlight entity that the challenge is editing
  • Highlight the current MapRoulette point when selected (like osm notes)
  • Add functionality to the I fixed it! , Can't Complete, Already Fixed and Not an Issue buttons
  • Add either a comment section or comment modal
  • A filter for challenge id

@tordans
Copy link

tordans commented Apr 18, 2024

Is there a way to get a preview branch running to make it easier to test the feature out?

@voscarmv
Copy link
Contributor

So cool to see this being built in real time! :D

Copy link
Contributor

@bhousel bhousel left a comment

Choose a reason for hiding this comment

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

This looks really great! Going to merge as-is, and I have a few minor comments about things we can clean up later. 🚀

*/
getData() {
const extent = this.context.systems.map.extent();
let challengeId = d3_select('.challenge-id-input').property('value');
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor issue -
In "Service code" like this, I try to avoid adding "UI code" like d3-selecting a value out of a field. Doing this couples the code pretty tightly - if someone changes the .challenge-id-input field elsewhere in Rapid, stuff in this part of the code will stop working, and that's unintuitive.

I think a cleaner way to do this is to have the service code keep track of the current challengeID in a class property. The UI code elsewhere can get or set this value, but really the service "owns" that bit of state.

I think of it like: the MapRouletteService code's job is only to talk to MapRoulette and keep track of all the data retrieved from there - ideally it shouldn't know anything about input fields. Actually the MapRouletteService code should even be able to work in an environment with no UI at all, like a CLI tool.

This is tough too because we do have UI code mixed in with Service code in other places - so you might even see it done that way in old code that we haven't cleaned up yet. We had a lot of d3 code in places like StreetviewService and MapillaryService, for doing the highlighting and styling of markers before we switched to WebGL. We also have some code there for dealing with the Mapillary viewer, and for this case we really need the Service and the UI to be working together.

this._abortUnwantedRequests(this._cache, tiles);

// get the challenge ID entered by the user
let challengeId = Number(d3_select('.challenge-id-input').property('value'));
Copy link
Contributor

Choose a reason for hiding this comment

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

same code smell as before - MapRouletteService should own the challengeID, not be d3-selecting it out of a field that is managed elsewhere.

.then(response => {
if (!response.ok) throw new Error(`Error posting comment: ${response.statusText}`);
return response.json();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

If i'm reading it correctly, this fetch looks abortable, but there isn't a catch that handles that situation? It could also be that we never abort it though elsewhere in the code so it doesn't need the AbortController?

      .catch(err => {
        if (err.name === 'AbortError') return;          // ok
        if (err instanceof Error) console.error(err);   // eslint-disable-line no-console
        // handle the failure of the post?
      })
      .finally(() => {
        delete this._cache.inflightPost[task.id];    // it's not inflight anymore
      });

})
.catch(err => {
console.log('In catch block', err);
// Handle any errors
Copy link
Contributor

Choose a reason for hiding this comment

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

same here? or this._cache.inflightPost[task.id] is mixing up the abort controllers from 2 different fetches which seems bad. We might be able to just get rid of them, since it's not like the user can abort their clicking of the "I'm done" button.

* @param xml
* @param callback
*/
_parseUserPreferencesXML(xml, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty cool actually - We've talked about reading/storing user preferences from the OSM API for a while: openstreetmap/iD#3002

We can expand this later and use it for other things.


div
.append('h4')
.html(l10n.tHtml('map_data.layers.maproulette.fix_title'));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably just a case where you copied some code from elsewhere that was doing it but.. We're trying to avoid using code like .html(l10n.tHtml(stringID)) because it circumvents string sanitization - #1274 has more on this. If these strings somehow ended up dangerous html, it gets inserted directly into the DOM and that's bad because it can do things to the user that we don't intend.

Anyway, we are cleaning these up as we go, and the risk is pretty minimal, so no worries - we just try not to add it to new code.

Aside: One thing React got right was - they named this feature dangerouslySetInnerHTML so people would think twice before using it!
https://react.dev/reference/react-dom/components/common#dangerously-setting-the-inner-html


detailsDiv
.append('p')
.html(`${parentId} / ${id}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess in that last comment, I forgot to say the right way of doing things...

In most cases you can use .text() instead of .html()
In D3, these things are mostly aliases for:

Unless you really need to jam html into the document, prefer .text()

.merge(editor)
.call(mapRouletteHeader.task(_maprouletteTask))
.call(mapRouletteDetails.task(_maprouletteTask))
.call(maprouletteSaveSection)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should pick one spelling and stick with it "mapRoulette" or "maproulette"

@@ -259,11 +273,16 @@ export function uiSidebar(context) {
inspector
.state('hide');

} else if (_wasRapid || _wasData || _wasNote || _wasQaItem) {
} else if (_wasRapid || _wasData || _wasNote || _wasQaItem || _wasMapRoulette) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha this code is getting pretty nutty..
Not your fault at all - just, I should really improve our sidebar UI!

@bhousel bhousel merged commit e031930 into main May 3, 2024
1 check passed
@bhousel bhousel deleted the maproulette_integration branch May 3, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants