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

Account for hard-deleted models, suppliers in asset history #13565

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

snipe
Copy link
Owner

@snipe snipe commented Sep 5, 2023

This catches the scenario where an asset had been previously classified with a model ID or supplier ID that was subsequently hard-deleted.

I think this transformer still needs to be refactored though. I'm seeing the n+1 query problem I mentioned a few weeks ago.

Screenshot 2023-09-05 at 4 02 03 PM

@what-the-diff
Copy link

what-the-diff bot commented Sep 5, 2023

PR Summary

  • Improved Error Handling For Deleted Models
    Instead of showing an error when a model has been deleted, the system now displays the name of the removed model. This feature enhances user experience by providing smoother navigation through the systems.

  • Enhanced Interaction with Removed Suppliers
    The system is now able to show the name of a deleted supplier instead of an error message. The added functionality will allow users to understand their past interactions better.

  • Internationalization Support for Asset Models
    A translation string for the phrase "Deleted asset model" has been added to the English language file. This inclusion supports our continued efforts towards improving accessibility and inclusivity for all users.

  • Translation Support for Suppliers
    A new translation string for "Deleted supplier" has been included in the English language file. This addition helps to accommodate users of different linguistic backgrounds for greater accessibility.

@snipe snipe requested a review from uberbrady September 5, 2023 15:03
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

This makes sense to me. I'm not sure I agree with you on the N+1 query problem needing to be solved here, but I can be convinced. But in the meantime, this seems like a somewhat safer way to go, moving forward.

@snipe
Copy link
Owner Author

snipe commented Sep 5, 2023

@uberbrady That endpoint is showing 53 queries - with an asset with a lot of history, that could get messy.

@snipe snipe merged commit 26452a8 into develop Sep 5, 2023
@snipe snipe deleted the fixes/asset_history_500_on_hard_deleted_models branch September 5, 2023 15:06
@snipe
Copy link
Owner Author

snipe commented Sep 5, 2023

Merging this for now, just to fix the 500, but we should probably revisit the solution here.

Additionally, I kinda don't think we should be hot-querying that stuff anyway. I kind of wanted it to be "At this moment in time of the change, the asset model was called 'Blah'"

@snipe snipe changed the title Account for hard-deleted models, suppliers Account for hard-deleted models, suppliers in asset history Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants