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

fix 2380 issue #2397

Closed
wants to merge 1 commit into from
Closed

Conversation

JisanAR03
Copy link
Contributor

Impact:

  • Existing company details in the main application may be lost if we proceed with the new model changes.
  • Modifying the endpoint to incorporate the new company ID will require significant time and effort, as all related endpoints will need to be updated.

Proposed Solution:
Rather than making extensive changes to the model and endpoints, we can resolve the issue by simply adding the company ID to the current company model. This solution is straightforward and will preserve the existing company details and functionality.

Conclusion:
The company ID is already a built feature used in various views. Therefore, we should avoid changing the model structure and opt for the simpler solution of adding the company ID to the existing model.

@DonnieBLT , @AtmegaBuzz , @arkid15r .. can you please review and any suggestion about what i did wrong ?

Copy link
Contributor

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

I guess you know my opinion based on previous PR review. If you can't use existing company.id add another field. I don't think it should be named company_id though as it's already Company model.

In general I don't mind have this as a temporary solution. It seems the codebase already has many of them.

@JisanAR03
Copy link
Contributor Author

I guess you know my opinion based on previous PR review. If you can't use existing company.id add another field. I don't think it should be named company_id though as it's already Company model.

In general I don't mind have this as a temporary solution. It seems the codebase already has many of them.

@arkid15r , actually sir , i did that cause initially I can't start working on company stuff without fixing this error , that's why I make this changes

@DonnieBLT
Copy link
Collaborator

Not sure if my comments are being seen. Please do not use UUID. Let's use a company_slug and the built in integer id as unique. id can be the primary key (it should already be built in) and slug is unique. There is one record we can delete so don't worry about the data.

@JisanAR03
Copy link
Contributor Author

Not sure if my comments are being seen. Please do not use UUID. Let's use a company_slug and the built in integer id as unique. id can be the primary key (it should already be built in) and slug is unique. There is one record we can delete so don't worry about the data.

ok cool , if we don't have to worry about the data , then I can complete whole stuff my own with help of @arkid15r mentorship . thanks a lot @DonnieBLT

@JisanAR03 JisanAR03 closed this Jul 4, 2024
@JisanAR03 JisanAR03 mentioned this pull request Jul 5, 2024
@JisanAR03 JisanAR03 deleted the company_error_fix branch July 9, 2024 15:22
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.

3 participants