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

fix(billing): remove quantity dropdown from frontend for nlp add-ons TASK-1482 #5464

Merged
merged 6 commits into from
Feb 14, 2025
Merged

fix(billing): remove quantity dropdown from frontend for nlp add-ons TASK-1482 #5464

merged 6 commits into from
Feb 14, 2025

Conversation

RuthShryock
Copy link
Member

@RuthShryock RuthShryock commented Jan 30, 2025 •
edited by jamesrkiger
Loading

🗒️ Checklist

  1. run linter locally
  2. update all related docs (API, README, inline, etc.), if any
  3. draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
  4. tag PR: at least frontend or backend unless it's global
  5. fill in the template below and delete template comments
  6. review thyself: read the diff and repro the preview as written
  7. open PR & confirm that CI passes
  8. request reviewers, if needed
  9. delete this section before merging

📣 Summary

Removes quantity dropdown from frontend for nlp add-ons.

👀 Preview steps

Bug template:

  1. ℹ️ have an account and a project
  2. Check out the Account Settings < Add-ons
  3. 🔴 [on main] notice that the NLP Package has a quantity dropdown
  4. 🟢 [on PR] notice that the NLP Package no longer has a quantity dropdown

Thoroughly test add-ons and subscription plans to make sure that everything is still working correctly.

@RuthShryock RuthShryock self-assigned this Jan 30, 2025
Akuukis
Akuukis requested changes Feb 3, 2025
View reviewed changes
Copy link
Contributor

@Akuukis Akuukis left a comment

Choose a reason for hiding this comment

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

code LGTM ✔️ previewed successfully locally as written ✔️

The only problem is that this is a bundle of 3 different fixes, please break up and PR separately.

jsapp/js/account/addOns/oneTimeAddOnRow.component.tsx Outdated Show resolved Hide resolved
jamesrkiger
jamesrkiger requested changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@jamesrkiger jamesrkiger left a comment

Choose a reason for hiding this comment

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

This seems to be fine for the UI change of removing the dropdown, but there is a lot of logic in the code for handling addon quantities (as opposed to subscription quantities, which are separate) that needs to be removed. Would you mind going through to remove any code that is no longer needed as a result of this change?

@Akuukis Akuukis self-requested a review February 4, 2025 17:15
Akuukis
Akuukis approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@Akuukis Akuukis left a comment

Choose a reason for hiding this comment

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

LGTM from my POV but James raised a good point ☝️

@RuthShryock RuthShryock changed the title fix(billing): remove quantity dropdown for nlp add-ons TASK-1482 fix(billing)!: remove quantity dropdown for nlp add-ons TASK-1482 Feb 4, 2025
@RuthShryock RuthShryock changed the title fix(billing)!: remove quantity dropdown for nlp add-ons TASK-1482 fix(billing)!: remove quantity from UI and backend for nlp add-ons TASK-1482 Feb 4, 2025
jamesrkiger
jamesrkiger requested changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@jamesrkiger jamesrkiger left a comment

Choose a reason for hiding this comment

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

Sorry, but I guess I should have specified that I was referring to the frontend code rather than the backend code. That's OK. We probably do want to consider removing the related backend code, but that will require some more careful testing. Can you split the backend work off into a separate PR and assign it to Guillermo and I for review?

For the frontend code, can you remove the interface changes for the time being? I would like us to be able to review/merge the frontend changes without having to rush the backend review.

Please let me know if that makes sense.

@RuthShryock RuthShryock force-pushed the 1482-remove-nlp-addons-dropdown branch from a5e5aef to 5499af9 Compare February 5, 2025 16:11
@RuthShryock RuthShryock changed the title fix(billing)!: remove quantity from UI and backend for nlp add-ons TASK-1482 fix(billing): remove quantity dropdown from frontend for nlp add-ons TASK-1482 Feb 5, 2025
@RuthShryock RuthShryock mentioned this pull request Feb 5, 2025
Merged
@magicznyleszek magicznyleszek removed their request for review February 6, 2025 10:36
jamesrkiger
jamesrkiger reviewed Feb 11, 2025
View reviewed changes
jsapp/js/account/addOns/addOnList.module.scss Outdated
@@ -149,6 +149,10 @@

.oneTime {
justify-content: center;

:last-child {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe some weird git issue. I think you removed this in a previous PR

jamesrkiger
jamesrkiger requested changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@jamesrkiger jamesrkiger left a comment

Choose a reason for hiding this comment

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

left one comment with a git/css issue. Other than that, lgtm

@jamesrkiger jamesrkiger merged commit 74cfec4 into main Feb 14, 2025
5 checks passed
@jamesrkiger jamesrkiger deleted the 1482-remove-nlp-addons-dropdown branch February 14, 2025 12:35
RuthShryock added a commit that referenced this pull request Feb 17, 2025
@RuthShryock
fix(billing): remove quantity from backend for nlp add-ons TASK-1482 (
f0f1d7d 
#5490)

### 💭 Notes
Removed all logic related to handling add-on quantity. Test billing
(buying add-ons, upgrading plans, updating submission/storage/nlp usage,
etc.) to make sure that nothing is broken.

PR to remove nlp add-on quantity on frontend: #5464
rgraber pushed a commit that referenced this pull request Feb 18, 2025
@RuthShryock @rgraber
fix(billing): remove quantity from backend for nlp add-ons TASK-1482 (
ce567bb 
#5490)

### 💭 Notes
Removed all logic related to handling add-on quantity. Test billing
(buying add-ons, upgrading plans, updating submission/storage/nlp usage,
etc.) to make sure that nothing is broken.

PR to remove nlp add-on quantity on frontend: #5464
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamesrkiger jamesrkiger jamesrkiger requested changes

@Akuukis Akuukis Akuukis approved these changes

Assignees

@RuthShryock RuthShryock

Labels
Front end
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RuthShryock @Akuukis @jamesrkiger