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

[Frontend][Review Metrics] Review Metrics Playtesting follow-ups #32

Merged
merged 5 commits into from
Sep 21, 2022

Conversation

terryttsai
Copy link
Contributor

@terryttsai terryttsai commented Sep 20, 2022

Description of the change

  • Fix overwritten values count
    • Due to duplicate datapoint values in the Data Upload API response, we overcount overwritten values sometimes.
      • I filed a task to fix this on the backend, but for now, we can ignore duplicates in the frontend. I didn't put a TODO to undo this change because I don't think it's really necessary.
  • Change "Overwrites" to "Overwritten Values"
  • Allow continuing to the Review Metrics page even if the upload had errors

Screen Shot 2022-09-21 at 9 26 14 AM

- Tweaked Review Metrics to make it more clear that their data has already been successfully uploaded - Changed the "Review 0 Uploaded Metrics" copy into a very simple No Metrics to Review

Screen Shot 2022-09-21 at 9 59 32 AM

Type of change

All pull requests must have at least one of the following labels applied (otherwise the PR will fail):

Label Description
Type: Bug non-breaking change that fixes an issue
Type: Feature non-breaking change that adds functionality
Type: Breaking Change fix or feature that would cause existing functionality to not work as expected
Type: Non-breaking refactor change addresses some tech debt item or prepares for a later change, but does not change functionality
Type: Configuration Change adjusts configuration to achieve some end related to functionality, development, performance, or security
Type: Dependency Upgrade upgrades a project dependency - these changes are not included in release notes

Related issues

Closes #XXXX

Checklists

Development

This box MUST be checked by the submitter prior to merging:

  • Double- and triple-checked that there is no Personally Identifiable Information (PII) being mistakenly added in this pull request

These boxes should be checked by the submitter prior to merging:

  • Tests have been written to cover the code changed/added as part of this pull request

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

@terryttsai terryttsai requested a review from a team September 20, 2022 23:52
@terryttsai terryttsai changed the title [Frontend][Review Metrics] Fix overwritten values count [Frontend][Review Metrics] Review Metrics Playtesting fixes Sep 21, 2022
@terryttsai terryttsai changed the title [Frontend][Review Metrics] Review Metrics Playtesting fixes [Frontend][Review Metrics] Review Metrics Playtesting follow-ups Sep 21, 2022
Copy link
Collaborator

@lilidworkin lilidworkin left a comment

Choose a reason for hiding this comment

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

Nice, thanks for knocking out all these fixes @terryttsai ! Appreciate how you enumerated everything that this PR does in the description too. 🙌

Continue
</Button>
)}
<Button
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a screenshot showing what this page looks like now if there are errors? I think there will be two calls to action -- a blue button on the left that says New Upload and a blue button on the right that says Continue. Is that right? That's fine for now (we need input on product and design on this) but just want to make sure I know the current behavior. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added screenshot in the description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the button on the right is blue

return (
<DatapointsTableDetailsCell key={key}>
{parseFloat(
(typeof value === "string" ? parseFloat(value) : value).toFixed(2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually not sure we want to do this -- I just commented on the doc, the issue with the "too many sig figs" was that what we were showing in the UI didn't match what was in the spreadsheet. I think if there are 2+ decimals in the spreadsheet, it's fine to show in the UI without rounding. LMK if that makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove this, I'd rather accurately portray what the user gave us, if it's showing up as too many sig figs from the backend, I'll leave it up to Nichelle to fix!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not actually sure where it's showing up as too many sig figs though -- could be backend or could be frontend -- can you coordinate with @nichelle-hall to investigate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is coming from the backend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed logic to round decimals

@@ -106,9 +106,6 @@ const ReviewMetrics: React.FC = observer(() => {
}, {} as { [key: string]: number });

metric.datapoints.forEach((dp) => {
if (dp.old_value !== null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for this fix!! Are the duplicate values on the backend causing any other issues? We should definitely fix the root cause on the backend, just curious if there are any other patches we need to do on the frontend in the meantime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this is the issue to fix: #31, and on the frontend, I'm rooting out duplicate datapoints before using it and hadn't found any more issues with this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good!

@terryttsai
Copy link
Contributor Author

if my math is right, (6/16)^9 or 0.015% chance of a commit hash starting with 9 letters in a row
eccdddc

@terryttsai
Copy link
Contributor Author

Added a very simple No Metrics to Review state for the review metrics page

Copy link
Collaborator

@lilidworkin lilidworkin left a comment

Choose a reason for hiding this comment

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

Awesome!!

@terryttsai terryttsai merged commit c3c6b69 into main Sep 21, 2022
@terryttsai terryttsai deleted the terry/review-metrics-fixes branch September 21, 2022 18:58
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.

2 participants