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

Combination bulk actions progress modal #28751

Merged
merged 25 commits into from Jun 24, 2022

Conversation

zuk3975
Copy link
Contributor

@zuk3975 zuk3975 commented Jun 14, 2022

Questions Answers
Branch? 8.0.x
Description? Add progress modal (which was introduced in #26004) to Combination bulk actions in new product page. Also make bulk endpoint accept array of combinationIds and handle form violation errors visualisation. Screenshot from 2022-06-19 17-22-29Screenshot from 2022-06-19 17-22-24Screenshot from 2022-06-19 17-19-09Screenshot from 2022-06-19 17-18-57
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket?
Related PRs If theme, autoupgrade or other module change is needed, provide a link to related PRs here.
How to test? Try bulk actions in new product combination page. It should show progress modal same as in product page.
Possible impacts?

@zuk3975 zuk3975 requested a review from a team as a code owner June 14, 2022 15:09
@prestonBot
Copy link
Collaborator

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

  • Your pull request does not seem to fix any issue, consider creating one (see note below) and linking it by writing Fixes #1234.

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

About linked issues

Please consider opening an issue before submitting a Pull Request:

  • If it's a bug fix, it helps maintainers verify that the bug is effectively due to a defect in the code, and that it hasn't been fixed already.
  • It can help trigger a discussion about the best implementation path before a single line of code is written.
  • It may lead the Core Product team to mark that issue as a priority, further attracting the maintainers' attention.

(Note: this is an automated message, but answering it will reach a real human)

@prestonBot prestonBot added the Refactoring Type: Refactoring label Jun 14, 2022
@zuk3975 zuk3975 changed the base branch from develop to 8.0.x June 14, 2022 15:09
@prestonBot
Copy link
Collaborator

prestonBot commented Jun 14, 2022

This pull request seems to contain new translation strings. I have summarized them below to ease up review:

(Note: this is an automated message, but answering it will reach a real human)

@prestonBot prestonBot added the Waiting for wording Status: action required, waiting for wording label Jun 14, 2022
@zuk3975 zuk3975 force-pushed the combination-bulk-progress-modal branch 5 times, most recently from d744998 to e1757f0 Compare June 19, 2022 11:23
@zuk3975 zuk3975 force-pushed the combination-bulk-progress-modal branch from 7d0f552 to f08156c Compare June 19, 2022 14:26
@zuk3975
Copy link
Contributor Author

zuk3975 commented Jun 19, 2022

Some concerns:

  1. There was one very strange behavior, but it disappeared and I couldn't reproduced it anymore - sometimes when form violations occurred, the js components failed to initialize (resulting in no dynamics in the form at all - disabling switches wouldn't work as well as DeltaQuantityInput)
  2. After form violation errors if you edited one field and it failed, once you switch that field off, the submit button is enabled even though it doesn't make much sense from user perspective (because then you can submit empty form, because all inputs are disabled). Couldn't find an easy way to fix it, so its still there.

@zuk3975 zuk3975 changed the title [WIP] Combination bulk actions progress modal Combination bulk actions progress modal Jun 20, 2022
Copy link

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

Some small comments, but overall lgtm

Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @zuk3975

great work this one turned out to be more complex than I anticipated. I have a few minor suggestions that are not that important. But the main comment is about how we could handle the form validation a bit more cleanly see the big comment on bulk-edition-handler.ts in particular

And I also suggested a possible optimization of the bulk controller

Comment on lines 54 to 55
* This property contains a form content fetched from api when form constraints are violated during form submit.
* This form will already have rendered errors where they belong (depending on violated fields).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* This property contains a form content fetched from api when form constraints are violated during form submit.
* This form will already have rendered errors where they belong (depending on violated fields).
* This property contains a form content fetched from api when form constraints are invalid during form submit.
* This form will already have rendered errors where they belong (depending on invalid fields).

$combinationIds = json_decode($combinationIds);
$errors = [];
foreach ($combinationIds as $combinationId) {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this part could be optimized, for now the form is rebuilt for each provided combination but it's always the same data so no need to build the form in the loop it could be built only once outside of the loop

@jolelievre jolelievre force-pushed the combination-bulk-progress-modal branch from 6808c6a to bc18317 Compare June 22, 2022 16:36
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @zuk3975

@jolelievre jolelievre merged commit a3c2209 into PrestaShop:8.0.x Jun 24, 2022
@Progi1984 Progi1984 added this to the 8.0.0 milestone Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Type: Refactoring Waiting for wording Status: action required, waiting for wording
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants