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

Fix save carrier with invalid ranges #8097

Merged
merged 1 commit into from Oct 23, 2017

Conversation

Projects
None yet
10 participants
@Azouz-Jribi
Contributor

Azouz-Jribi commented Jul 6, 2017

Questions Answers
Branch? develop
Description? When you create or edit a carrier with invalid ranges, you should not be able to save it.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-3155
How to test? Go to BO > shipping > carrier > Edit , add a new range and fill its input with incorrect values. Then, try to click another input field, the incorrect fields will be red, error will be displayed and the submit button will be disabled.

This change is Reviewable

@prestonBot

This comment has been minimized.

Show comment
Hide comment
@prestonBot

prestonBot Jul 6, 2017

Collaborator

Hello Azouz-Jribi!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

Collaborator

prestonBot commented Jul 6, 2017

Hello Azouz-Jribi!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@vincentbz

This comment has been minimized.

Show comment
Hide comment
@vincentbz

vincentbz Jul 11, 2017

Contributor

Hi @Azouz-Jribi

Could you move it to 1.7.2.x branch please ?
So that it can be taken in 1.7.2.1 version.

Thanks !

Contributor

vincentbz commented Jul 11, 2017

Hi @Azouz-Jribi

Could you move it to 1.7.2.x branch please ?
So that it can be taken in 1.7.2.1 version.

Thanks !

@vincentbz vincentbz added this to the 1.7.2.1 milestone Jul 11, 2017

@Azouz-Jribi Azouz-Jribi changed the base branch from develop to 1.7.2.x Jul 11, 2017

Show outdated Hide outdated js/admin/carrier_wizard.js Outdated
@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Jul 13, 2017

Contributor

Hello @Azouz-Jribi
There is a problem when I try to change a range or when I click several times in the different fields.
The page freezes.

pr8097

Contributor

marionf commented Jul 13, 2017

Hello @Azouz-Jribi
There is a problem when I try to change a range or when I click several times in the different fields.
The page freezes.

pr8097

@vincentbz

This comment has been minimized.

Show comment
Hide comment
@vincentbz

vincentbz Jul 21, 2017

Contributor

Hi @Azouz-Jribi

could you check this with @marionf please ?

Thanks !

Contributor

vincentbz commented Jul 21, 2017

Hi @Azouz-Jribi

could you check this with @marionf please ?

Thanks !

@Azouz-Jribi

This comment has been minimized.

Show comment
Hide comment
@Azouz-Jribi

Azouz-Jribi Jul 21, 2017

Contributor

Hi @marionf,
This bug was already exist from the beginning, but with the new modification will be more reproducible.

bug js

After some change the page crashes, see the GIF.

Contributor

Azouz-Jribi commented Jul 21, 2017

Hi @marionf,
This bug was already exist from the beginning, but with the new modification will be more reproducible.

bug js

After some change the page crashes, see the GIF.

Show outdated Hide outdated js/admin/carrier_wizard.js Outdated
Show outdated Hide outdated js/admin/carrier_wizard.js Outdated
Show outdated Hide outdated js/admin/carrier_wizard.js Outdated
@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Jul 25, 2017

Contributor

Hello @Azouz-Jribi
I can't validate your PR, the page freezes to easily.
Also, some fields are disabled and I can't enter the price for the range.
I attached a new screenrecord on the forge ticket to show you.

Contributor

marionf commented Jul 25, 2017

Hello @Azouz-Jribi
I can't validate your PR, the page freezes to easily.
Also, some fields are disabled and I can't enter the price for the range.
I attached a new screenrecord on the forge ticket to show you.

@Azouz-Jribi

This comment has been minimized.

Show comment
Hide comment
@Azouz-Jribi

Azouz-Jribi Jul 27, 2017

Contributor

Hi @marionf
i will try to fix it, but it takes time.

Contributor

Azouz-Jribi commented Jul 27, 2017

Hi @marionf
i will try to fix it, but it takes time.

@vincentbz vincentbz added WIP and removed waiting for author labels Jul 28, 2017

@vincentbz vincentbz modified the milestones: 1.7.2.1, 1.7.2.2 Jul 28, 2017

Putting this on hold since not approved by QA

@eternoendless eternoendless modified the milestones: 1.7.2.3, 1.7.2.2 Aug 23, 2017

@Azouz-Jribi

This comment has been minimized.

Show comment
Hide comment
@Azouz-Jribi

Azouz-Jribi Aug 24, 2017

Contributor

Hello @marionf
Correction is done.
Can you retest it please ?

Thanks ;)

Contributor

Azouz-Jribi commented Aug 24, 2017

Hello @marionf
Correction is done.
Can you retest it please ?

Thanks ;)

@eternoendless

@Azouz-Jribi QA needs to wait for code review before they can verify tickets.

Show outdated Hide outdated js/admin/carrier_wizard.js Outdated
Show outdated Hide outdated js/admin/carrier_wizard.js Outdated
@Azouz-Jribi

This comment has been minimized.

Show comment
Hide comment
@Azouz-Jribi

Azouz-Jribi Aug 25, 2017

Contributor

Hi @eternoendless
Correction is done, can you check it ?

Thanks

Contributor

Azouz-Jribi commented Aug 25, 2017

Hi @eternoendless
Correction is done, can you check it ?

