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

PM-17006: Remove send custom deletion date option #1290

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

matt-livefront
Copy link
Collaborator

@matt-livefront matt-livefront commented Jan 21, 2025

🎟️ Tracking

PM-17006

📔 Objective

Removes the custom deletion date option from sends.

📸 Screenshots

New send:

Before After
Screenshot 2025-01-21 at 1 06 55 PM Screenshot 2025-01-21 at 1 03 04 PM

Edit send:

Before After
Screenshot 2025-01-21 at 1 07 02 PM Screenshot 2025-01-21 at 1 03 11 PM

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.45%. Comparing base (c734428) to head (2060e1a).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1290      +/-   ##
==========================================
- Coverage   89.47%   89.45%   -0.03%     
==========================================
  Files         733      733              
  Lines       46267    46224      -43     
==========================================
- Hits        41399    41351      -48     
- Misses       4868     4873       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Jan 21, 2025

Logo
Checkmarx One – Scan Summary & Details83dbfe4a-8e1d-4c57-9e68-623e7c6b2666

Great job, no security vulnerabilities found in this Pull Request

@@ -211,7 +211,7 @@ class AddEditSendItemProcessorTests: BitwardenTestCase { // swiftlint:disable:th
subject.state.name = "Name"
subject.state.type = .text
subject.state.text = "Text"
subject.state.deletionDate = .custom
subject.state.deletionDate = .custom(Date(year: 2023, month: 11, day: 5))
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓Could we just create a constant for this deletion date, since they are all same?

@@ -263,7 +263,7 @@ class AddEditSendItemViewTests: BitwardenTestCase { // swiftlint:disable:this ty
processor.state.fileName = "example_file.txt"
processor.state.fileData = Data("example".utf8)
processor.state.isHideTextByDefaultOn = true
processor.state.deletionDate = .custom
processor.state.deletionDate = .custom(Date(year: 2023, month: 11, day: 5, hour: 9, minute: 41))
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ same with this date, could we just create one, and reuse?

Comment on lines +131 to +133
case .edit:
[.oneHour, .oneDay, .twoDays, .threeDays, .sevenDays, .thirtyDays, .custom(customDeletionDate)]
}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't custom option be available only if the send is configured in such way? Lmk if I'm wrong but I think that as it's here the user could just create a send with 1 hour deletion date and then edit it to a custom one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The custom option in edit mode is slightly different than before. Previously, you could select custom and then use the calendar to change the date to anything. Custom now just shows the fixed date in the menu. This makes it so:

  1. If you previously had a custom deletion date, that would still be preserved.
  2. We don't have to do date math - meaning if you choose 3 days, the data model only stores the calculated date of 3 days from today as the deleted date. So when you go to edit that send, we don't need to back calculate that you choose 3 days and instead can show the deletion date as the fixed custom option in the menu.

Like this:
Screenshot 2025-01-21 at 1 03 11 PM

Copy link
Member

Choose a reason for hiding this comment

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

Oh missed that, thanks for explaining love it 🚀

@matt-livefront matt-livefront merged commit bb1c48b into main Jan 23, 2025
9 checks passed
@matt-livefront matt-livefront deleted the matt/PM-17006-remove-send-custom-deletion-date branch January 23, 2025 16:51
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.

3 participants