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

feat: settings to allow new networks and show notif when a network is disallowed #1658

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

Conversation

diivi
Copy link
Contributor

@diivi diivi commented Mar 17, 2023

Adds 2 columns to the profile model:

  1. shows the "This wifi is not allowed" notification only when it is checked. Shows the notif by default.
  2. adds new networks to allowed networks only when checked. Does not add by default.

Related Issue

Fixes #1655

Motivation and Context

#1654

How Has This Been Tested?

Manually tested.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING guide.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.

@diivi diivi marked this pull request as ready for review March 17, 2023 12:38
src/vorta/application.py Outdated Show resolved Hide resolved
src/vorta/store/migrations.py Outdated Show resolved Hide resolved
src/vorta/views/schedule_tab.py Outdated Show resolved Hide resolved
@diivi diivi requested a review from real-yfprojects March 19, 2023 10:52
Copy link
Collaborator

@real-yfprojects real-yfprojects left a comment

Choose a reason for hiding this comment

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

LGTM. I didn't test the new notification setting.

@real-yfprojects real-yfprojects requested a review from m3nu March 21, 2023 10:09
src/vorta/application.py Outdated Show resolved Hide resolved
@m3nu
Copy link
Contributor

m3nu commented Apr 5, 2023

I feel like "Show notification when a network is disallowed" duplicates the option that's already there under Misc - "Display notification when background tasks fails". If you look at notifications and backups in general:

  1. It may succeed: we have a notification option for that
  2. It may fail for some random reason: we have a notification for that too
  3. It may fail due to disallowed network: this PR

The question is if 2 and 3 are different enough to have their own option. Or if 3 is just some other failure. In any case, the option is better suited under Misc as it's so similar to the others.

@diivi
Copy link
Contributor Author

diivi commented Apr 5, 2023

@bcelary what do you think (about the difference between 2 and 3)? Since you requested this in the first place.

Otherwise, yeah, maybe this should be under Misc.

@bcelary
Copy link

bcelary commented Apr 6, 2023

Seems like misc is the right place to keep it consistent. It looks like this is really a different category of a message that is not really an error but like an info. Similarly there could be an error that there is no available network at all which seems pointless because user is likely aware of this (not sure if there's such a message but just as an example).

Caveat here that I'm not an UX expert.

@real-yfprojects
Copy link
Collaborator

I think 2 and 3 can be merged to one notification.

@diivi
Copy link
Contributor Author

diivi commented Apr 6, 2023

I think 2 and 3 can be merged to one notification.

So I should drop the second part of this PR entirely, and @bcelary can use the Misc - Display notification when background tasks fails setting to disable wifi notifications?

It looks like this is really a different category of a message that is not really an error but like an info

I agree; but a different setting for this use case probably only makes sense if there's an overlap, i.e. you want to hide an error, not the wifi notification (or vice versa), but end up hiding both because of the current setting.

@bcelary
Copy link

bcelary commented Apr 6, 2023

I realize that too many options is not ideal so I'm definitely good with whatever you decide.

Maybe some criticality should be added to notifications so that user would be able to decide the level of severity but that would require a new feature. I think the most sensible is to simply drop that part indeed.

@diivi
Copy link
Contributor Author

diivi commented Apr 7, 2023

What should I do about this - #1658 (comment)?

@real-yfprojects
Copy link
Collaborator

What should I do about this - #1658 (comment)?

Didn't we discuss that we treat blocked wifi like any other error?

@diivi diivi requested a review from real-yfprojects April 13, 2023 06:57
@real-yfprojects
Copy link
Collaborator

Can you upgrade to PyQt6 (#1685)?

@@ -97,6 +97,7 @@ class BackupProfileModel(BaseModel):
pre_backup_cmd = pw.CharField(default='')
post_backup_cmd = pw.CharField(default='')
dont_run_on_metered_networks = pw.BooleanField(default=True)
allow_new_networks = pw.BooleanField(default=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it defaults to False, but in migrations it's True? Suggest to use True everywhere, which is the current behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think yf and I discussed this before (been long so idr exactly where, I tried searching). The new behaviour should not surprise existing users, so the migration changes nothing. But for new users, having this value as false is more beneficial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

found it - #1658 (comment)

@m3nu
Copy link
Contributor

m3nu commented Jun 23, 2023

This is almost ready.

@m3nu m3nu self-assigned this Jun 23, 2023
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.

Settings for rejecting WIFIs by default and for showing WIFI not allowed messages.
5 participants