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

Handle permalink attribute on product create #4024

Conversation

nandita2010
Copy link
Contributor

@nandita2010 nandita2010 commented Apr 12, 2021

Description
An ActiveModel::UnknownAttributeError error is seen when a product is being created with a permalink attribute.

The reason for this error is because the request contains permalink key that Rails ActiveRecord is looking for in Products table. This was the case in the past but in a commit, permalink attribute in products was renamed to slug and moved to taxons model. Ideally, permalink attribute should have been removed from permitted_attributes.rb under @@product_attributes array. Looks like that was missed.

This PR removes the attribute from product's permitted attributes and from the document

This PR takes care of issue #3988

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
    -[ ] I have updated Guides and README accordingly to this change (if needed)
    - [ ] I have added tests to cover this change (if needed)
    - [ ] I have attached screenshots to this PR for visual changes (if needed)

@kennyadsl
Copy link
Member

Thanks for your contribution, @nandita2010!

I'm taking a closer look and couldn't figure out where we are actually using the permalink field for Spree::Product in Solidus.

Can you help me understand if we really need it or it was just an API documentation issue?

@nandita2010
Copy link
Contributor Author

You're right @kennyadsl

Permalink attribute is not used anywhere in product.
Reading the issue my understanding was that the client is sending a permalink attribute that was causing the call to fail. Introducing a setter solved this without functionally changing anything.

But, yes. If we can update the document and republish it to the clients so they do not pass the permalink attribute will do the job and avoid this error. My bad, I didn't know this was an option.

Could we consider using permit? "params.require(: product).permit(:name, :description, :meta_description ...)" as an additional check on the controller. We can then use action_on_unpermitted_parameters to raise an error.

@kennyadsl
Copy link
Member

Thanks for getting back @nandita2010. I think we already use permitted attributes on the controller level, see here. Not sure why permalink is there though. Worth exploring the git history and understand if we can just remove that attribute from the list. Also, removing it and see if some specs fail will give us some useful information.

@nandita2010
Copy link
Contributor Author

Thank you @kennyadsl for the pointer. Looks like permalink was an attribute in products which was later renamed to slug in product and moved to taxons model. Ideally in commit d4d9550 where all the related changes have been made, permalink attribute should have been removed from permitted_attributes.rb under @@product_attributes array. Looks like that was missed.
I will undo the changes from previous commit and create a new commit with the attribute removed from here and ensure all rspecs pass with the new change.

@nandita2010 nandita2010 force-pushed the handle-permalink-attribute-in-product-create branch from ee0456d to 4ab4c66 Compare April 19, 2021 07:43
@nandita2010
Copy link
Contributor Author

PS - Force-pushed only because I had not set the right git config for user.name and email for that commit.

Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

This looks good to me. Could you rebase/squash this down so it's just one commit that makes the intended change. The initial change and revert can be dropped.

@nandita2010 nandita2010 force-pushed the handle-permalink-attribute-in-product-create branch from 25cb5e3 to 0d31f49 Compare April 20, 2021 03:04
@nandita2010
Copy link
Contributor Author

Hi @jarednorman, please have a look at the updated commits.

@nandita2010
Copy link
Contributor Author

nandita2010 commented Apr 27, 2021

Hi @kennyadsl Please have a look at the PR

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks @nandita2010!

@kennyadsl kennyadsl merged commit 25daae3 into solidusio:master May 5, 2021
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