Thanks

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Aug 28, 2017

Contributor

Hello @Azouz-Jribi
When I try to change a range, even if the range isn't overlapping anymore, the fields stay in red and there is an error message. I have to click several time in the fields to make it disappear.
I attached a new screenrecord on the forge ticket.

capture du 2017-08-28 10-42-24

Contributor

marionf commented Aug 28, 2017

Hello @Azouz-Jribi
When I try to change a range, even if the range isn't overlapping anymore, the fields stay in red and there is an error message. I have to click several time in the fields to make it disappear.
I attached a new screenrecord on the forge ticket.

capture du 2017-08-28 10-42-24

@Azouz-Jribi

This comment has been minimized.

Show comment
Hide comment
@Azouz-Jribi

Azouz-Jribi Sep 18, 2017

Contributor

Hello @marionf
Correction is done.

@eternoendless can you check it ?

Contributor

Azouz-Jribi commented Sep 18, 2017

Hello @marionf
Correction is done.

@eternoendless can you check it ?

@eternoendless eternoendless changed the base branch from 1.7.2.x to develop Oct 12, 2017

@Azouz-Jribi

This comment has been minimized.

Show comment
Hide comment
@Azouz-Jribi

Azouz-Jribi Oct 13, 2017

Contributor

Hi @eternoendless
Rebase is done ;)

Contributor

Azouz-Jribi commented Oct 13, 2017

Hi @eternoendless
Rebase is done ;)

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 13, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 16, 2017

Member

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


js/admin/carrier_wizard.js, line 766 at r1 (raw file):

var carriersRangeInputs = {
	/** Check the carriers range inputs after each change */
	'checkCarriersRangeInput': function() {

It's not necessary to add single quotes to object property names.


js/admin/carrier_wizard.js, line 766 at r1 (raw file):

var carriersRangeInputs = {
	/** Check the carriers range inputs after each change */
	'checkCarriersRangeInput': function() {

I think the name of this method is misleading. The way it is now, it would make you believe it actually performs the validation, but it actually initializes the watch on inputs in order to validate them on change.

Method names should be an abstraction of what the method actually does.


js/admin/carrier_wizard.js, line 790 at r1 (raw file):

			$(this).off();
			if ($('.wizard_error').length === 0) {
				index = $(this).closest('td').index();

Missing var before index here


Comments from Reviewable

Member

eternoendless commented Oct 16, 2017

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


js/admin/carrier_wizard.js, line 766 at r1 (raw file):

var carriersRangeInputs = {
	/** Check the carriers range inputs after each change */
	'checkCarriersRangeInput': function() {

It's not necessary to add single quotes to object property names.


js/admin/carrier_wizard.js, line 766 at r1 (raw file):

var carriersRangeInputs = {
	/** Check the carriers range inputs after each change */
	'checkCarriersRangeInput': function() {

I think the name of this method is misleading. The way it is now, it would make you believe it actually performs the validation, but it actually initializes the watch on inputs in order to validate them on change.

Method names should be an abstraction of what the method actually does.


js/admin/carrier_wizard.js, line 790 at r1 (raw file):

			$(this).off();
			if ($('.wizard_error').length === 0) {
				index = $(this).closest('td').index();

Missing var before index here


Comments from Reviewable

@Azouz-Jribi

This comment has been minimized.

Show comment
Hide comment
@Azouz-Jribi

Azouz-Jribi Oct 17, 2017

Contributor

Hello @eternoendless
Can you check it ?

Thanks ;)

Contributor

Azouz-Jribi commented Oct 17, 2017

Hello @eternoendless
Can you check it ?

Thanks ;)

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 17, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 17, 2017

Member

Looks good to me

The code has grown too complex and needs refactoring though.


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Oct 17, 2017

Looks good to me

The code has grown too complex and needs refactoring though.


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@prasanthSelvar

This comment has been minimized.

Show comment
Hide comment
@prasanthSelvar

prasanthSelvar Oct 19, 2017

Hello @Azouz-Jribi,
I can still save carrier when I have the warning message "the range is not valid".
Especially in the case where the first value is greater than the second value.
See range.gif on the JIRA ticket BOOM-3155.

prasanthSelvar commented Oct 19, 2017

Hello @Azouz-Jribi,
I can still save carrier when I have the warning message "the range is not valid".
Especially in the case where the first value is greater than the second value.
See range.gif on the JIRA ticket BOOM-3155.

@Azouz-Jribi

This comment has been minimized.

Show comment
Hide comment
@Azouz-Jribi

Azouz-Jribi Oct 19, 2017

Contributor

Hello @prasanthSelvar
Correction is done ;)

Contributor

Azouz-Jribi commented Oct 19, 2017

Hello @prasanthSelvar
Correction is done ;)

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Oct 19, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- js/admin/carrier_wizard.js  8
         

See the complete overview on Codacy

codacy-bot commented Oct 19, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- js/admin/carrier_wizard.js  8
         

See the complete overview on Codacy

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 19, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 20, 2017

Member

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Oct 20, 2017

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Oct 23, 2017

Thank you @Azouz-Jribi

@eternoendless eternoendless merged commit 0419b44 into PrestaShop:develop Oct 23, 2017

3 checks passed

codacy/pr Good work! A positive pull request.
Details
code-review/reviewable 1 file reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment