diff --git a/app/org/maproulette/framework/controller/ChallengeReportController.scala b/app/org/maproulette/framework/controller/ChallengeReportController.scala index cac67f1c1..8ad40d0c9 100644 --- a/app/org/maproulette/framework/controller/ChallengeReportController.scala +++ b/app/org/maproulette/framework/controller/ChallengeReportController.scala @@ -69,6 +69,22 @@ class ChallengeReportController @Inject() ( } } + /** + * Lists every report filed against a challenge, resolved ones included, so a + * reader can see what has been raised about it and where each report stands. + * Open to anyone, like the challenge comments that filing a report posts: + * the reporter's identity and words are already public through those. The + * reporter's email and the admin side of the triage record are stripped. + * + * @param challengeId The challenge in question + * @return The reports against that challenge, newest first + */ + def listForChallenge(challengeId: Long): Action[AnyContent] = Action.async { implicit request => + this.sessionManager.userAwareRequest { _ => + Ok(Json.toJson(this.challengeReportService.retrieveReportsForChallenge(challengeId))) + } + } + /** * Lists reports, newest first. Superusers only. * diff --git a/app/org/maproulette/framework/model/ChallengeReport.scala b/app/org/maproulette/framework/model/ChallengeReport.scala index cd0483a14..9962282f4 100644 --- a/app/org/maproulette/framework/model/ChallengeReport.scala +++ b/app/org/maproulette/framework/model/ChallengeReport.scala @@ -11,9 +11,11 @@ import play.api.libs.json._ /** * A report against a challenge's design -- "this challenge is poorly designed * and is causing people to make incorrect edits" -- rather than a bug or a - * feature request. Any authenticated user can file one; only superusers can - * read them, because a report names the reporter and may carry the email - * address they volunteered for follow-up. + * feature request. Any authenticated user can file one, and anyone can read + * what has been reported about a challenge, since filing also posts a public + * challenge comment naming the reporter and quoting the report. The triage + * queue is superusers only: it carries the email address the reporter may have + * volunteered for follow-up, and the notes admins leave on a decision. * * Reports are resolved, never deleted: an admin marks one actioned (say, after * archiving the challenge) or dismissed, so the history of what was reported diff --git a/app/org/maproulette/framework/repository/ChallengeReportRepository.scala b/app/org/maproulette/framework/repository/ChallengeReportRepository.scala index c2762a629..09de92599 100644 --- a/app/org/maproulette/framework/repository/ChallengeReportRepository.scala +++ b/app/org/maproulette/framework/repository/ChallengeReportRepository.scala @@ -171,8 +171,8 @@ class ChallengeReportRepository @Inject() (override val db: Database) extends Re /** * Retrieves a reporter's still-open report against one challenge, if they - * have one. This is the only report-reading path open to a non-superuser: - * it returns nothing but the caller's own report. + * have one. It returns nothing but the caller's own report, so a + * non-superuser may read through it. */ def retrieveOpenForReporter(challengeId: Long, reporterId: Long): Option[ChallengeReport] = { this.withMRConnection { implicit c => @@ -193,6 +193,37 @@ class ChallengeReportRepository @Inject() (override val db: Database) extends Re } } + /** + * Lists every report filed against one challenge, newest first, resolved + * ones included. Rows come back whole; it is the service that decides which + * parts of them a given caller may see. + */ + def listForChallenge(challengeId: Long): List[ChallengeReport] = { + this.withMRConnection { implicit c => + SQL( + s"""SELECT COUNT(*) OVER() AS full_count, $selectColumns + $fromClause + WHERE cr.challenge_id = {challengeId} + ORDER BY cr.reported_at DESC""" + ).on(Symbol("challengeId") -> challengeId) + .as(this.parser.*) + } + } + + /** + * The ids of every challenge carrying at least one open report. The archive + * scheduler uses this to leave reported challenges alone: archiving one + * would drop it out of the admin triage queue, which defaults to challenges + * that are still active, before anyone had ruled on the report. + */ + def challengeIdsWithOpenReports(): List[Long] = { + this.withMRConnection { implicit c => + SQL"""SELECT DISTINCT challenge_id FROM challenge_reports + WHERE status = ${ChallengeReport.STATUS_OPEN}""" + .as(get[Long]("challenge_id").*) + } + } + /** * Counts a reporter's still-open reports against one challenge, so the same * person cannot file the same complaint repeatedly. diff --git a/app/org/maproulette/framework/service/ChallengeReportService.scala b/app/org/maproulette/framework/service/ChallengeReportService.scala index 6727c9422..bbd1d1f8c 100644 --- a/app/org/maproulette/framework/service/ChallengeReportService.scala +++ b/app/org/maproulette/framework/service/ChallengeReportService.scala @@ -16,8 +16,10 @@ import org.slf4j.LoggerFactory /** * Handles reports filed against a challenge's design. Filing is open to any - * authenticated user; reading and triaging is restricted to superusers, since - * a report carries the reporter's identity and possibly their email address. + * authenticated user, and what a report says is readable by anyone, since + * filing posts a public challenge comment saying the same thing. The triage + * queue is restricted to superusers: it carries the email address a reporter + * may have volunteered, and the notes admins leave for each other. * * Filing a report also posts a challenge comment naming the reporter and * quoting the report, which is what tells the challenge owner that something @@ -140,6 +142,47 @@ class ChallengeReportService @Inject() ( def retrieveOwnOpenReport(user: User, challengeId: Long): Option[ChallengeReport] = this.repository.retrieveOpenForReporter(challengeId, user.id) + /** + * Retrieves every report filed against a challenge, newest first and + * resolved ones included, so anyone weighing up the challenge can see what + * has been raised about it and where each report stands. + * + * Open to anyone, signed in or not, because filing a report already posts a + * challenge comment naming the reporter and quoting what they wrote -- the + * reporter's identity and words are public either way. What is not public is + * stripped: see [[publicView]]. + * + * @param challengeId The challenge in question + * @return The reports against that challenge + */ + def retrieveReportsForChallenge(challengeId: Long): List[ChallengeReport] = + this.repository.listForChallenge(challengeId).map(this.publicView) + + /** + * Reduces a report to the parts that are already public knowledge. Out go + * the email address the reporter volunteered for follow-up, which the + * accompanying challenge comment deliberately omits, and the admin side of + * the triage record -- who ruled on the report and the note they left for + * their own purposes. The outcome and its date survive, so a reader can see + * that a report was dealt with. + */ + private def publicView(report: ChallengeReport): ChallengeReport = + report.copy( + reporterEmail = None, + reviewedBy = None, + reviewedByName = None, + reviewComment = None + ) + + /** + * The ids of every challenge carrying at least one open report, so the + * archive scheduler can leave those challenges alone until an admin has + * ruled. Internal to the scheduler -- it names no reporter, but it is not + * exposed over the API either. + */ + def challengeIdsWithOpenReports(): Set[Long] = + this.repository.challengeIdsWithOpenReports().toSet + /** * Lists reports for the admin dashboard. * diff --git a/app/org/maproulette/jobs/SchedulerActor.scala b/app/org/maproulette/jobs/SchedulerActor.scala index a5ff78276..87f3d5594 100644 --- a/app/org/maproulette/jobs/SchedulerActor.scala +++ b/app/org/maproulette/jobs/SchedulerActor.scala @@ -756,9 +756,20 @@ class SchedulerActor @Inject() ( logger.info(action + " - Stale Date: " + staleDate); + // A challenge someone has reported is left alone until an admin has ruled + // on the report: archiving it would drop it out of the triage queue, which + // defaults to challenges that are still active, before anyone looked at it. + val reportedChallengeIds = this.serviceManager.challengeReport.challengeIdsWithOpenReports() + if (reportedChallengeIds.nonEmpty) { + logger.info( + action + " - Skipping " + reportedChallengeIds.size + " challenge(s) with an open report" + ) + } + this.serviceManager.challenge .activeChallenges() .filter(challenge => challenge.created.toString("yyyy-MM-dd") < staleDate) + .filterNot(challenge => reportedChallengeIds.contains(challenge.id)) .foreach(challenge => { val tasks = this.serviceManager.challenge.getTasksByParentId(challenge.id); diff --git a/conf/v2_route/challengereport.api b/conf/v2_route/challengereport.api index 986c3d3e5..f6edcf8e4 100644 --- a/conf/v2_route/challengereport.api +++ b/conf/v2_route/challengereport.api @@ -66,6 +66,29 @@ POST /challenge/:challengeId/report @org.maproulette.framework. GET /challenge/:challengeId/report/mine @org.maproulette.framework.controller.ChallengeReportController.retrieveOwnOpenReport(challengeId:Long) ### # tags: [ Challenge Report ] +# operationId: challenge_report_list_for_challenge +# summary: List the Reports Against a Challenge +# description: Returns every report filed against a challenge, newest first, resolved ones included, so a reader can see what has been raised about it and where each report stands. Open to anyone, like the challenge comments that filing a report posts -- the reporter's identity and words are already public through those. The email a reporter volunteered for follow-up, and the admin side of the triage record (who ruled on a report and the note they left), are stripped; the outcome and its date remain. +# responses: +# '200': +# description: The reports against this challenge +# content: +# application/json: +# schema: +# type: array +# items: +# $ref: '#/components/schemas/org.maproulette.framework.model.ChallengeReport' +# parameters: +# - name: challengeId +# in: path +# description: The id of the challenge in question +# required: true +# schema: +# type: integer +### +GET /challenge/:challengeId/reports @org.maproulette.framework.controller.ChallengeReportController.listForChallenge(challengeId:Long) +### +# tags: [ Challenge Report ] # operationId: challenge_report_list # summary: List Challenge Reports # description: Lists reports filed against challenges, newest first, for the admin triage dashboard. Restricted to superusers, because a report carries the reporter's identity and any email address they volunteered. Each returned report includes the total number of matches in fullCount. diff --git a/test/org/maproulette/framework/FrameworkMasterSuite.scala b/test/org/maproulette/framework/FrameworkMasterSuite.scala index 113910d36..d5a671b3a 100644 --- a/test/org/maproulette/framework/FrameworkMasterSuite.scala +++ b/test/org/maproulette/framework/FrameworkMasterSuite.scala @@ -24,6 +24,7 @@ class FrameworkMasterSuite extends Suites with BeforeAndAfterAll with TestDataba new ChallengeServiceSpec, new ChallengeRepositorySpec, new ChallengeReportRepositorySpec, + new ChallengeReportServiceSpec, new TeamImageRepositorySpec, new TeamAvatarRepositorySpec, new ChallengeListingServiceSpec, diff --git a/test/org/maproulette/framework/repository/ChallengeReportRepositorySpec.scala b/test/org/maproulette/framework/repository/ChallengeReportRepositorySpec.scala index 954253695..ee4f05a68 100644 --- a/test/org/maproulette/framework/repository/ChallengeReportRepositorySpec.scala +++ b/test/org/maproulette/framework/repository/ChallengeReportRepositorySpec.scala @@ -99,6 +99,41 @@ class ChallengeReportRepositorySpec(implicit val application: Application) exten this.repository.retrieveOpenForReporter(challenge.id, this.defaultUser.id) mustEqual None } + "return every report on a challenge, newest first, resolved ones included" taggedAs ChallengeReportRepoTag in { + val challenge = + this.createChallengeStructure("report_challenge_history", this.defaultProject.id, 1) + val other = this.createChallengeStructure("report_other_history", this.defaultProject.id, 1) + + this.repository.listForChallenge(challenge.id) mustEqual List() + + val first = report(challenge) + this.repository + .updateStatus(first.id, ChallengeReport.STATUS_DISMISSED, this.defaultUser, None) + val second = report(challenge) + + // A report on a different challenge stays out of this challenge's history. + report(other) + + val history = this.repository.listForChallenge(challenge.id) + history.map(_.id) mustEqual List(second.id, first.id) + history.map(_.status) mustEqual + List(ChallengeReport.STATUS_OPEN, ChallengeReport.STATUS_DISMISSED) + history.map(_.reporterId).distinct mustEqual List(Some(this.defaultUser.id)) + } + + "name only the challenges carrying an open report" taggedAs ChallengeReportRepoTag in { + val challenge = this.createChallengeStructure("report_open_ids", this.defaultProject.id, 1) + + this.repository.challengeIdsWithOpenReports() must not contain challenge.id + + val open = report(challenge) + this.repository.challengeIdsWithOpenReports() must contain(challenge.id) + + this.repository + .updateStatus(open.id, ChallengeReport.STATUS_ACTIONED, this.defaultUser, None) + this.repository.challengeIdsWithOpenReports() must not contain challenge.id + } + "filter a listing by status and by challenge, newest first" taggedAs ChallengeReportRepoTag in { val challenge = this.createChallengeStructure("report_listing", this.defaultProject.id, 1) diff --git a/test/org/maproulette/framework/service/ChallengeReportServiceSpec.scala b/test/org/maproulette/framework/service/ChallengeReportServiceSpec.scala new file mode 100644 index 000000000..86d2958e1 --- /dev/null +++ b/test/org/maproulette/framework/service/ChallengeReportServiceSpec.scala @@ -0,0 +1,79 @@ +/* + * Copyright (C) 2020 MapRoulette contributors (see CONTRIBUTORS.md). + * Licensed under the Apache License, Version 2.0 (see LICENSE). + */ + +package org.maproulette.framework.service + +import org.maproulette.framework.model.{ChallengeReport, User} +import org.maproulette.framework.util.{ChallengeReportTag, FrameworkHelper} +import play.api.Application + +class ChallengeReportServiceSpec(implicit val application: Application) extends FrameworkHelper { + val service: ChallengeReportService = + this.application.injector.instanceOf(classOf[ChallengeReportService]) + + private val COMMENT = "This challenge asks mappers to delete valid buildings." * 2 + + "ChallengeReportService" should { + "list everyone's reports on a challenge, without the reporter's email" taggedAs ChallengeReportTag in { + val challenge = + this.createChallengeStructure("report_service_list", this.defaultProject.id, 1) + + this.service.retrieveReportsForChallenge(challenge.id) mustEqual List() + + val filed = + this.service.create(this.defaultUser, challenge.id, COMMENT, Some("mapper@example.com")) + filed.reporterEmail mustEqual Some("mapper@example.com") + + val visible = this.service.retrieveReportsForChallenge(challenge.id) + visible.map(_.id) mustEqual List(filed.id) + // The reporter is named -- filing already posted a challenge comment + // saying so -- but the address they left for follow-up is not. + visible.head.reporterName mustEqual Some(this.defaultUser.name) + visible.head.reporterEmail mustEqual None + visible.head.comment mustEqual COMMENT + } + + "show that a report was resolved without exposing who ruled on it, or their note" taggedAs ChallengeReportTag in { + val challenge = + this.createChallengeStructure("report_service_resolved", this.defaultProject.id, 1) + val filed = this.service.create(this.defaultUser, challenge.id, COMMENT, None) + + this.service.updateStatus( + User.superUser, + filed.id, + ChallengeReport.STATUS_ACTIONED, + Some("archived the challenge") + ) + + val visible = this.service.retrieveReportsForChallenge(challenge.id).head + visible.status mustEqual ChallengeReport.STATUS_ACTIONED + visible.reviewedAt mustBe defined + visible.reviewedBy mustEqual None + visible.reviewedByName mustEqual None + visible.reviewComment mustEqual None + + // The admin dashboard still sees the whole record. + val forAdmin = this.service.retrieve(User.superUser, filed.id).get + forAdmin.reviewedBy mustEqual Some(User.superUser.id) + forAdmin.reviewComment mustEqual Some("archived the challenge") + } + + "name the challenges carrying an open report, so the archiver can skip them" taggedAs ChallengeReportTag in { + val challenge = + this.createChallengeStructure("report_service_open_ids", this.defaultProject.id, 1) + + this.service.challengeIdsWithOpenReports() must not contain challenge.id + + val filed = this.service.create(this.defaultUser, challenge.id, COMMENT, None) + this.service.challengeIdsWithOpenReports() must contain(challenge.id) + + this.service + .updateStatus(User.superUser, filed.id, ChallengeReport.STATUS_DISMISSED, None) + this.service.challengeIdsWithOpenReports() must not contain challenge.id + } + } + + override implicit val projectTestName: String = "ChallengeReportServiceSpecProject" +} diff --git a/test/org/maproulette/framework/util/FrameworkHelper.scala b/test/org/maproulette/framework/util/FrameworkHelper.scala index ec097bd55..359c3f761 100644 --- a/test/org/maproulette/framework/util/FrameworkHelper.scala +++ b/test/org/maproulette/framework/util/FrameworkHelper.scala @@ -167,6 +167,7 @@ object ChallengeListingTag extends Tag("challengelisting") object ChallengeListingRepoTag extends Tag("challengelistingrepo") object ChallengeSnapshotTag extends Tag("challengesnapshot") object TeamImageRepoTag extends Tag("teamimagerepo") +object ChallengeReportTag extends Tag("challengereport") object ChallengeReportRepoTag extends Tag("challengereportrepo") object TeamAvatarRepoTag extends Tag("teamavatarrepo") object ProjectTag extends Tag("project")