Skip to content

Conversation

@eric-elem
Copy link
Collaborator

What does this PR do?

Ensures all fields from the requests form are sent to the backend

Tasks to be completed

  • Ensure fields from request form template aren't sent
  • Ensure fields that can have a single or many value(s) are always sent as arrays
  • Rename keys for array fields to be plural

How can this be tested

  • Add a print statement to print received payload in the dashboard POST endpoint in views.py. Run the server.
  • Visit the dashboard endpoint (e.g http://127.0.0.1:5000/dashboard), create some checks and click the save button.
  • Verify that the payload printed on the server console contains all the submitted fields

What is the relevant pivotal tracker story?

Story #155514650

@eric-elem eric-elem requested review from Ashaba and muhallan February 28, 2018 13:05

if(data.length > 0) {
$.ajax({
url: 'http://127.0.0.1:5000/dashboard',
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a way we can ensure the url is not hardcoded? For example pushing this code to another server would lead to failure.

Think of getting the host using JQuery

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @Ashaba. I have changed this to url: '/dashboard'. This way, Ajax will automatically append /dashboard to whatever the current root url is.

@eric-elem eric-elem force-pushed the ch-requests-payload branch from 7285d0c to c272aee Compare March 1, 2018 08:31
@coveralls
Copy link

coveralls commented Mar 1, 2018

Pull Request Test Coverage Report for Build #54

  • 0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 31.454%

Totals Coverage Status
Change from base Build #51: 0.0%
Covered Lines: 24405
Relevant Lines: 77590

💛 - Coveralls

data: JSON.stringify(data),
contentType: "application/json; charset=utf-8",
success: function (response) {
console.log(response);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should logs be allowed in production code?

Copy link
Collaborator Author

@eric-elem eric-elem Mar 5, 2018

Choose a reason for hiding this comment

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

No, they shouldn't. I added this line for debug purposes and forgot to remove it. I have removed it now. I suggest we add a story to automate checking of such using git hooks. CC @Ashaba

@eric-elem eric-elem force-pushed the ch-requests-payload branch from c272aee to f1c7052 Compare March 5, 2018 08:52
@muhallan
Copy link
Collaborator

muhallan commented Mar 5, 2018

Please rebase with develop.

@eric-elem eric-elem force-pushed the ch-requests-payload branch from f1c7052 to 31b7375 Compare March 5, 2018 13:18
@muhallan
Copy link
Collaborator

muhallan commented Mar 5, 2018

I have noticed that one can send multiple headers with the same key and they are allowed. Is that the accepted behaviour?

Copy link
Owner

@Ashaba Ashaba left a comment

Choose a reason for hiding this comment

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

Add tests.

@muhallan
Copy link
Collaborator

muhallan commented Mar 7, 2018

@Ashaba why have you closed this?

@Ashaba
Copy link
Owner

Ashaba commented Mar 7, 2018

Please create a new PR

@eric-elem eric-elem deleted the ch-requests-payload branch March 29, 2018 06:46
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.

5 participants