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.

Sign up for GitHub

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

Jump to bottom

PLU-309: [TILES-ATOMIC-INCREMENT-4] backend implementation for increment/decrement row value #835

Open
wants to merge 2 commits into
base: fix/tiles/update-operator
Choose a base branch
from
Open

PLU-309: [TILES-ATOMIC-INCREMENT-4] backend implementation for increment/decrement row value #835

wants to merge 2 commits into from

Conversation

pregnantboy
Copy link
Contributor

@pregnantboy pregnantboy commented Dec 24, 2024 •
edited
Loading

TL;DR

Add backend implementation for 'add' and 'subtract' operations for Tiles update row action

What changed?

  • Modified patchTableRow to handle mathematical operations
    • added tests
    • error handling for invalid numeric operations
  • Updated column metadata for tiles action to maintain column ordering
    • Fixed variable sorting logic for null order values

How to test?

  1. Create a tile with a few columns and values that are both numbers and strings
  2. Test updating cells using:
    • normal updates (set as)
    • adding
    • subtracting
    • mixed
  3. Verify error handling by:
    • Attempting math operations on non-numeric values (both original values and operands)
  4. Check that columns maintain their order in the UI

Regression test

  1. Check that existing tiles update rows are still working

This was referenced Dec 24, 2024
Merged
Merged
@pregnantboy Graphite App
Copy link
Contributor Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@pregnantboy pregnantboy mentioned this pull request Dec 24, 2024
Open
@pregnantboy pregnantboy marked this pull request as ready for review December 24, 2024 08:05
@pregnantboy pregnantboy requested a review from a team as a code owner December 24, 2024 08:05
@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Dec 24, 2024 •
edited
Loading

Datadog Report

Branch report: feat/tiles/update-operator-backend
Commit report: 4e5afd5
Test service: plumber

✅ 0 Failed, 736 Passed, 0 Skipped, 2m 18.89s Total Time

@pregnantboy pregnantboy changed the title feat: add backend logic to perform atomic increment/decrement PLU-309: [TILES-ATOMIC-INCREMENT-4] enhance multirow-multicol UI with custom styles and operators Dec 24, 2024
@linear Linear
Copy link

linear bot commented Dec 24, 2024

PLU-309 Tiles: Increment counter action

@pregnantboy pregnantboy changed the title PLU-309: [TILES-ATOMIC-INCREMENT-4] enhance multirow-multicol UI with custom styles and operators PLU-309: [TILES-ATOMIC-INCREMENT-4] backend implementation for increment/decrement row value Dec 24, 2024
m0nggh
m0nggh reviewed Jan 3, 2025
View reviewed changes
packages/frontend/src/helpers/variables.ts Outdated
@@ -32,13 +32,13 @@ function sortVariables(variables: Variable[]): void {
variables.sort((a, b) => {
// Put vars with null order last, but preserve ordering (via `sort`'s
// stability) if both are null.
if (!a.order && !b.order) {
if (a.order == null && !b.order == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

b.order == null instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

goood catch

m0nggh
m0nggh approved these changes Jan 3, 2025
View reviewed changes
Copy link
Contributor

@m0nggh m0nggh left a comment

Choose a reason for hiding this comment

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

other than the small nit, lgtm

tested that the

  • 3 operations work
  • error is caught for non-numeric add/subtract
  • old update row steps still work
  • columns variables now maintain order

@pregnantboy pregnantboy force-pushed the feat/tiles/update-operator-backend branch from 6419851 to 76739a8 Compare January 9, 2025 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m0nggh m0nggh m0nggh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pregnantboy @m0nggh