Skip to content

Restructure id logic and move private apps to plugins_data collection#1285

Merged
beastoin merged 16 commits intomainfrom
improve-id
Nov 14, 2024
Merged

Restructure id logic and move private apps to plugins_data collection#1285
beastoin merged 16 commits intomainfrom
improve-id

Conversation

@mdmohsin7
Copy link
Copy Markdown
Member

@mdmohsin7 mdmohsin7 commented Nov 11, 2024

  • Use ULID for ID
  • Store private and public plugins in plugin_data
  • Migrate existing private plugins to plugin_data
  • Cleanup some of the unused functions
  • Redirect some of the plugins endpoints to use apps functions
  • Fix analytics for new apps
  • Test with old and new apps

Part of #1290

@beastoin
Copy link
Copy Markdown
Collaborator

man, for the auto generated id, pls use:
1/ slugify
2/ if existed, + ulid
3/ should have the length limits(min/max)

@mdmohsin7

@mdmohsin7
Copy link
Copy Markdown
Member Author

1/ Yes slugifying it
2/ If id already exists, then I'm appending 5 random numbers to it at the end. adding ulid makes more sense
3/ minimum 1 works as well. The max will depend on the name of the plugin. We should ideally restrict the name to certain length?

@beastoin

@mdmohsin7
Copy link
Copy Markdown
Member Author

I'll change to use ulid in the next PR, can we have these minor changes merged for now?

@beastoin
Copy link
Copy Markdown
Collaborator

1/ find a better implementation pls, too manny failures with your code
2/ ulid is optional, add the incrementing number is good enough.
3/ restrict the id's length pls. min 5 max 128 chars.

@mdmohsin7

@mdmohsin7
Copy link
Copy Markdown
Member Author

1/ Thinking of using this python package for it https://pypi.org/project/python-slugify/
2/ That would mean storing a separate counter or iteratively checking for the incremented id one by one?
3/ Alright will do

@beastoin

@beastoin
Copy link
Copy Markdown
Collaborator

1/ good enough
2/ we dont do leetcode here, btw think it a bit more more pls 🌚

@mdmohsin7

@mdmohsin7
Copy link
Copy Markdown
Member Author

2/ I only did one leetcode problem in my life haha. Thinking of storing the most recently incremented id in a firestore collection

@beastoin
Copy link
Copy Markdown
Collaborator

2/ our plugin is not that big yet, just count()+1

@mdmohsin7

@mdmohsin7
Copy link
Copy Markdown
Member Author

2/ Oh okay lol. slugified name + doc_count + 1

If the app is private, I am appending -private to them. Not making any changes to this btw

@mdmohsin7 mdmohsin7 changed the title Check for slashes in names for app id Restructure id logic and move private apps to plugins_data collection Nov 12, 2024
@beastoin
Copy link
Copy Markdown
Collaborator

4/ please run a test for https://github.com/BasedHardware/omi/pull/1285/files#diff-e9334601001806a62e30da99b753c713c2c20213a661ced0a88542e66e39697eR26 / since i encounter the error with similar code at #1300 (comment)

5/ you could simplify the database > apps by using a wrapper func in utils > app

  • get_available_apps(uid: str|None) -> List to list all not deleted, approved apps + private apps owned by uid
  • get_available_app_by_id(id: str, uid: str|None) -> App to get 01 not deleted, approved app or 01 private app owned by uid

@mdmohsin7

@mdmohsin7
Copy link
Copy Markdown
Member Author

4/ Though I did not get that error when running locally, will try to check
5/ Renaming get_apps_data_from_dbto get_available_apps because it already does the same thing in utils. Creating a new func in utils named get_available_app_by_id which will call get_app_by_id_db in db/apps

@beastoin

@mdmohsin7
Copy link
Copy Markdown
Member Author

Did the suggested changes and used BaseCompositeFilter for AND filter for multiple where queries @beastoin

@beastoin
Copy link
Copy Markdown
Collaborator

lgtm @mdmohsin7 🥳 pls resolve the code conflict

@mdmohsin7
Copy link
Copy Markdown
Member Author

lgtm @mdmohsin7 🥳 pls resolve the code conflict

Resolved 🥳

@beastoin beastoin merged commit 02f91d5 into main Nov 14, 2024
@beastoin beastoin deleted the improve-id branch November 14, 2024 06:48
beastoin added a commit that referenced this pull request Nov 14, 2024
Glucksberg pushed a commit to Glucksberg/omi-local that referenced this pull request Apr 28, 2026
…BasedHardware#1285)

- [x] Use ULID for ID
- [x] Store private and public plugins in `plugin_data`
- [x] Migrate existing private plugins to `plugin_data`
- [x] Cleanup some of the unused functions
- [x] Redirect some of the plugins endpoints to use apps functions
- [x] Fix analytics for new apps
- [x] Test with old and new apps

Part of BasedHardware#1290
Glucksberg pushed a commit to Glucksberg/omi-local that referenced this pull request Apr 28, 2026
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