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

Fallback to resources class / link to child resource feature #1398

Merged
merged 2 commits into from
Nov 8, 2022

Conversation

elassadi
Copy link
Contributor

@elassadi elassadi commented Nov 4, 2022

Description

Link to child resources

A small bug.
if the parent resource is the same as the base class or if the child class is missing we fallback to the parent class.
We may as well raise an exception ?

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Manual review steps

  1. Step 1
  2. Step 2

Manual reviewer: please leave a comment with output from the test if that's the case.

@codeclimate
Copy link

codeclimate bot commented Nov 4, 2022

Code Climate has analyzed commit f5248b3 and detected 0 issues on this pull request.

View more on Code Climate.

@elassadi elassadi changed the title Fallback to resources class Fallback to resources class / link to child resource feature Nov 4, 2022
@adrianthedev
Copy link
Collaborator

Hey @elassadi. Can you share an example where this bug occurs in the dummy app?

elassadi added a commit to elassadi/avo that referenced this pull request Nov 6, 2022
@elassadi
Copy link
Contributor Author

elassadi commented Nov 6, 2022

I just prepared https://github.com/elassadi/avo/tree/demo/reference-to-base-class to show the case

elassadi added a commit to elassadi/avo that referenced this pull request Nov 6, 2022
@adrianthedev
Copy link
Collaborator

OK! Got it! I see the issue and the fix now. Thank you!

Can I ask you to add a spec for this so we can bullet proof it?

@elassadi
Copy link
Contributor Author

elassadi commented Nov 7, 2022

I don't see any useful specs to cover a missing Model.
I am not even sure if we should not let the exception being raised
But since it will affect parent resources as well. I opt to use a rescue (fallback) in our case.
If I got some time, I will think about adding meaningful specs, for now I don't see any

@adrianthedev
Copy link
Collaborator

I think it would be enough adding that Brother model, seed the DB with a record, and make sure the page does not crash like before.

I'll look into it and see if I can add one.

@adrianthedev adrianthedev merged commit ae9a82c into avo-hq:main Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants