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

[AC-1650] [AC-1578] #3320

Merged
merged 10 commits into from
Nov 1, 2023
Merged

[AC-1650] [AC-1578] #3320

merged 10 commits into from
Nov 1, 2023

Conversation

cturnbull-bitwarden
Copy link
Contributor

Type of change

- [ ] Bug fix
- [X] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Code changes

  • file.ext: Description of what was changed and why

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@cturnbull-bitwarden cturnbull-bitwarden requested a review from a team October 3, 2023 20:03
@cturnbull-bitwarden cturnbull-bitwarden requested a review from a team as a code owner October 3, 2023 20:03
Thomas-Avery
Thomas-Avery previously approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

SM changes look good.

@bitwarden-bot
Copy link

bitwarden-bot commented Oct 3, 2023 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – eed4f1b6-c585-462f-8bd5-f17cdbf4ce25

No New Or Fixed Issues Found

@eliykat
Copy link
Member

eliykat commented Oct 4, 2023

This will cause conflicts with #3164; also the improvements in that PR would avoid all the additional duplicated Plan objects here. I suggest either waiting until that is merged, or if you can't wait for that, can you please take over the subsequent reviews for #3164 as it will require additional changes if this is merged first.

@cturnbull-bitwarden cturnbull-bitwarden requested a review from a team as a code owner October 6, 2023 17:44
@cturnbull-bitwarden cturnbull-bitwarden marked this pull request as draft October 6, 2023 17:51
@cturnbull-bitwarden
Copy link
Contributor Author

This will cause conflicts with #3164; also the improvements in that PR would avoid all the additional duplicated Plan objects here. I suggest either waiting until that is merged, or if you can't wait for that, can you please take over the subsequent reviews for #3164 as it will require additional changes if this is merged first.

Thanks for the heads-up, @eliykat! We had planned to sneak this in before @cyprain-okeke's refactor, but now we need to make additional refactors that would further block his work. We're going to switch up the priorities to get his work in first, then rebase this on top of his work. Moving this to be a draft for now

@cturnbull-bitwarden cturnbull-bitwarden changed the base branch from master to ac-1451/refactor-staticstore-plans-and-consuming-logic October 16, 2023 19:51
@cturnbull-bitwarden cturnbull-bitwarden marked this pull request as ready for review October 16, 2023 19:52
Base automatically changed from ac-1451/refactor-staticstore-plans-and-consuming-logic to master October 17, 2023 14:56
@cyprain-okeke cyprain-okeke requested a review from a team as a code owner October 17, 2023 14:56
@cyprain-okeke cyprain-okeke requested a review from Jingo88 October 17, 2023 14:56
cyprain-okeke
cyprain-okeke reviewed Oct 18, 2023
View reviewed changes
Copy link
Contributor

@cyprain-okeke cyprain-okeke left a comment

Choose a reason for hiding this comment

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

Looks Good

src/Core/Models/StaticStore/Plan.cs Show resolved Hide resolved
cyprain-okeke
cyprain-okeke previously approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@cyprain-okeke cyprain-okeke left a comment

Choose a reason for hiding this comment

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

LGTM

@cturnbull-bitwarden cturnbull-bitwarden removed request for a team and Jingo88 October 18, 2023 12:51
cyprain-okeke
cyprain-okeke previously approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@cyprain-okeke cyprain-okeke left a comment

Choose a reason for hiding this comment

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

Looks Good

Thomas-Avery
Thomas-Avery previously approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

LGTM

@bitwarden-devops-bot bitwarden-devops-bot temporarily deployed to QA Cloud October 24, 2023 12:13 Inactive
@cturnbull-bitwarden cturnbull-bitwarden mentioned this pull request Oct 30, 2023
Merged
Thomas-Avery
Thomas-Avery previously approved these changes Oct 30, 2023
View reviewed changes
cyprain-okeke
cyprain-okeke previously approved these changes Oct 30, 2023
View reviewed changes
r-tome
r-tome previously approved these changes Oct 31, 2023
View reviewed changes
@bitwarden-devops-bot bitwarden-devops-bot temporarily deployed to EU-QA Cloud October 31, 2023 15:14 Inactive
@cturnbull-bitwarden
Resolved issue where getting plans would return a value that LINQ pre…
6c140da 
…viously cached when feature flag was in a different state
@bitwarden-devops-bot bitwarden-devops-bot temporarily deployed to EU-QA Cloud October 31, 2023 18:07 Inactive
@cturnbull-bitwarden
Merge branch 'master' into AC-1650-AC-1578-Plans
e28d3a6 
# Conflicts:
#	src/Core/Constants.cs
@bitwarden-devops-bot bitwarden-devops-bot temporarily deployed to EU-QA Cloud November 1, 2023 02:01 Inactive
@cturnbull-bitwarden cturnbull-bitwarden enabled auto-merge (squash) November 1, 2023 12:40
@cturnbull-bitwarden cturnbull-bitwarden merged commit f9fc43d into master Nov 1, 2023
44 of 45 checks passed
@cturnbull-bitwarden cturnbull-bitwarden deleted the AC-1650-AC-1578-Plans branch November 1, 2023 12:43
cturnbull-bitwarden added a commit that referenced this pull request Nov 1, 2023
@cturnbull-bitwarden
[AC-1650] [AC-1578] (#3320)
61a393a 
* Upgraded old 2019 plans to have the same features as 2020 and beyond

* Removed redundant test and moved additional test cases to GetByOrgIdAsync_SmNoneFreePlans_ReturnsNull

* Fixed issue where feature flag wasn't returning correct plans

* Resolved issue where getting plans would return a value that LINQ previously cached when feature flag was in a different state

---------

Co-authored-by: cyprain-okeke <[email protected]>
# Conflicts:
#	src/Core/Constants.cs
@bitwarden-devops-bot bitwarden-devops-bot temporarily deployed to EU-QA Cloud November 1, 2023 20:03 Inactive
@bitwarden-devops-bot bitwarden-devops-bot temporarily deployed to EU-QA Cloud November 1, 2023 20:35 Inactive
cturnbull-bitwarden added a commit that referenced this pull request Nov 3, 2023
@cturnbull-bitwarden
[AC-1650] [AC-1578] (#3320)
8084a94 
* Upgraded old 2019 plans to have the same features as 2020 and beyond

* Removed redundant test and moved additional test cases to GetByOrgIdAsync_SmNoneFreePlans_ReturnsNull

* Fixed issue where feature flag wasn't returning correct plans

* Resolved issue where getting plans would return a value that LINQ previously cached when feature flag was in a different state

---------

Co-authored-by: cyprain-okeke <[email protected]>
# Conflicts:
#	src/Core/Constants.cs
@cturnbull-bitwarden cturnbull-bitwarden mentioned this pull request Nov 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r-tome r-tome r-tome approved these changes

@cyprain-okeke cyprain-okeke cyprain-okeke approved these changes

@Thomas-Avery Thomas-Avery Thomas-Avery 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.
7 participants
@cturnbull-bitwarden @bitwarden-bot @eliykat @Thomas-Avery @cyprain-okeke @r-tome @bitwarden-devops-bot