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

Add ActiveRecord::Store Support #98

Merged
merged 11 commits into from
Sep 23, 2021

Conversation

jacobdaddario
Copy link
Contributor

This is a start of a PR to address #95. I'm not totally familiar with how Madmin works internally, so I think I just need to address this at the generator level. My understanding is after the Madmin class is generated, the super classing just shoves the generated attributes into the attributes class attribute using the attributes API. At least that's as far as I understood.

Regardless, this still needs tests, so let me know if there's something I missed. I poked around a bit, but I was a bit time-pressured tonight.

@excid3
Copy link
Owner

excid3 commented Sep 15, 2021

I think this looks like a great start @jacobdaddario! 🙏

We'll have to add an example in the dummy app to test against.

You're exactly right about how it works. We basically load the model, then poke around to find all the attributes (which includes columns and virtual attributes) and then filter out what we don't want to display.

You set it up right because we want to generate fields for the store_accessors, but not the json column itself since you won't generally be editing the raw JSON.

Let's add a column or model to the dummy app and a store_accessor or two.

I don't have great tests for the resource generator yet. I mostly just run them in the dummy app and confirm it added the fields correctly right now.

@excid3 excid3 added the enhancement New feature or request label Sep 15, 2021
@jacobdaddario
Copy link
Contributor Author

Awesome! Thanks for the super detailed response. It gives me great direction. I’ll get the tests going ASAP. I’ll shoot for this weekend at the latest.

@jacobdaddario
Copy link
Contributor Author

I've added some test data, but for some reason the dummy app is erroring on me. I'm getting an error mentioning that the attribute PostResource doesn't exist on the Madmin post resource. What's weird is that I didn't change any code there, and the error is highlighting the attribute :tags line in the resource file.

Any idea what's going on with this?

@excid3
Copy link
Owner

excid3 commented Sep 15, 2021

Post the error and I'll take a look.

@jacobdaddario
Copy link
Contributor Author

jacobdaddario commented Sep 16, 2021

Hey! Back from work and had some dinner. Here's the image of the error:
Screen Shot 2021-09-15 at 7 14 45 PM
I'll keep poking around on my own to see if I can figure it out.

Edit:
I figured it out. The string "PostResource" fails to respond to constantize. The error doesn't really make sense though. Regardless I've tracked it down to Madmin.resource_names. Weirdly this is only happening on my branch. I just checked and master works fine... I wonder what's causing it.

@jacobdaddario
Copy link
Contributor Author

Alright, a couple things:

  1. The bug with constantize was because PostResource couldn't be instantiated. There were some attributes on it such as tag and enum which must've existed but ultimately had their migrations deleted. You can actually see this in the diff of my second commit when I triggered the schema being regenerated.
  2. I then tried to regenerate the PostResource file, but the generator is actually failing. Looks like with Rails > 6.1, Madmin expects that the routes file has a madmin.rb routes file split out from the main file. This is breaking since the dummy app only has one routes.rb file.

I've corrected both of these bugs and regenerated the posts resource. Now after regenerating the users resource to test the accessors, a bug with representing the store has popped up. For now I've pushed my changes. I'll work on rooting the bug out another time. Shooting for this weekend at the latest.

@jacobdaddario
Copy link
Contributor Author

I worked on it a little more. Got everything working and then the test suite broke... Will investigate more when I have time. I'll also end up squashing all of these commits together.

@jacobdaddario
Copy link
Contributor Author

jacobdaddario commented Sep 18, 2021

Edit: Shoot, the CI failed because the earlier versions of rails don't support the divided routes file... This is gonna be interesting to work out. Do you have any ideas on how to handle this? I'm not used to supporting multiple versions of Rails.

Hey! I think this is meragble. When I regenerated the post and user resources, I accidentally overrode some custom behavior which has been re-implemented now. That said, there were some attributes on the post resource that don't seem to exist in the database anymore, so I removed them and edited a failing test that was dependent on them.

I added a little bit to the resource class to get things functioning. I don't think we need tests for them since I don't see similar tests for changes like that in the code base, but if you feel we do I can go ahead and add some.

Also, I can squash the commits down if you'd like. There was definitely some troubleshooting involved to get familiar with the test suite.

Otherwise, I manually inspected the dummy app and the Store columns seem to be showing up correctly!

@excid3
Copy link
Owner

excid3 commented Sep 20, 2021

This looks really good!

I can squash when we merge this, so no worries there.

I actually wrote a little helper method to check for the separated routes file for madmin:

def separated_routes_file?

Good call on testing against JSON and text columns. 👍

We use that in the generators so they can check for the separate file and generate routes there. The dummy app probably needs to stick to the regular routes file for backwards compatibility and easier testing.

Are the routes the last piece here?

@jacobdaddario
Copy link
Contributor Author

Looks like something else got caught by the CI too… Let me take a closer look and I’ll just ping you when I’m done so I don’t keep setting off GitHub notifications for you.

@jacobdaddario
Copy link
Contributor Author

I've addressed the things the CI kept complaining about. For some reason when I run database migrations, it keeps wanting to change bigint columns to integers which threw the CI for a loop. I manually overwrote the ones that were causing CI failures, but I don't know if running another database migration might have caused more to flip. Regardless, I think I've addressed the CI problems.

@excid3
Copy link
Owner

excid3 commented Sep 22, 2021

I'm not sure why it writes them as integer. I have that same issue with Pay. Rails switched the default at one point and I thought that integer was just an alias to bigint. The code looks like it too. I couldn't quite figure out if it was the Rails version or the migration versions causing it and never did find a solution.

@jacobdaddario
Copy link
Contributor Author

Sweet, well I think this is done then so I won't add any more commits. Let me know if something needs changed still.

@excid3 excid3 merged commit fc73721 into excid3:master Sep 23, 2021
@jacobdaddario jacobdaddario deleted the store-accessor-fields branch September 23, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants