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

Fix deleted parent filter #1135

Merged
merged 3 commits into from
Sep 3, 2024
Merged

Fix deleted parent filter #1135

merged 3 commits into from
Sep 3, 2024

Conversation

CollinBeczak
Copy link
Contributor

@CollinBeczak CollinBeczak commented Jun 27, 2024

Both the /taskCluster endpoint and the /tasks/box/:left/:bottom/:right/:top endpoint should return the same tasks (the format of the data should be different, but the tasks themselves should be the same and the amount of tasks should be the same).

Issue: The two endpoints, when passed the same filtering parameters, would pass back different results.

Solution: Fixed the filtering conditions of these two endpoints:
PUT /taskCluster
PUT /tasks/box/:left/:bottom/:right/:top

More details on the solution:

  • Remove a manually added filter on enabled challenges from the taskCluster endpoint and use the built in search parameter instead.
  • Fix the deleted challenges and projects filters to filter for challenges and projects that have a "false" deleted status.
  • Remove manually added filter on deleted projects and challenges.

related: maproulette/maproulette3#2211

/taskCluster endpoint WHERE statement in sql query with the manually added "deleted" related filters:

WHERE (tasks.location && ST_MakeEnvelope (-111.91609382363669, 40.714313359738256, -111.86871528359762, 40.748394608673685, 4326)) AND 
(tasks.status IN (0,3,6)) AND ((c.status IN (3,4,0,-1) OR c.status IS NULL)) AND 
(NOT c.requires_local) AND (c.enabled) AND (c.is_archived = false) AND 
c.deleted = false AND p.deleted = false`

/tasks/box endpoint WHERE statement in sql query where filters on deleted project and challenges should be present:

WHERE (p.enabled) AND (tasks.location && ST_MakeEnvelope (-111.91609382363669, 40.714313359738256, -111.86871528359762, 40.748394608673685, 4326)) AND 
(tasks.status IN (0,3,6)) AND ((c.status IN (3,4,0,-1) OR c.status IS NULL)) AND 
(NOT c.requires_local) AND (c.enabled) AND (c.is_archived = false) 
ORDER BY RANDOM() DESC LIMIT 1001 OFFSET 0;

/taskCluster endpoint WHERE statement in sql query after change:

WHERE (c.deleted = false AND p.deleted = false) AND (tasks.location && ST_MakeEnvelope (-111.90092327097236, 40.72337196386074, -111.87723400095282, 40.740412450956256, 4326)) AND 
(tasks.status IN (0,3,6)) AND ((c.status IN (3,4,0,-1) OR c.status IS NULL)) AND 
(NOT c.requires_local) AND (c.enabled) AND (c.is_archived = false)

/tasks/box endpoint WHERE statement in sql query after change:

WHERE (c.deleted = false AND p.deleted = false) AND (tasks.location && ST_MakeEnvelope (-111.90092327097236, 40.72337196386074, -111.87723400095282, 40.740412450956256, 4326)) AND 
(tasks.status IN (0,3,6)) AND ((c.status IN (3,4,0,-1) OR c.status IS NULL)) AND 
(NOT c.requires_local) AND (c.enabled) AND (c.is_archived = false) 
ORDER BY RANDOM() DESC LIMIT 1001 OFFSET 0;

The reason why p.enabled is missing from the new PUT /tasks/box/:left/:bottom/:right/:top endpoint is because i removed the manually added condition that added it to that specific endpoint. The filter that is supposed to be used is blocked by a condition:

if (projectSearch) {
  filterList = this.filterProjects(params) :: this.filterProjectEnabled(params) :: filterList
}

projectSearch value is determined by this function:

  /**
    * Filters by p.display_name with a like %projectSearch%
    * @param params with inverting on 'ps'
    */
  def filterProjectSearch(params: SearchParameters): FilterGroup = {
    FilterGroup(
      List(
        FilterParameter.conditional(
          Project.FIELD_DISPLAY_NAME,
          s"'${SQLUtils.search(params.projectSearch.getOrElse(""))}'",
          Operator.ILIKE,
          params.invertFields.getOrElse(List()).contains("ps"),
          true,
          params.projectSearch != None,
          Some("p")
        )
      )
    )
  }

At the moment this change is necessary for the endpoints outputs to match, but further investigation of why that condition is needed.

Copy link

@CollinBeczak CollinBeczak marked this pull request as ready for review July 19, 2024 20:47
@CollinBeczak CollinBeczak requested a review from ljdelight July 19, 2024 21:49
Copy link
Contributor

@ljdelight ljdelight left a comment

Choose a reason for hiding this comment

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

LGTM

@ljdelight
Copy link
Contributor

Removing if (params.projectEnabled.getOrElse(false)) block might result in some edge case, but it's fine to merge

@ljdelight ljdelight merged commit 5a6a2ab into main Sep 3, 2024
11 checks passed
@ljdelight ljdelight deleted the fix-deleted-parent-filter branch September 3, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants