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

added source col, removed mobile column from label validation table #3380

Merged
merged 29 commits into from
Sep 13, 2023

Conversation

dylanbun
Copy link
Collaborator

@dylanbun dylanbun commented Sep 1, 2023

Resolves #3164

Added a new column, source. Logs locations of where users did their validations. Possiblities are GalleryCard, GalleryExpanded, LabelMap, ValidateDesktop, ValidateMobile, AdminLabelMap, AdminLabelSearch, AdminUserDashboard, and AdminContributionPage. Since we now have a way to distinguish between ValidateDesktop and ValidateMobile, removed the is_mobile column from the label validation table.

Before/After screenshots (if applicable)

Before:
image

After:
image

Testing instructions
  1. Go to Project Sidewalk and start testing various forms of validation
  2. Go to label validation table and confirm correct sources for each validation type.
Things to check before submitting the PR
  • I've written a descriptive PR title.
  • I've added/updated comments for large or confusing blocks of code.
  • I've included before/after screenshots above.
  • I've tested on mobile (only needed for validation page).

@dylanbun dylanbun self-assigned this Sep 1, 2023
@@ -71,6 +71,7 @@ function Tracker() {
timestamp: timestamp,
zoom: pov ? pov.zoom : null,
is_mobile: isMobile()
// @mikey should this is_mobile be removed?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@misaugstad can you confirm if this is_mobile should not be removed? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Is it being used anywhere that you haven't removed? Is there a reason that you're not sure if this one should be removed?

@misaugstad
Copy link
Member

removed the is_mobile column from the label validation table

Did you mark the previous mobile ones as ValidateMobile in your new column? And IIRC, validations through both the Validate page had a different mission type from all the other validations, so I think that we could tell which ones were ValidateDesktop as well?

@dylanbun
Copy link
Collaborator Author

dylanbun commented Sep 1, 2023

Did you mark the previous mobile ones as ValidateMobile in your new column?

Whoops! No, I did not but I just added to that latest commit.

And IIRC, validations through both the Validate page had a different mission type from all the other validations, so I think that we could tell which ones were ValidateDesktop as well?

If I am understanding this correctly are you saying we could also update the source column for certain existing rows as "ValidateDesktop" ? I would think so but just to make sure isn't that just based on the canvas height and width (height: 440, width: 720?).

@misaugstad
Copy link
Member

If I am understanding this correctly are you saying we could also update the source column for certain existing rows as "ValidateDesktop" ? I would think so but just to make sure isn't that just based on the canvas height and width (height: 440, width: 720?).

No I was thinking about the mission_type table which has one called validation and one called labelmapValidation. I believe that we had just mashed everything except for ValidateMobile and ValidateDesktop into the labelmapValidation mission type.

This was done because validations needed to be tied to missions, the Validate missions are set to 10 labels, but doing a validation through LabelMap, Gallery, etc. doesn't have a specific number of labels in the mission, so we just made a new mission type for them.

I'm assuming we could use this to figure out which ones are from the Validate page. Though everything else hasn't been delineated in a way that we could use.

@dylanbun
Copy link
Collaborator Author

@misaugstad ready for review!

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Looking pretty good, just looking to fix a couple things and rename a few other things!

conf/evolutions/default/202.sql Show resolved Hide resolved
app/formats/json/ValidationTaskSubmissionFormats.scala Outdated Show resolved Hide resolved
app/models/label/LabelValidationTable.scala Outdated Show resolved Hide resolved
public/javascripts/Admin/src/Admin.LabelSearch.js Outdated Show resolved Hide resolved
conf/evolutions/default/202.sql Outdated Show resolved Hide resolved
@dylanbun
Copy link
Collaborator Author

@misaugstad ready for review!

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Looks great, thank you @dylanbun !!

@misaugstad misaugstad merged commit 92d991d into develop Sep 13, 2023
@misaugstad misaugstad deleted the 3164-source-col branch September 13, 2023 20:37
@misaugstad misaugstad mentioned this pull request Sep 29, 2023
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.

Add a "source" column in the label_validation table
3 participants