-
Notifications
You must be signed in to change notification settings - Fork 10
Fix PiCT reported bugs + build enhancements (14) #46
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
Conversation
Hi @tate1650 - Could you please keep this PR open and refrain from merging until I give the word? |
Hello @tate1650 - Please note that the Online Shopping feature is still a WIP, so we've implemented a toggle system to hide it until it is determined production' ready. To ensure everything is set up correctly, you will need to create a record in the new toggles DB collection with the toggle name "onlineShoppingFeature" as shown below -> You can easily add this using the Swagger UI tool (once it's fixed) with the same admin credentials used for the Sanctioned Seller Reporting [see screenshot below]. Alternatively, you may add the record manually. I'll be around if you need assistance. Thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lookin' good! Left a few suggestions, most of which are not blocking a merge. There are two I'd like to address before merging though:
- Making sure anything online shopping-related is protected by a toggle-check on the BE. Unless the feature is in a somewhat stable state, in which case we can just aim to have that done before it goes to prod ("somewhat stable" meaning if someone like a mod is messing around and hits that endpoint then nothing in the app will break)
- If y'all have any interest in pursuing the embedded-SellerItem schema, are you all comfortable with the
seller_items
collection being dropped later on staging? We'd probably need to make the change from reference --> embedding before the model goes to prod. (Note that this item is totally irrelevant if you'd like to keep using the referenced collection instead).
Thanks for the feedback @tate1650 - |
Since I can’t submit a formal review, I'll just drop a comment here. I noticed that seller_id is being queried in SellerItem, but I don’t see an index on it. Bringing it up since it was flagged earlier and could impact performance. Also, are the /item/* routes (like add/delete) behind a feature toggle such as onlineShoppingFeature? I didn’t spot a check in the controller, wondering if that’s handled elsewhere. Otherwise, things are looking solid! |
Hi @DarinHajou - please review the PR @ map-of-pi/map-of-pi-backend-react#254 which addresses Tate's feedback. Thx! |
…E + fix N+1 query + add indexing to seller item.
Hello @tate1650 - PR has been updated per your feedback. Please review the changes accordingly. I will be standing by. Thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Left one tiny comment on error messages, and had a sudden lightbulb moment about toggle stuff. If you wanna merge this and address these two items in a follow up PR I'm fine with that. Lemme know what you'd prefer to do.
I've addressed your latest PR comments - please feel free to take another look. |
Hi @tate1650 - we'll be adding a quick FE change to this PR; please refrain from merging for the time being until I give the word. Thanks again! |
[see commits]
This PR is subject to change if additional fixes + enhancements are introduced before approval.