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

Refer to groups by UUID #1078

Merged

Conversation

orange-elephant
Copy link
Contributor

Happy new year!

For now entries can only be added to 1 group, but basically everything needed to make adding to multiple possible is already laid down here, and I'll be happy to work on that if this PR is merged.

@alexbakker
Copy link
Member

alexbakker commented Jan 15, 2023

Awesome work as always! Unfortunately I haven't had time to look at this yet, and I may not be able to get to it in the next couple of weeks either. Sorry about that. Starting February I should have some more time for Aegis.

Copy link
Member

@alexbakker alexbakker left a comment

Choose a reason for hiding this comment

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

Apologies for the delay. I've added some initial comments. Don't be afraid to push back if you think I'm asking for something that wouldn't work well. With a large patch like this it's easy to miss details.

@orange-elephant orange-elephant force-pushed the entries-in-multiple-groups branch 2 times, most recently from b6c1f80 to 5bd190d Compare April 30, 2023 15:39
@orange-elephant
Copy link
Contributor Author

I've finally gotten round to addressing the feedback, everything you suggested was okay. The only thing I'm still not sure about is how to get the "New group" button into the dropdown checkboxes so I've reverted that to the original approach for now and see if I can come up with a workaround.

It would probably be possible to migrate entries that claim to be in a group that has been discarded due to duplicate names to the UUID of the group of the same name which was kept but this would be a super rare circumstance and the user could easily add them back to the group so I'm not sure if that would be worth it, what do you think?

@alexbakker alexbakker force-pushed the entries-in-multiple-groups branch 5 times, most recently from 9ff4c7b to ba91010 Compare August 16, 2023 21:10
@alexbakker
Copy link
Member

Finally had some time to look at this again. Overall I'm pretty happy with this, but I'd like to take another look at the migration logic later this week before merging this, because I don't think I fully understand it yet. For example, I don't understand why we need to call Vault.migrateOldGroupIfNecessary(_vault, entry); in VaultRepository.addEntry(VaultEntry entry).

Also, I've made a couple of minor changes here and there, I hope you don't mind.

@orange-elephant
Copy link
Contributor Author

why we need to call Vault.migrateOldGroupIfNecessary(_vault, entry); in VaultRepository.addEntry(VaultEntry entry).

This was because addEntry is used instead of Vault.fromJson when importing a JSON vault file (at least anytime other than the intro slides) so if the JSON backup had an old style group then this needs to be converted when it is added to the vault. This conversion may only need to happen if the entry is being added from ImportEntriesActivity since I don't think its possible for QR codes to contain info about a group.

Also, I've made a couple of minor changes here and there, I hope you don't mind.

No problem, the group model is much better than what I had

@alexbakker alexbakker force-pushed the entries-in-multiple-groups branch 8 times, most recently from c346507 to 48c74a2 Compare September 11, 2023 20:27
 - Also lays the foundations for adding entries to multiple groups and changing group names

Co-authored-by: Alexander Bakker <[email protected]>
Copy link
Member

@alexbakker alexbakker left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@alexbakker alexbakker merged commit 305e157 into beemdevelopment:master Sep 11, 2023
4 checks passed
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.

2 participants