Feature/better error for missing resource #2062

bryszard · 2023-11-26T11:10:45Z

Description

Fixes # 1960

We're adding a better handling for errors when we encounter a missing resource on association fields.

Right now the error is unhelpful and requires developer to understand the internal logic of Avo.
We want to give a better Dev Experience and point to solutions.

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

Screenshots

Error:

UPDATED WITH NEW MESSAGE

Video

BEFORE APPLYING REVIEW SUGGESTIONS
https://github.com/avo-hq/avo/assets/12682792/3b4f14f8-700a-4770-ba37-cecd66683052

Manual review steps

Go to the dummy app to Teams page and choose any Team, add show_location_field=1 query param
On the Team show page go to the bottom and observe what happens with the slot for locations field.
Go to the Courses page and choose any Course, add show_location_field=1 query param
On the Course show page go to the bottom and observe what happens with the slot for locations field.
Go to the Stores page , add show_location_field=1 query param and see the error
Go to the Events page, add show_location_field=1 query param and see the error

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

codeclimate · 2023-11-26T11:11:12Z

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

View more on Code Climate.

adrianthedev · 2023-12-01T13:21:57Z

This looks great @bryszard! Thank you!
Would you be able to move it to the main branch so it will work with Avo 3?

bryszard · 2023-12-01T13:24:01Z

Definitely.

I have kind of a busy time, but I plan to finish it in the next several days.

adrianthedev

This looks great!

I think this catch can be added to BelongsToField.target_resource, HasBaseField.field_resource and HasBaseField.target_resource.

app/controllers/avo/application_controller.rb

adrianthedev · 2023-12-01T13:33:47Z

No worries. Take your time, and thank you for the contribution ❤️

spec/system/avo/exposed_methods_base_controller_spec.rb

bryszard · 2023-12-06T12:27:01Z

@Paul-Bob I'm ready for review, although I have a doubt:

Since I've merged the newest main I have a problem with the display of the dummy app. The resources are not grouped correctly as they used to and as they do when I check the Demo App. They are all displayed in one non-collapsable group in alphabetical order:

Do you know what could be the problem?

Paul-Bob · 2023-12-06T15:41:34Z

@bryszard

The resources are not grouped correctly as they used to

They are grouped correctly, give a quick read here https://docs.avohq.io/3.0/resource-panels.html

We refactored the way that items are fetched from resource on avo3 allowing more customization

Let me know if the docs are clear or if there are some thinks that can be better explained

PS: Thank you for the contribution. I'll review it during this week

adrianthedev · 2023-12-06T20:44:06Z

I'm checking your commits and you're a legend @bryszard.
Thank you for taking the time to fix these 💪
We appreciate it.

Paul-Bob

Great PR @bryszard! Thank you!!

I let a comment about removing the error suppression on tests, anything else it's great!

adrianthedev

I tweaked the message and the tests.

I noticed that you disabled the error catching, but I think it's important to not do that.
We can check if an error was raised. I pushed a sample on the test you created.

On the last two specs we should not disable the error catching.
We should take a different approach. Maybe like I did on events?
Show the field only if a param is present?
This way we won't get random crashes when we do other tests or navigate the app, but still be able to test that scenario.

One thing that I'm a bit afraid of is coupling of the database tables.
For example, you can't have an event without a location. So in reality in the UI we will never be able to create an event because it will fail on the new page (no location resource present).
How about making the location optional to the event?
Maybe the same with location and stores?

Thsi way they are not so coupled and will give us some leeway with creating records.

To do:

Make models less coupled
remove the error catching from the two unrelated tests
show the error fields only if a param is present so we can test them

This is solid progress!
Thank you!

Let's address the issues above and we'll be able to merge it very soon.

spec/system/avo/skip_show_view_spec.rb

spec/system/avo/resource_sidebar_spec.rb

adrianthedev · 2023-12-08T11:33:29Z

BTW @bryszard, are you on our Discord?

bryszard · 2023-12-08T13:19:53Z

BTW @bryszard, are you on our Discord?

Yes, Adrian, I am there :)

bryszard · 2023-12-10T21:20:36Z

Thank you guys for the review. I'm back from holiday and will be able to adjust it in the next days.

bryszard · 2023-12-12T22:58:14Z

