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

[10.0][ADD] Return also state label (not only technical value) #404

Merged

Conversation

acsonefho
Copy link
Contributor

@acsonefho acsonefho commented Sep 12, 2019

Depends on MR #407
(Please review first depending MR to avoid comment code related to others MR)

Currently we return (into json result) the state (technical value) of the object (cart and invoice for this case). But we also have to return the translated label of this state (as the front doesn't have to manage translation on data).

@acsonefho acsonefho force-pushed the 10.0-selection_state_label branch 2 times, most recently from 744109b to 28f8ba3 Compare September 19, 2019 09:46
@codecov-io
Copy link

codecov-io commented Sep 19, 2019

Codecov Report

Merging #404 into 10.0 will increase coverage by <.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             10.0     #404      +/-   ##
==========================================
+ Coverage   89.49%   89.49%   +<.01%     
==========================================
  Files         141      141              
  Lines        3684     3685       +1     
==========================================
+ Hits         3297     3298       +1     
  Misses        387      387
Impacted Files Coverage Δ
shopinvader_invoice/services/invoice.py 100% <ø> (+3.03%) ⬆️
shopinvader/services/abstract_sale.py 97.61% <100%> (+0.05%) ⬆️
shopinvader/services/service.py 77.9% <83.33%> (+0.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43201c5...feddf0c. Read the comment docs.

@acsonefho acsonefho force-pushed the 10.0-selection_state_label branch from 28f8ba3 to 131a639 Compare September 20, 2019 06:05
@Cedric-Pigeon
Copy link

@acsonefho Could you resolve conflicts please?

@acsonefho acsonefho force-pushed the 10.0-selection_state_label branch from 131a639 to 4e2359e Compare September 27, 2019 07:34
@acsonefho
Copy link
Contributor Author

@Cedric-Pigeon It's done :) But this MR depends on #407 (also in conflict) but who is waiting a feed back from @sebastienbeau . So I let this MR in WIP.

@acsonefho acsonefho force-pushed the 10.0-selection_state_label branch from 4e2359e to 1a494fc Compare October 21, 2019 10:33
@acsonefho acsonefho force-pushed the 10.0-selection_state_label branch from 1a494fc to ac84bb1 Compare November 8, 2019 10:43
@acsonefho acsonefho changed the title WIP [10.0][ADD] Return also state label (not only technical value) [10.0][ADD] Return also state label (not only technical value) Nov 8, 2019
@rousseldenis
Copy link
Contributor

@acsonefho What's the status of this ?

@rousseldenis rousseldenis added this to the 10.0 milestone Nov 28, 2019
@acsonefho
Copy link
Contributor Author

@acsonefho What's the status of this ?

Wait for reviewers and then merge :)

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

Code review.

Ok, but I'm in favor of a more generic mechanism that allow to return a tuple or ... in the json for all fields translate = True

@acsonefho acsonefho force-pushed the 10.0-selection_state_label branch from ac84bb1 to 1e0b087 Compare March 18, 2020 12:36
@simahawk
Copy link
Contributor

Code review.

Ok, but I'm in favor of a more generic mechanism that allow to return a tuple or ... in the json for all fields translate = True

I agree. We could have something in base_jsonify to ask for value/label in the parser.
Eg: {"value": "foo", "label": "Foo"}.
If you like the idea it would make sense to change state + state_label w/ a dictionary already.

@Cedric-Pigeon Cedric-Pigeon force-pushed the 10.0-selection_state_label branch from 1e0b087 to fedb902 Compare March 18, 2020 14:06
@acsonefho
Copy link
Contributor Author

Code review.
Ok, but I'm in favor of a more generic mechanism that allow to return a tuple or ... in the json for all fields translate = True

I agree. We could have something in base_jsonify to ask for value/label in the parser.
Eg: {"value": "foo", "label": "Foo"}.
If you like the idea it would make sense to change state + state_label w/ a dictionary already.

It's a great idea!! But what about compatibility with front side who already use the state key?
Do we have to keep both for while?

@Cedric-Pigeon Cedric-Pigeon force-pushed the 10.0-selection_state_label branch 3 times, most recently from feddf0c to ac84bb1 Compare March 18, 2020 15:34
@simahawk
Copy link
Contributor

@acsonefho that's an option, yes. If you go with it add TODO to drop it.
In any case, if you want to use the label you will have to touch the frontend -> better you use the new format.

@shopinvader-git-bot
Copy link

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@Cedric-Pigeon
Copy link

/ocabot merge patch

@shopinvader-git-bot
Copy link

This PR looks fantastic, let's merge it!
Prepared branch 10.0-ocabot-merge-pr-404-by-Cedric-Pigeon-bump-patch, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Mar 19, 2020
Signed-off-by Cedric-Pigeon
@shopinvader-git-bot
Copy link

It looks like something changed on 10.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 10.0-ocabot-merge-pr-404-by-Cedric-Pigeon-bump-patch, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Mar 19, 2020
Signed-off-by Cedric-Pigeon
@shopinvader-git-bot
Copy link

It looks like something changed on 10.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 10.0-ocabot-merge-pr-404-by-Cedric-Pigeon-bump-patch, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Mar 19, 2020
Signed-off-by Cedric-Pigeon
@shopinvader-git-bot
Copy link

It looks like something changed on 10.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 10.0-ocabot-merge-pr-404-by-Cedric-Pigeon-bump-patch, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Mar 19, 2020
Signed-off-by Cedric-Pigeon
@shopinvader-git-bot
Copy link

It looks like something changed on 10.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 10.0-ocabot-merge-pr-404-by-Cedric-Pigeon-bump-patch, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Mar 19, 2020
Signed-off-by Cedric-Pigeon
@shopinvader-git-bot
Copy link

@Cedric-Pigeon your merge command was aborted due to failed check(s), which you can inspect on this commit of 10.0-ocabot-merge-pr-404-by-Cedric-Pigeon-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@acsonefho acsonefho force-pushed the 10.0-selection_state_label branch from ac84bb1 to a229e9e Compare March 20, 2020 07:08
@acsonefho acsonefho force-pushed the 10.0-selection_state_label branch from a229e9e to 5bca8da Compare March 20, 2020 07:09
@Cedric-Pigeon
Copy link

/ocabot merge patch

@shopinvader-git-bot
Copy link

This PR looks fantastic, let's merge it!
Prepared branch 10.0-ocabot-merge-pr-404-by-Cedric-Pigeon-bump-patch, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Mar 20, 2020
Signed-off-by Cedric-Pigeon
@shopinvader-git-bot
Copy link

It looks like something changed on 10.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 10.0-ocabot-merge-pr-404-by-Cedric-Pigeon-bump-patch, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Mar 20, 2020
Signed-off-by Cedric-Pigeon
@shopinvader-git-bot
Copy link

It looks like something changed on 10.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 10.0-ocabot-merge-pr-404-by-Cedric-Pigeon-bump-patch, awaiting test results.

@shopinvader-git-bot shopinvader-git-bot merged commit b78da42 into shopinvader:10.0 Mar 20, 2020
@shopinvader-git-bot
Copy link

Congratulations, your PR was merged at 9dba638. Thanks a lot for contributing to shopinvader. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants