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

MI-121: Updates the "generateMeshToken" function to return a "custome… #39

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

brettcutt-aligent
Copy link
Contributor

The Auth module had been updated to generate an auth JWT containing a generic "customer_id" property rather than one named "bc_customer_id". This had an undesired affect when the Auth module was used in the BigCommerce module because the function that decodes the auth JWT looks for a "bc_customer_id" property which no longer existed. The BigCommerce module is being updated to also have a generic naming convention so the "bc_customer_id" property is now named "customer_id".

…r_id" property instead of "bc_customer_id".

Updates the "getBcCustomerIdFromMeshToken" function to look for a "customer_id" property instead of "bc_customer_id".
@michael-west-aligent
Copy link
Contributor

looks good to me.~!

approved.

Copy link
Contributor

@michael-west-aligent michael-west-aligent left a comment

Choose a reason for hiding this comment

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

comment explains the change, linked ticket.
v good

@tvhees
Copy link
Contributor

tvhees commented Nov 25, 2024

I don't think this is a patch version change? Presumably it breaks any code relying on bc_customer_id being returned.

I would expect this to be v1.1.0, returning both customer_id and bc_customer_id (with a deprecation) for backwards compatibility. I'm not sure if we did this with the auth module either though.

Other than that - looks like a formatting issue is blocking the merge.

Update the getBcCustomerIdFromMeshToken function to look for a bc_customer_id but comment as deprected to make the fix backwards compatible.
@brettcutt-aligent
Copy link
Contributor Author

I don't think this is a patch version change? Presumably it breaks any code relying on bc_customer_id being returned.

I would expect this to be v1.1.0, returning both customer_id and bc_customer_id (with a deprecation) for backwards compatibility. I'm not sure if we did this with the auth module either though.

Other than that - looks like a formatting issue is blocking the merge.

@tvhees I've made updates so it backwards compatible and marked it as 1.1.0. I just branched this one off the main branch. Should I be instead branching off branch version/bigcommerce-1.1.0?

I had a look at the release process https://aligent.atlassian.net/wiki/spaces/MG/pages/3074719963/Graphql+Mesh+Module+Release+Process. I understand creating a feature/fix PR to a version branch but not so sure I understand creating fix/feature PR to the main branch as well. Wouldn't that mean the main branch has no version control?

@brettcutt-aligent
Copy link
Contributor Author

Looks like all open PR's are failing on the same code quality issue
2024-11-27_12-31

@TheOrangePuff
Copy link
Contributor

@brettcutt-aligent I might not be understanding your question correctly but you just need to create another PR that targets the version branch

@brettcutt-aligent
Copy link
Contributor Author

@brettcutt-aligent I might not be understanding your question correctly but you just need to create another PR that targets the version branch
2024-11-28_08-46
@TheOrangePuff

In there current situation a version/bigcommerce-1.1.0 branch exists but lets as say a hyperthetical version/bigcommerce-2.1.0 branch exists. Following the documentation. If I want to make a bug fix on 1.0.0 I shouldn't be branching off the main branch because that would be at version 2. If I'm building on top of a version, shouldn't I be branching off that same version branch and not main?
Also wouldn't that mean the latest version 1 fix should not be put back into main branch if it's on version 2?

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.

4 participants