@bryszard

The resources are not grouped correctly as they used to

They are grouped correctly, give a quick read here https://docs.avohq.io/3.0/resource-panels.html

We refactored the way that items are fetched from resource on avo3 allowing more customization

Let me know if the docs are clear or if there are some thinks that can be better explained

PS: Thank you for the contribution. I'll review it during this week

@Paul-Bob I think I found the reason - you've moved the Avo::Menu::Builder to the avo-menu plugin and it's under Pro licence (https://docs.avohq.io/3.0/menu-editor.html). When I run the dummy app I don't have it.

irb(main):006> Avo::Menu::Builder
(irb):6:in `<main>': uninitialized constant Avo::Menu (NameError)

bryszard · 2023-12-12T23:51:56Z

@adrianthedev I've applied all the suggestions, but there is one failing spec:

After moving the model_missing_resource_spec to spec/features now for some reason it is raising #<ActionController::RoutingError: No route matches [GET] "/admin/resources/events"> (eg. here)

The problem is that I cannot reproduce it locally with bin/test ./spec/features/avo/model_missing_resource_spec.rb. I also don't see a problem with that route when running bin/dev. Could you help me here? :)

Paul-Bob · 2023-12-13T14:06:38Z

@bryszard

The resources are not grouped correctly as they used to

They are grouped correctly, give a quick read here https://docs.avohq.io/3.0/resource-panels.html
We refactored the way that items are fetched from resource on avo3 allowing more customization
Let me know if the docs are clear or if there are some thinks that can be better explained
PS: Thank you for the contribution. I'll review it during this week

@Paul-Bob I think I found the reason - you've moved the Avo::Menu::Builder to the avo-menu plugin and it's under Pro licence (https://docs.avohq.io/3.0/menu-editor.html). When I run the dummy app I don't have it.
irb(main):006> Avo::Menu::Builder
(irb):6:in `<main>': uninitialized constant Avo::Menu (NameError)

Ah you meant the menu, yes, on avo versions > 3 the main branch contains mainly community version features, the pro/advanced features are distributed by packages.

Paul-Bob

@adrianthedev I've applied all the suggestions, but there is one failing spec:

After moving the model_missing_resource_spec to spec/features now for some reason it is raising #<ActionController::RoutingError: No route matches [GET] "/admin/resources/events"> (eg. here)

spec/features/avo/generators/resource_generator_spec.rb was overriding than deleting the event resource and controller I pushed a commit with a fix

Great PR this kind of feature will increase developer experience by far!

Thank you!

…nd BelongsToField

adrianthedev

Stellar work @bryszard!
This was a big pain point for Avo users, not getting enough feedback on what's wrong.

Thank you for your time and contribution.

github-actions bot added the Feature label Nov 26, 2023

Paul-Bob changed the base branch from main to 2.x November 27, 2023 10:36

Paul-Bob self-requested a review November 27, 2023 11:34

Paul-Bob assigned bryszard Nov 27, 2023

adrianthedev reviewed Dec 1, 2023

View reviewed changes

app/controllers/avo/application_controller.rb Outdated Show resolved Hide resolved

bryszard force-pushed the feature/better_error_for_missing_resource branch from f6d5ab9 to d54ea37 Compare December 6, 2023 08:20

bryszard changed the base branch from 2.x to main December 6, 2023 08:22

bryszard commented Dec 6, 2023

View reviewed changes

spec/system/avo/exposed_methods_base_controller_spec.rb Outdated Show resolved Hide resolved

bryszard marked this pull request as ready for review December 6, 2023 12:27

Paul-Bob requested changes Dec 7, 2023

View reviewed changes

adrianthedev requested changes Dec 8, 2023

View reviewed changes

spec/system/avo/skip_show_view_spec.rb Outdated Show resolved Hide resolved

spec/system/avo/resource_sidebar_spec.rb Outdated Show resolved Hide resolved

Paul-Bob requested review from adrianthedev and Paul-Bob December 19, 2023 08:05

Paul-Bob approved these changes Dec 19, 2023

View reviewed changes

bryszard added 3 commits December 19, 2023 14:26

Add locations table for an example

44adff4

Autofix annotations

fd88b73

Add association to Team

b99929e

bryszard and others added 26 commits December 19, 2023 14:26

Add newlines to the missing resource message

ce7782d

Move message to the MissingResourceError class

333e085

Catch errors for field_resource and target_resource on HasBaseField a…

5eaed6e

…nd BelongsToField

Add spec for the has_many and has_and_belongs_to_many error handling

aeafeb1

Fix schema version

e15a547

Add location_id reference to events for belongs_to showcase

287835b

Add Event resource and menu item in order to showcase belongs_to example

cf5ae07

Add single seeds for Location and Event

78fb4e6

Add factories for Event and Location

bb8458c

Move belongs_to :team to the Location instead of Event

bac64c8

Move missing resource error specs to one place

2bd47f0

Add spec for belongs_to case

6161fe8

Normalize name in the error message

872caad

Move migrations to the dummy app

8132ca5

Add comment to Event resource for consistency

bab373a

Add Store model and resource for has_one showcase

91f4b51

Adjust factories - add one for Store, remove city on Location

2c7b6d0

Rename spec file to model_missing_resource_spec

2a12594

Add spec for has_one case

87d0fb5

Ignore not related server errors which are handled in failing specs

e56f32e

Annotate new models

4522289

Make all associations nullable for flexibility

d5f5269

tweak error message

65cf80a

Hide locations under query param, remove suppression of server errors

8b27acd

schema

d2b4fef

fix resource_generator_spec

9bec1b8

adrianthedev force-pushed the feature/better_error_for_missing_resource branch from b2e0941 to 9bec1b8 Compare December 19, 2023 12:36

adrianthedev approved these changes Dec 19, 2023

View reviewed changes

adrianthedev merged commit f4813ed into avo-hq:main Dec 19, 2023
11 of 14 checks passed

adrianthedev mentioned this pull request Jan 3, 2024

More concise errors reporting for local developer errors #2311

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/better error for missing resource #2062

Feature/better error for missing resource #2062

bryszard commented Nov 26, 2023 •

edited

Loading

codeclimate bot commented Nov 26, 2023 •

edited

Loading

adrianthedev commented Dec 1, 2023

bryszard commented Dec 1, 2023

adrianthedev left a comment

adrianthedev commented Dec 1, 2023

bryszard commented Dec 6, 2023

Paul-Bob commented Dec 6, 2023 •

edited

Loading

adrianthedev commented Dec 6, 2023

Paul-Bob left a comment

adrianthedev left a comment

adrianthedev commented Dec 8, 2023

bryszard commented Dec 8, 2023

bryszard commented Dec 10, 2023

bryszard commented Dec 12, 2023 •

edited

Loading

bryszard commented Dec 12, 2023 •

edited

Loading

Paul-Bob commented Dec 13, 2023

Paul-Bob left a comment

adrianthedev left a comment

Feature/better error for missing resource #2062

Feature/better error for missing resource #2062

Conversation

bryszard commented Nov 26, 2023 • edited Loading

Description

Checklist:

Screenshots

Video

Manual review steps

codeclimate bot commented Nov 26, 2023 • edited Loading

adrianthedev commented Dec 1, 2023

bryszard commented Dec 1, 2023

adrianthedev left a comment

Choose a reason for hiding this comment

adrianthedev commented Dec 1, 2023

bryszard commented Dec 6, 2023

Paul-Bob commented Dec 6, 2023 • edited Loading

adrianthedev commented Dec 6, 2023

Paul-Bob left a comment

Choose a reason for hiding this comment

adrianthedev left a comment

Choose a reason for hiding this comment

To do:

adrianthedev commented Dec 8, 2023

bryszard commented Dec 8, 2023

bryszard commented Dec 10, 2023

bryszard commented Dec 12, 2023 • edited Loading

bryszard commented Dec 12, 2023 • edited Loading

Paul-Bob commented Dec 13, 2023

Paul-Bob left a comment

Choose a reason for hiding this comment

adrianthedev left a comment

Choose a reason for hiding this comment

bryszard commented Nov 26, 2023 •

edited

Loading

codeclimate bot commented Nov 26, 2023 •

edited

Loading

Paul-Bob commented Dec 6, 2023 •

edited

Loading

bryszard commented Dec 12, 2023 •

edited

Loading

bryszard commented Dec 12, 2023 •

edited

Loading