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

Add request_number rule #460

Closed

Conversation

davidvieiratrustly
Copy link

Parent issue
#459

Closes #459

Technical implementation details

Implement a new 'request_number' as described on the issue.

@davidvieiratrustly
Copy link
Author

I would love some help on this prettifier thing. I'm new to javascript/typescript development, I'm a Java developer (please don't throw rocks at me :P)

@255kb
Copy link
Member

255kb commented Apr 29, 2021

Thanks for the contribution @davidvieiratrustly
For Prettier, the easiest is to install the extension (I guess you are using Webstorm or Intellij) https://prettier.io/docs/en/webstorm.html
You could also run the CLI with the following command npx prettier --write

@davidvieiratrustly
Copy link
Author

davidvieiratrustly commented Apr 29, 2021

Thanks for the help, did the prettifier commit now. The builds are failing because I had to change 3 projects for this issue and they are connected.

@255kb
Copy link
Member

255kb commented Apr 29, 2021

Yes this is expected. We don't have integration tests running of the 3 projects when one is modified.

@davidvieiratrustly
Copy link
Author

Feel free to contact me again if you need anything else regarding this issue.

Have a good day, and thank you for creating such a useful program.

@255kb
Copy link
Member

255kb commented Apr 29, 2021

Thank you :)
You will see I made a bunch of comments on your PR mockoon/commons-server#5
Also, could you please prettify the code for the two other PR also (after the requested changes)?

@davidvieiratrustly
Copy link
Author

Yeap, no problem.

@255kb
Copy link
Member

255kb commented Apr 29, 2021

After testing a bit, I think it's working well.
However, if you create a first route with no rules (let's say a 200), and a second one with request_number=1, you can never get the first route.
I wonder if this is a logical behavior. Until now, the rules system was serving the first response by default if no rule matched. I have the feeling that changing this default behavior could make things a little bit confuse. What do you think?

Also, I don't really like removing inputs from the UI. Then things are not aligned and it looks a little bit weird in my opinion. I would rather see something like a disabled input:
image
But CSS is not ready for this. I think when everything is done and tested, I will work on the UI part if it's ok for you.

@davidvieiratrustly
Copy link
Author

I would love some help on the UI part, I'm already way out of the comfort zone :P UI-wise, I think we should do an increment integer input for this rule.

About the default response, for me, it's the logical thing, but I'm open to suggestions. Maybe we could change the label to Multiple of request number, instead of Request number (starting in 1).

@255kb
Copy link
Member

255kb commented Apr 29, 2021

Sure, I will do the UI part. Also, you are right, value could be an input of type number and we could use the directive appInputNumber with a minimum of 1 and max of Infinity. It would require some changes to the types I guess.
Still, I am not completely convinced about the looping feature. It's the kind of hidden feature I would like to avoid. In the previous setup I tried, not getting the first response at all felt like a bug. Maybe you were right and having a button like the responses randomization would have been better 🤔
Maybe we could continue the discussion on the Discord server? https://discord.com/invite/JmTBk59MTb

@davidvieiratrustly
Copy link
Author

Yeap, no problem. I have to travel now, but I agree with you. I found it quite confusing implementing this on the rules.

@255kb
Copy link
Member

255kb commented Apr 29, 2021

@davidvieiratrustly yes, the more I thought about it the weirder it became. But now I think I got the solution. We should implement both features. One with a button that trigger sequential responses like you wanted, ignoring the rules. And one where you can say that Response X is served when we get call index Y. So, we get both systems, it's less confusing, and we don't lose your work :)
Let's talk about it when you come back!

@davidvieiratrustly
Copy link
Author

Yeah, I think it's the perfect solution like that. Can I implement it and you take care of the UI?

@255kb
Copy link
Member

255kb commented Apr 29, 2021

Sure! I will. Also adding a button will break the responses "header" a bit due to the length. So, I will need to rework it. Also do not worry if you don't know where or how to implement what I will list below. But like this we have an exhaustive list of what to do.

So, let's summarize:

Feature 1:

  • one button to trigger sequential responses next to the randomize button.
  • It must toggle off the randomize button I think and vice versa.
  • It ignores the rules (rules ignored warning must be displayed in the rules tab, like for the randomization feature).
  • it loops through the responses (with your modulo implementation).

Feature 2:

  • a new type of rule target to serve a response when it's call n° X.
  • Without looping system. Does a basic parseInt of the rule.value and supports regex so we can serve a specific response when it's call 5 to 6 for example.

We need to cover both features with tests in commons-server and on the main app with UI automated tests (I can do them).

@davidvieiratrustly
Copy link
Author

davidvieiratrustly commented Apr 30, 2021

@255kb I will close the pull request to let you work on the UI piece. I updated the common and common-server PRs and did the tests for both features.

Let me know if I can assist in anything. Have a nice day :D

@255kb
Copy link
Member

255kb commented May 3, 2021

Thank you @davidvieiratrustly I will take it from here and add the UI part and the migration. Thanks again for this great contribution!

@davidvieiratrustly
Copy link
Author

Thanks! Eager to start using this feature

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

Successfully merging this pull request may close these issues.

New type of rules that would allow serving different responses depending on the "index" of the call.
2 participants