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

Allow groups to restrict by browser integration key (#6437) #9852

Merged
merged 1 commit into from
Jan 14, 2024

Conversation

ekimd
Copy link
Contributor

@ekimd ekimd commented Sep 18, 2023

Relates to issue #6437

This branch adds the ability to set a browser restriction on groups. This allows a given group tree to be visible only to a specific browser. Previously, the workaround was to have separate databases for each browser. However, this can become cumbersome under situations where someone may have a dozen or more browsers that each have to be unlocked whenever the screensaver activates.

Screenshots

image

Testing strategy

  • Added a new test function to test the new behavior

Type of change

  • ✅ New feature (change that adds functionality)

@ekimd ekimd force-pushed the restrict_browser_key branch 3 times, most recently from 42fbf09 to cdcea83 Compare September 18, 2023 04:26
@droidmonkey droidmonkey added this to the v2.8.0 milestone Sep 18, 2023
@ekimd ekimd force-pushed the restrict_browser_key branch 2 times, most recently from 2b64b4e to 6ffdaf2 Compare October 9, 2023 04:05
@ekimd
Copy link
Contributor Author

ekimd commented Oct 9, 2023

@droidmonkey I don't understand the codecov/patch failure--what do I need to do to fix that?
Thanks

@droidmonkey
Copy link
Member

We set an ideal threshold for new code introduced to be 75% covered by unit tests. It's optional though.

@ekimd
Copy link
Contributor Author

ekimd commented Oct 31, 2023

Thanks. I tried increasing the test coverage but couldn't find a clear path given that some of the parent functions in BrowserService aren't tested.

How do I request a code review?

@varjolintu
Copy link
Member

Could you solve the conflicts? Rebase the branch.

@ekimd ekimd force-pushed the restrict_browser_key branch from 6ffdaf2 to 3bf086e Compare October 31, 2023 17:55
@ekimd
Copy link
Contributor Author

ekimd commented Oct 31, 2023

Rebase complete

@varjolintu
Copy link
Member

Some of the code could be cleaned a little bit. For example:

  • Use QStringList instead if QList<QString> if possible.
  • Use auto instead of QString when creating a new variable.
  • Return {} instead of QString().
  • If function parameter has a default value, use {} again.

@ekimd ekimd force-pushed the restrict_browser_key branch from 3bf086e to 453b2d0 Compare October 31, 2023 18:42
@ekimd
Copy link
Contributor Author

ekimd commented Oct 31, 2023

Thanks for the fast response!

I think I caught them all, but let me know if there's anything else. I couldn't return {} in BroswerService.cpp:996 because I got a compiler error returning an initializer list from that lambda function.

@ekimd ekimd force-pushed the restrict_browser_key branch from 453b2d0 to a5d3cbb Compare October 31, 2023 18:51
@varjolintu
Copy link
Member

Tested this, and it works nicely. Only thing that caught my eye is the strange separator in the combobox:
Screenshot 2023-11-01 at 7 01 48

When looking at other comboboxes, the separators look much more smaller:
Screenshot 2023-11-01 at 7 01 30

@ekimd
Copy link
Contributor Author

ekimd commented Nov 1, 2023

Interesting. I think that's because the screenshot above is actually referencing a QMenu not a ComboBox (see EditWidgetIcons.cpp:170).

@varjolintu varjolintu self-requested a review November 4, 2023 08:01
Copy link
Member

@varjolintu varjolintu left a comment

Choose a reason for hiding this comment

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

Approved. We are missing a combobox separator style, but the scope is outside of this PR.

@ekimd
Copy link
Contributor Author

ekimd commented Nov 10, 2023

Anything else I need to do to get approval on this?

@ekimd
Copy link
Contributor Author

ekimd commented Nov 21, 2023

bump

@varjolintu
Copy link
Member

bump

Bump? This PR is already accepted.

@ekimd
Copy link
Contributor Author

ekimd commented Dec 7, 2023

Going to need some help here. There was a merge conflict so I resolved that but now one of the unit tests is failing and I'm not sure why. (It seems unrelated)

@droidmonkey
Copy link
Member

droidmonkey commented Dec 7, 2023

You can ignore, we have sporadic failures since some tests rely on randomness and infrequently don't produce the expected results.

@ekimd
Copy link
Contributor Author

ekimd commented Dec 7, 2023

Since I'm not familiar with the development cadence for this project, what is the typical timeline/workflow between approval, review, and merge?

@droidmonkey
Copy link
Member

I will merge it in when I get the chance. Unfortunately my time is very limited lately.

@droidmonkey droidmonkey merged commit 884386c into keepassxreboot:develop Jan 14, 2024
10 of 11 checks passed
@droidmonkey droidmonkey modified the milestones: v2.8.0, v2.7.10 Nov 28, 2024
@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Browser pr: backported Pull request backported to previous release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants