-
-
Notifications
You must be signed in to change notification settings - Fork 929
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
Upgrade to avo 3 #4342
Upgrade to avo 3 #4342
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4342 +/- ##
==========================================
- Coverage 96.97% 96.02% -0.95%
==========================================
Files 424 424
Lines 8892 8942 +50
==========================================
- Hits 8623 8587 -36
- Misses 269 355 +86 ☔ View full report in Codecov by Sentry. |
4653038
to
b160f51
Compare
b160f51
to
2641e91
Compare
360e348
to
18877eb
Compare
help: "A comment explaining why this action was taken.<br>Will be saved in the audit log.<br>Must be more than 10 characters." | ||
end | ||
|
||
# Would be nice if there was a way to force a field to show up as visible |
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.
cc @adrianthedev this seems to be something where avo3 is stricter than avo2, and it would be nice if there was a way to force a field to be visible even if avoiding thinks it shouldn't be
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.
Hey @segiddins. Can you please share what is the problem? Maybe I can figure out an easier fix for your use case and remove this monkeypatch.
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.
we have fields that we need to show up in forms, even though Avo considers them "computed" (because the underlying record has no setter for them)
bd5bf10
to
7e74bc8
Compare
@adrianthedev @simi this should be ready to review! Only thing is it would be nice to point to a release of avo that removes the activestorage dependency instead of pointing to a git commit |
84efb82
to
321a6c2
Compare
This is green and ready to ship, @simi please review |
👀 |
The latest release should have the AS dependency removed. In the next release we will remove literal types altogether so we'll have less problems like those. |
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 good, a couple minor things that seem like they need to be cleaned up. Thanks for making this happen!
Gemfile
Outdated
@@ -62,12 +62,20 @@ gem "faraday-multipart", "~> 1.0" | |||
gem "timescaledb", "~> 0.3" | |||
|
|||
# Admin dashboard | |||
gem "avo", "~> 2.53" | |||
# github needed until https://github.com/avo-hq/avo/pull/3215 is released |
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.
it was released 😅
Rakefile
Outdated
# TODO: remove this when we point back to a release version of Avo | ||
namespace :assets do | ||
task precompile: "avo:build-assets" | ||
end |
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.
this PR is back on an Avo release now right?
test/test_helper.rb
Outdated
# if avo_request_pattern.matches?(request) | ||
# { status: 200, body: { id: :pro, valid: true, payload: {} }.to_json, | ||
# headers: { "Content-Type" => "application/json" } } | ||
# end |
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.
Do we need the comment? I’d rather remove things that don’t run if we can
I still have eyes on this PR, so feel free to ping me if I can help. |
321a6c2
to
32f18c6
Compare
@simi @colby-swandale just waiting on y'all to review to ship this, it needs a rebase almost every time we update deps |
# allow dotenv to specify RAILS_GROUPS | ||
if defined?(Dotenv::Rails) | ||
Dotenv::Rails.load | ||
Bundler.require(*Rails.groups) |
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.
Any chance we can move the duplicate Bundler.require
or is this required for Dotenv?
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.
It's required to allow .Env to control the rails groups
@segiddins apologies, I've been a bit under the weather the past few days. I merged #4883 today, so you will need to rebase once again, apologies 🙇🏻 |
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
32f18c6
to
56325b1
Compare
Awesome @segiddins! |
No description provided.