Update app and issues workflow #4
Conversation
index.js
Outdated
| }, | ||
| milestones: { | ||
| next_patch_milestone: '1.7.4.3', | ||
| next_minor_milestone: '1.7.5.0', | ||
| next_minor_milestone: '1.7.5.0' |
There was a problem hiding this comment.
These are very outdated but can be fixed later
|
@sowbiba Looks good to me. Let's see if @eternoendless has something to add, else it's OK @PierreRambaud and @NeOMakinG can also help to review Javascript |
index.js
Outdated
| if (rule != null) { | ||
| context.log.info('[Index] Received webhook matches rule ' + rule + ' requirements'); | ||
| ruleApplier.applyRule(rule, context); | ||
| if (rules != null) { |
There was a problem hiding this comment.
| if (rules != null) { | |
| if (rules !== null) { |
There was a problem hiding this comment.
I don't see the change in file 🤔
src/rule-applier.js
Outdated
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks so duplicating, wdyt @PierreRambaud ?
There was a problem hiding this comment.
Then would it be possible to try to do something to avoid duplicate code ? Sometimes it's too overkill but there it looks like not
index.js
Outdated
| context.log.info('[Index] Received webhook matches rule ' + rule + ' requirements'); | ||
| ruleApplier.applyRule(rule, context); | ||
| if (rules != null) { | ||
| for (const rule of rules) { |
There was a problem hiding this comment.
The for statement is also done in applyRules, is it needed to do both?
src/rule-applier.js
Outdated
|
|
||
| switch (rule) { | ||
| case Rule.A1: | ||
| this.applyRuleA1(context); |
There was a problem hiding this comment.
Should be better to find a way to remove this useless switch, otherwise in few weeks, we'll have a 500 lines only for the switch.
There was a problem hiding this comment.
I tried to do it better
| @@ -0,0 +1,3 @@ | |||
| /node_modules/ | |||
| /tests/ | |||
| !.eslintrc.js | |||
There was a problem hiding this comment.
Please configure your IDE so it auto-add new line at the end of files!
| browser: true, | ||
| node: true, | ||
| es6: true, | ||
| jquery: true, |
There was a problem hiding this comment.
You right. It seems not needed. I will check what every parameter is for in env and globals
There was a problem hiding this comment.
Env is for context, globals is for globals variables.
| google: true, | ||
| document: true, | ||
| navigator: false, | ||
| window: true, |
There was a problem hiding this comment.
No need too, it's node side, not client side right ?
| name: 'global', | ||
| message: 'Use window variable instead.', |
There was a problem hiding this comment.
It's node side, you'll never use window
Co-Authored-By: SZCZUPAK Valentin <valentin.szczupak@prestashop.com>
sowbiba
force-pushed
the
add-missing-rules
branch
from
6243d96
to
0b830cf
Compare
No description provided.