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

Display limits of email recovery #3281

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

florentos17
Copy link
Collaborator

tackling the issue 3219 (duplicated with the issue 3274)

@florentos17
Copy link
Collaborator Author

florentos17 commented Nov 18, 2024

This curious thing is happening: when accessing the recover deleted messages panel, at first an error is thrown, but when i trigger a hot reload, then everything is fine:

IMG_0450.mov

the method is this:

String getRestorationHorizon(BuildContext context) {
    return ((_session
        ?.props[0] as Map<CapabilityIdentifier, CapabilityProperties>?)
        ?[capabilityDeletedMessagesVault]
        ?.props[0] as Map<String, dynamic>) // line causing the error
        ['restorationHorizon'];
  }

and the error is Type 'Null' is not a subtype of type 'Map<String, dynamic>' in type cast

Copy link

This PR has been deployed to https://linagora.github.io/tmail-flutter/3281.

@florentos17 florentos17 force-pushed the Display-Limits-of-Email-Recovery branch from 7ba818f to aa8edd1 Compare November 20, 2024 15:08
@florentos17 florentos17 force-pushed the Display-Limits-of-Email-Recovery branch from aa8edd1 to de6b5e4 Compare November 20, 2024 15:11
@florentos17
Copy link
Collaborator Author

florentos17 commented Nov 20, 2024

the issue i had the other day is now resolved, everything is working fine ! it looks like this
Screenshot from 2024-11-20 16-05-37

in the web version, the options in the dropdown are filtered out so that "30 days" and more do not show if the horizon is 15 days for example. but in the mobile version, the period is selected with this widget (same as the one we get by clicking the little calendar icon in the web version) and i did not want to change the suggested options because the widget is also used for other features.
Screenshot from 2024-11-20 16-05-45

@florentos17 florentos17 marked this pull request as ready for review November 21, 2024 09:43
@chibenwa
Copy link
Member

@hoangdat is this work ok?

@florentos17 could you rebase this?

@florentos17
Copy link
Collaborator Author

if it's all good for you then i'll squash and rebase

@hoangdat
Copy link
Member

  • Must have verifying step after user select Deletion date. In this case, I choose the time range for the last 1 year.
image


// this class is a local copy of the one found at
// https://github.com/desktop-dart/duration/blob/master/lib/src/parse/parse.dart#L6
static Duration parseDuration(String input, {String separator = ','}) {
Copy link
Member

Choose a reason for hiding this comment

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

why we need this? how about using dependency directly?

Copy link
Member

Choose a reason for hiding this comment

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

I remembered it was asked by @tddang-linagora

Copy link
Contributor

Choose a reason for hiding this comment

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

We should minimize the using of third party library. If we only need a small function, let's copy it over.

Copy link
Member

Choose a reason for hiding this comment

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

from my point of view, we should use 3rd party lib to get the support from the community, don't need effort in maintaining it.

Copy link
Member

Choose a reason for hiding this comment

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

Tend to agree with @hoangdat .

Why maintain this code if we have it from a healthy package (size, community, etc..)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'll drop the commit removed package duration usages when squashing then (if ok for you @tddang-linagora)

@florentos17
Copy link
Collaborator Author

florentos17 commented Nov 28, 2024

  • Must have verifying step after user select Deletion date. In this case, I choose the time range for the last 1 year.

it is already done on the backend side (the request is modified to be sure it complies with the calendar limit if not already).

but you are absolutely right, we could add a check here on the frontend as well: circle the date in red if invalid and/or refuse to send the request if invalid.

but do you think it’s necessary ? the user is already warned with the yellow banner, and it’s a tiny and hidden feature, so maybe it could be ok to leave it like that ?

@hoangdat
Copy link
Member

but do you think it’s necessary ? the user is already warned with the yellow banner, and it’s a tiny and hidden feature, so maybe it could be ok to leave it like that ?

  • the best solution: prevent user to select the date range out of the limitation, but for it, huge effort in refactoring the data picker.
  • the acceptable solution: warning for unaccepted picked date.

Btw, when I push the recovery request with date time out of limitation, back-end still response the completed status with 0 completion, 0 .... We should have better response for this.

@chibenwa
Copy link
Member

the acceptable solution: warning for unaccepted picked date.

Technically with the yellow banner that's already the case.

The option is quite hidden (who knows where deleted message menu is) and is niche. 1% of users will open dead letter menu item. 1% will manually select a date. So we are speaking of user experience for 0.01%.

Are we lying to the user? No, backend will transparently filter data, which is what the banner says. Acceptable.

Can we please move on?

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.

4 participants