Hello Harshal,
We review the patch and have some questions:
1) Is there any particular reason to initialize variables and functions in the same place? We believe that it would be more readable there were no chaining of variable creation, specially if those variables are functions. Check line:
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@ max_value = field.max, isValid = true, intPattern = new RegExp("^-?[0-9]*$"),
- isMatched = intPattern.test(value);
+ isMatched = intPattern.test(value),
+ trigger_invalid_event = function(msg) {
2) The functions added in both places look very similar, can they be merged and extracted? We are talking about the trigger_invalid_event function.
3) The following change is very similar to the trigger_invalid_event, was there a reason not to use it?
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1573,25 +1584,23 @@ this.model.errorModel.unset(name); this.model.set(name, value); this.listenTo(this.model, "change:" + name, this.render);
- if (this.model.collection || this.model.handler) {
- (this.model.collection || this.model.handler).trigger(
- 'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
- );
+ // Check if other fields of same model are valid before
+ // triggering 'session:valid' event
+ if(_.size(this.model.errorModel.attributes) == 0) {
+ if (this.model.collection || this.model.handler) {
+ (this.model.collection || this.model.handler).trigger(
+ 'pgadmin-session:model:valid', this.model, (this.model.collection || this.model.handler)
+ );
+ } else {
+ (this.model).trigger(
+ 'pgadmin-session:valid', this.model.sessChanged(), this.model
+ );
+ }
4) We also noticed that the following change sets look very similiar. Is there any reason to have this code duplicated? If not this could be a good time to refactor it.
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -1528,7 +1528,18 @@
@@ -1573,25 +1584,23 @@
@@ -1631,7 +1640,18 @@
@@ -1676,25 +1696,23 @@
Thanks
Joao & Shruti
