-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Store metadata for solidus resources #5951
base: main
Are you sure you want to change the base?
Store metadata for solidus resources #5951
Conversation
Aims at solving #5951 |
Sure, basically I am hoping to have Solidus support something similar to the |
What resources do you think need it most and why? |
Product, variant, order, line item, and shipment would be reasonable defaults. They are the most important core models and my experience they are the records that need additional fields in the broadest number of cases. |
Ok, given your pref for products and transactions also returns and RMA requests make sense at this point. Can you live with them + users and I stop bugging you? |
Honestly, I don't think it's super important on which models we will add those metadata fields for the core. What's more important is creating a developer-friendly journey to add more of them when needed. What I envision for the success of this initiative is:
{
"key": "serial_number",
"type": "string",
"value": "123abc",
"access": "public", // Can also be the roles which have permissions, like "admin"?
"api": true // If we want this field to be included in the REST API payloads
}
With these three elements, we have enough flexibility and we don't have to assume anything on where the fields are needed, which is without doubt something that dramatically changes from store to store. |
@kennyadsl i really feel strongly about the admin part being a @MadelineCollier task. She's deep into that right now and it would take us much more time to do that. Can we slip that into her existing work? |
Yes, but it needs to be prioritized with the rest of the tasks in that case. By the way, the feature can live without the admin part, and we can add that later, isn't it? |
For me it's an API feature with read only admin |
Given how addresses are made implications should be documented. For how it works now, would two addresses with different metadata create two different addresses or one address? @jarednorman |
Addresses with the same fields will always be the same record, so you can't really attach metadata to them that's specific to a particular use of that address. |
How do you usually handle regional differences in address paterns (First Name Last Name for example is required for legal reasons in Italy if you want to create an invoice. |
@shahmayur001 please remove the following ressources.
|
Addresses have a unified name field and that's part of what makes up an address, so it's very unlikely that an address ever actually gets reused by different users (unless they are different accounts/orders created by the same person) as @kennyadsl alluded to in our call. |
If you need to split first and last name, you can use something like https://github.com/basecamp/name_of_person where needed. In my experience, this is enough for 99% of the occurrences, especially in Italy where there are few name exceptions. |
There is also the |
Hey, in that case i would stash it in a table and add an Interface, but to me it seems we find more and more overhead to integrate for something that was going to leave minimal integration work and shift again something over to implementation instead of having something ready to use. I am not sure that that is the right way to go. @kennyadsl can you make a more lightweight ready to use suggestion? |
if someone has a proposal that does not require a full rewrite and allows customer facing as well admin facing metadata, we are totally up for that. |
Guys, we really like solidus, we have contributed brands, several front-end fixes, just finished up subscriptions, have offered to contribute financially to fixing up license issues and want to keep going on reviews and sitemap to fix those up too with an eye on GA4 and structured data, but there must be a clear direction and not burning our hours as well as yours. I strongly believe rendering solidus more complete makes sense of it's own (GA4, Structured Data, Brands, Reviews,....) and we are perfectly happy to contribute to that, but there must be some flexibility on your side. For this feature to make sense for us we need to have the possibility to insert fully dynamic key value pairs, I don't feel that manual configuration (as suggested by @kennyadsl) is adding anything here. As @jarednorman said, if you three deem it not acceptable it's not gonna be part of the core. We are absolutely willing to kick in a resource to sponsor the development and continue fixing up extensions we need, but it must make sense for everybody. For us a system where we have to configure values instead of writing random values does not make sense, it's really a deal breaker for us (even if you deem that a useless feature). We are absolutely willing to comply with any strategy you three come up with to allow:
It will trickle down with support for other extensions we want to fix (such as Multi Store and subscriptions), but we have the need for a clear road. I ask you three to figure out what you want to do here, a hard no is absolutely fine for me. I think Solidus is a great product, but to receive more contributions back from existing customers it needs to guide more than it does now (on ecommerce basics like SEO) and go less defensive regarding what's expected from the customer. Right now no matter if I open the demo store or any extension it's broken left and right, we would like to help with that, but meta data is a core feature for us in the way we envisioned it (Public and Private with dynamic key value pairs). If you feel that's too much of a stretch that's fine, but I don't feel we have been lacking will to contribute. I am looking forward to your reply, whichever it may be. |
Hey @fthobe, I understand the frustration but as I mentioned already, this kind of structural changes might take time. We don't want to merge a solution in core just because it's ready, or another project implemented the feature in this way, or because one company needs the feature ASAP. For example, @mamhoff (sorry for the ping here), one of the most skilled and experienced engineer we have in our lucky community, took ~1 year to refine the new promotion system, and more or less the same amount of time in the past to improve the pricing system. He did that by submitting PRs, getting feedback and reiterating patiently. For both, we have stable, robust, and flexible implementations now, which don't require a lot of maintenance. I'm sorry if it might sound harsh, but we need to put the maintainability of the platform in front of everything, because we are a small team as you can see, and we have a job. No one in the core team is paid to work on Solidus. That's also why there might be a delay in answering discussions, and commenting over and over again has the opposite effect: giving us more things to read in the amount of time we can dedicate to Solidus. That said: again, I'm not saying we need to implement the solution as I proposed, but I'm waiting for some constructive comment around why what you copy/pasted from Spree is better abd still have to see some real reasoning that is not: "we need it this way" or "we don't want to re-do the work again". I'm sorry but this is not the way we merged things in core so far and (I think I can talk on behalf of the @solidusio/core-team here), we want to stick with this approach. As the core team, we are responsible for finding the thin line between what can be merged and what needs more work. Please, be patient. And if you need this immediately, just do it in your project, there's nothing bad with that. If this PR goes in core as is, only have to remove the custom code and it will keep working. If it will look like something else you can both keep your solution or migrate to the new one. Final thoughts: thanks for contributing, we appreciate the effort you are putting into upstreaming the work, which is not common nowadays. |
Very well said, Alberto. I think your proposal seems to be a good solution to the problem. I like the idea of using user roles as access control of attributes to return. The proposal in this PR is not I would support ending up in core. There is no need in having two sets of attributes, if we can use Solidus' access control (provided by CanCanCan) for returning just the attributes for the particular user requesting. Something that we already do for other attributes. |
(Edited out some typos) My problem is that I have no idea what you three are governance wise talking about: most extensions fail circle ci, most do not work, license issues are plenty which rendered any installation (also from a legal standpoint) of extensions hard to impossible on current code base. The only feedback I receive are either changing feature specifications or rebuffs. I find it personally very hard to believe that our will to fix a magnitude of extensions is outweighted by our only hard requirement to have public and private meta data in a setting that allows dynamic values, which are literally the only request we have to sustain feature wise a contribution. Right now, to be frank, I have the impression that we can contribute only features that align with the maintainer interests and not community interests which renders for me any discussion that I had with @tvdeyen and @kennyadsl moot at best, especially regarding competition to Shopify and a lack of return PRs from the community, what I saw till today is, with some very notable exceptions from @kennyadsl, exactly handbook behaviour on how to not do it. I am absolutely sure that all of you three have technical competencies that far outweighs ours on this topic right now, but I also believe that you three receive exactly the level of contribution right now that is appropriate for the approach of community management you currently drive. Which is fine, just write it on the website and the readme, because right now I have significant struggles to see any coordinated effort or decision processes that are open or public (I at least couldn’t find one on the new promotion system which to my understanding was a sponsored effort by candlescience). I renew this one last time: find a way to work with us on the meta data topic or continue a politic of closing down this application, the result in my opinion will just be very similar to what’s present right now, a community majorly driven by three companies used as a sandbox product. I find that very sad and in stark conflict to what I have heard from you about requiring a better community. We are not asking you to merge anything you feel uncomfortable with, we are asking you to help us align what we need with what you want. |
This is by the way an email I received: On Fri, 6 Dec 2024 at 20:29, wrote: I submitted those PRs on Solidus Digital Downloads. I kept them very specific and small and only addressed the “main” issues to get it running in v4.4. I am going to see if anything comes out of them, as I am not only evaluating the software, but the repo owners. Anyway, I just saw you comment on the metadata PR that you submitted and I thought it was spot on, so thank you for making it. I have worked on/with many open source projects and sometimes the maintainers are happy to get community involvement and sometimes not. It sounds like you guys have been using Solidus enough that you are pretty vested…. Do you mind sharing some insight on your experience and really your candid thoughts? I can say that with my limited experience, I have noticed some things that give me pause:
I have also found some things that are a huge positive for me:
Also, if you don’t mind I’d like to ask a couple of specific questions: Sorry for the long winded email, but I would love to get some of your thoughts as you have spent much more time with the platform. Thanks, |
@fthobe I don't think the core team doesn't want to have a discussion on this feature. Quite the opposite is happening, actually. No one in the core team needs this feature and still, we are here discussing and trying to help shape a better form of it, which is in contrast with your "I have the impression that we can contribute only features that align with the maintainer interests and not community interests", isn't it?. The only kind of discussion happening is: merge it as is or we'll stop contributing, without even trying to argument why the proposed version is a good/better implementation. I don't understand how we went into this "blackmail" mode, honestly but this is not how I want to interact with the community. I am part of the core team member because I enjoy working on open-source projects and interacting with people, and this is not enjoyable. I'll get back to this issue/PR when the mood of the conversation changes. Regarding the status of the ecosystem, we have a lot that can be improved, true. I don't think we will improve that by merging anything that is proposed, blindly. |
@kennyadsl again, all of you three can feel free to make a suggestion
We will as we have in the past positively evaluate it. If all we get is continuously changing specs with continuously changing instructions we will not byte. Transport spree meta was always seen by all sides as flawed, there was no discussion there. We are absolutely willing to work with you on that if you give clear instructions that maintain the desired outcome, right now I find the process in which you accept community contributions very flawed. If you or any of the other two want to give that a try and come up clear instructions or an alternative that creates the same outcome we will be all yours. If the process continues to be the same of gradually burn resources and change road mid project we won't. |
I told you about my doubts on copy/pasting what Spree did since the first discussion we had, isn't it? Still, you moved on with doing that without trying to design any other possible solution. You really can't blame us for burning resources. Please @fthobe, be clear: is copy/pasting this feature from Spree the only possible outcome we can have here? That's understandable but if that's the case, I think it's fair to stop discussing any other unrelated governance issue and go straight to the point: there's a clear lack of will to think and design an alternative solution unless this work comes from someone else. If that's not the case, I'm totally open to continuing the discussion and please tell me why we should go with this design, which again, I'm not totally against. I just would like to constructively understand if we have a better path, together with who has this need and probably has the best product vision of the feature. |
Edit 1: Public Catalogue Metadata is rendered optional Disclaimer@kennyadsl This is the best I can do.
What I think it does very well is giving a universal backend for information and that's the outcome I try to focus on. I do believe TBH that three kind of information are needed (see below):
I have broken down the entire scope below. If you people can decide upon how to do it I would even go as far as to trash the existing solution and start from scratch but the outcome needs to be clear. I strongly believe you people do know best how this can be achieved and we are absolutely willing to sponsor the whole development back to back including documentation, but I also strongly believe that the outcome should allow to stash pretty much anything that fits into a json array and leave application to the operator of solidus, but most of all be clear from the beginning. I added a comment to the end in merits of all the policy discussions. What I wrote down here contains:
If the specs below are an outcome that you feel you can support we are all for it to getting back to the drawing board if we get a reasonable amount of information during the process to allow non redundant work for us and a reasonable solution for you. Types of Meta Data and why we need itOverviewI have broken meta data down in three large topics visible in the table below.
Administrative MetadataWe do not believe that solidus is used anywhere as standalone system (edge cases excluded of course). Transactional resources are either by nature integrated with 3rd party systems (eg Payments, Billing in Italy with eInvoicing) or by operational requirement (ERP, CRMs like Salesforce,...). For that purpose I see meta data as a senseful extension on admin site for all transactional ressources (also payments, stock movements (even if we won't use them) and RMAs. Given that more and more countries switch to eInvoicing (Germany is next and US has had testing phases for B2B in 2022) in the coming years I personally (and I might well be alone with that opinion) deem meta data on orders a hard requirement to future proof the system. Customer Meta DataCustomer meta data allows users to customize a cart (incomplete order) with either administrative (PO numbers,...) or customisation information (eg an URL to a picture to customize a shirt, text strings for incisions or in our case IMEIs to relate to a subscription) allowing to significantly extend the features without changing the underlying model. Fields should be accessible on front-end and backend. (Public) Catalogue Meta DataData that allows to extend the product catalogue to comply with legal requirements or marketing requirements well aware that surely different use cases will be hacked into it by others in the future. Given that the name Public Meta Data has been sufficiently burned I called it Catalogue Meta Data. SchemaMeta Data should net a reliable outcome: ImplementationMake a decent suggestion and we will deliver it, I think it's important to provide a logic to enable and disable this feature. There can be different approaches:
Scopes & ArraysNature of the ValuesThe system should be agnostic to the value contained. A decision has been made to store the data in additional columns in a jsonb which by now is relatively widely supported. Some preexisting installations might have to upgrade DBs (I am looking at SQLite). "key1" : "value1" should lead to the same behaviour as "key2" : "value2" without requiring modification inside the code. Reasonable provisions should be taken inside the code base to edit the quantities and content size of key value pairs and appropriate errors should be returned where rate or size limits are exceeded. Administrative Meta DataAllow appropriate roles to customize resources with private fields not end-user accessible.
Customer Meta DataAllowing users to customize an incomplete order (or I'd say a cart) and call the information after a completed order. Admins should in an ideal case be allowed to edit those information after purchase, but I feel that can wait if technical limitations are currently in place.
Public Catalogue Meta DataFields that can be manipulated by administrative roles and are read only. Solidus is extremely weak chested on SEO and deserves more ways to stash data.
Personal CommentI personally do not believe that Shopify discarded meta fields for any other reason than the platform being used as a "store it all" backend for applications that were not financially viable for shopify given their pricing structure. They were by the way substituted by application meta fields which are alive and well in the current api for usage on front-end and backend, but that barriers have been in my again personal opinion be included to make this a more premium tier oriented feature for sales channels such as their partners. I think all of you three are very smart cookies (and I really believe that) and from what I heard mamhoff is smarter than all of us combined, but I really believe you people need a strong rain check. You need to make up your mind if this repo is a sandbox for your own products or an open platform (which right now it's not) and advertise it accordingly. Right now I do not personally see how the current approach is going to net you what you want (from a business perspective as well as community perspective) and I personally prefer drawing lines and provide clarity straight from the beginning and break bread over this topic or don't break bread at all. There's absolutely no obligation for you to follow us on this, but there's also absolutely no obligation for us to give back where it doesn't make sense for all parties. Again, all under the premise that if the outcome is reasonable we are happy to contribute and bear the costs for the contribution, clearer than that I can't phrase it. We have without hesitation contributed to several extensions and legal topics already and are happy to continue if you open up the decision making process beyond a simple I do not like it, otherwise it's simply spree maintained by three parties instead of one for us. Example 1: Create order
Example Reponse 1: Create order
|
Thanks Fabian, this is a constructive recap (except for the last point, which IMHO doesn't make sense because we are here discussing, invalidating your point). Some notes:
This is smart because as soon as we try, it's tough to imagine putting those fields in fixed buckets. You already spotted 3 categories while describing the feature which could (not sure) have inconsistencies, for example, I don't understand the access level of the Customer Meta Data. Is it public or private?
|
Hey, thank you for your reply. Some clarifications, where I use the words "In my opinion" it is solely to underline that personally have a POV that can also be wrong and corrected.
Some additional notes regarding maintenance of key value pairs:
|
Hey @kennyadsl, I studied your suggestion. I find the outcome again shifts over the burden to maintain custom fields in the code not providing any significant difference compared to a solution where a store operator customizes the fields by adding custom code. I am willing to reason with any suggestion that creates an outcome where fields can be random (meaning the key and value are not defined in code) and editable in two sets:
and in addition, yes, in a perfect world also one set for catalogue data publicly readable to the public editable only by admins. Before I dedicate myself to a more dettaigled response to your suggestion and I can't stress this enough:
is acceptable for us. You solution does not create an outcome significantly different to manually hack in a field into the code. It renders the solution admittedly less static than before, but it does not significantly change the dynamic. I am really interested to find a solution here, but TBH all you offer is the same amount of outcome variability that already pains this platform wrapped up in a different flavour of storing data without targeting the real problem: if any shop operator needs to create every time a custom field or implementation updating is a pain as we saw with taxons and brands. I do not want to have this level of maintenance and to be honest looking at the ecosystem it's exactly this we are currently struggling with across every extension and feature we touch. This doesn't make your solution technically inviable in general, it makes it just not adapt to problem we are trying to solve. I personally, with my very limited knowledge, do not believe that the burden of maintaining two additional columns per transaction resource does create any significant additional burden compared to variety of different implementations of solidus that are currently making up the installation base. I personally believe that a feature like metadata for customers and admins could render future installations of solidus easy to maintain and update as they they largely remove the need for custom fields and customisations, which is the goal we are personally aiming for (and that in all transparency aligns with our overall business goals). I extend my (I really mean it) warmest invitation to you to help us get out goal achieved in a collaborative way or simply help us reach the opinion that a for us desired outcome is not possible. It's not blackmailing you to say that we suspend contributions, it's a business decision as you probably make others with your organisation as well. Given that the entire discussion across issue and PR is going on for almost a month now I am looking for a solution within a reasonable amount of time. |
Summary
This implementation extends the Solidus framework to add metadata support across multiple Spree resources. It involves database migrations, model enhancements, attribute updates, API configuration, and comprehensive testing to ensure robust functionality.
List of Resources
The following Spree resources are configured to support metadata fields:
Database Schema Updates
To support metadata, two new JSON/JSONB columns (public_metadata and private_metadata) are added to each of the above resources.
Migration
PR Model Configuration
A shared concern Spree::Metadata is introduced to handle serialization and facilitate operations on the metadata fields.
Concern Implementation
Attribute Configuration
The public_metadata and private_metadata fields are incorporated into the Spree attribute system.
Permitted Attributes
Strong Parameters
API Configuration
Testing
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: