cpfergus1 / zylytyPractice

0 stars 1 forks source link

ZYLYTY #1

Open ZYLYTY opened 5 months ago

ZYLYTY commented 5 months ago

Hello @cpfergus1,

Thank you for submitting your challenge to us.

You are receiving this message because we found the score of your project quite low, which usually triggers some alerts in our system that highlight the possibility of your submission having been unfairly scored by our system.

I will be putting here some findings so we can try to reach a fairer score. 😃

cpfergus1 commented 5 months ago

Also, don't forget the payload that /login needs to return, it is not just the Set-Cookie header and HTTP 200, there's a JSON body it should return as well.

💯 it returns a json response...

And yes, I would expect an issue if the header does not set content-type to "application/json"

maybe it is defaulting to text?

I don't think Rails out of the box automatically expects that header and I was assuming that it was set... That might be the issue. I will make a quick change on that

cpfergus1 commented 5 months ago

Do you have a record of what your request looks like with headers so I can verify?

cpfergus1 commented 5 months ago

I just pushed another commit to force the request to application json if it is not set. It would then try to parse the body and either modify the request to application/json or revert to original context if there is a json parse error. This is a little heavy handed, but it may work. Please let me know if you see the same error

cpfergus1 commented 5 months ago

Also found the endpoint for importing users and delete paths was incorrect, ended up fixing that as well 🤦

cpfergus1 commented 5 months ago

I think I found the issue: this is how I am receiving the content-type "application/x-www-form-urlencoded"

cpfergus1 commented 5 months ago

Okay! I fixed the issue for login, the error was occurring because rails was reading the incoming content type as "application/x-www-form-urlencoded"

So when it tried to render the response, it was looking for the correct format and trying to respond in HTML. I was assuming that we would always have the header "application/json" except for import requests so I only created json responses. I never created an HTML format response so it was returning a 400 for cannot find response template

ZYLYTY commented 5 months ago

Great, it seems the login issue is gone now:

image

Can you check the other endpoints? The login now is even sending the JSON response correctly, so you nailed the bug.

You have other endpoints behaving the same way as the login was behaving.

Eg:

image

image

Maybe the issue is the same?

cpfergus1 commented 5 months ago

I believe so, I am looking into it now

cpfergus1 commented 5 months ago

Okay, I found a few issues, one was on the creation of a new thread, I missed the complete structure ->

I had:

{
  "category": "Default",
  "title": "Exciting Discussion",
  "openingPost": {
    "Let's talk about something exciting!"
  }
}

Whereas the story had:

{
  "category": "Default",
  "title": "Exciting Discussion",
  "openingPost": {
    "text": "Let's talk about something exciting!" <---- MISSING TEXT KEY
  }
}

Additionally, on the return of a thread, my structure was off when trying to use Jbuilder to generate the json response. I was missing a key so it faulted and returned a 400 for not being able to correctly build the object. Both of these issues were due to lack of attention to detail! 😭

Last I was looking further down and noticed that all of your user import stories are sending content-type: application/json

where as the story is:

POST /csv
Content-Type: text/csv <----- type mismatch here
Token: <admin_api_key>

username,password,e-mail
user1,password1,user1@example.com
user2,password2,user2@example.com

This will also blow up on my end. Should I change to account for application/json or is that an error on the request side?

image

  def import
    unless params[:file].present? && params[:file].content_type == "text/csv"
      render json: { error: 'Bad Request: Please provide a valid CSV file' }, status: :bad_request
      return
    end
   ....

P.S. I pushed changes to fix the first two items

cpfergus1 commented 5 months ago

I modified the csv endpoint to not care about the headers and look for File or Text as the parameters based on the input.

Hopefully that will remedy that issue.

Let me know what you find on the next run 😄

cpfergus1 commented 5 months ago

This story is not clear: image

image

Are we supposed to return {"searchResults: []"} or nothing at all?

ZYLYTY commented 5 months ago

Okay, I found a few issues, one was on the creation of a new thread, I missed the complete structure ->

I had:

{
  "category": "Default",
  "title": "Exciting Discussion",
  "openingPost": {
    "Let's talk about something exciting!"
  }
}

Whereas the story had:

{
  "category": "Default",
  "title": "Exciting Discussion",
  "openingPost": {
    "text": "Let's talk about something exciting!" <---- MISSING TEXT KEY
  }
}

Additionally, on the return of a thread, my structure was off when trying to use Jbuilder to generate the json response. I was missing a key so it faulted and returned a 400 for not being able to correctly build the object. Both of these issues were due to lack of attention to detail! 😭

Last I was looking further down and noticed that all of your user import stories are sending content-type: application/json

where as the story is:

POST /csv
Content-Type: text/csv <----- type mismatch here
Token: <admin_api_key>

username,password,e-mail
user1,password1,user1@example.com
user2,password2,user2@example.com

This will also blow up on my end. Should I change to account for application/json or is that an error on the request side?

image

  def import
    unless params[:file].present? && params[:file].content_type == "text/csv"
      render json: { error: 'Bad Request: Please provide a valid CSV file' }, status: :bad_request
      return
    end
   ....

P.S. I pushed changes to fix the first two items

You are right! That request is not correct, the content type should be text/csv. 🤯

I'll request a fix for that test.

Thanks for reporting!

ZYLYTY commented 5 months ago

This story is not clear: image

image

Are we supposed to return {"searchResults: []"} or nothing at all?

It should be {"searchResults" : { } } (not [ ]). The numbers represent the thread ids where you found matches, and the arrays associated with the numbers are the matches with the extra 3 adjacent words on each side.

Yeah this instruction section is a bit tricky to explain, maybe we can add more examples.

Thank you for sharing the feedback 💪

This endpoint is the most tricky of them all because it has tiny details in its implementation and in the way it presents the matching results.

cpfergus1 commented 5 months ago

I corrected what I could with the current error returns from my latest results. I would be interested to see what else I would need to fix after the next run 😄

Just a note: I also found these stories to not be clear, but after seeing the test name I realized what I actually should be doing: image image

These tests passed on accident due to the previous errors with the content-type and would have failed if the correct content-type was passed. I initially created users and returned - row: error and didn't rollback the transactions that passed if there were any. The uncertainty here whether or not we should actually roll back the entire transaction instead of just processing the good rows (perhaps this is a best practice I am not aware of yet).

ZYLYTY commented 5 months ago

I just re-runned the evaluator. You have a typo that is affecting you many tests:

image

You are sending "created_at" instead of "createdAt".

ZYLYTY commented 5 months ago

I corrected what I could with the current error returns from my latest results. I would be interested to see what else I would need to fix after the next run 😄

Just a note: I also found these stories to not be clear, but after seeing the test name I realized what I actually should be doing: image image

These tests passed on accident due to the previous errors with the content-type and would have failed if the correct content-type was passed. I initially created users and returned - row: error and didn't rollback the transactions that passed if there were any. The uncertainty here whether or not we should actually roll back the entire transaction instead of just processing the good rows (perhaps this is a best practice I am not aware of yet).

There are some dependencies between tests. If we create a category in one test, we might expect that category to be there in the future. If such test fails, it will have a cascading effect on future tests that depend on that result. The same with the registered user, since we then use its session to test everything else.

This is the nature of testing a blackbox, the alternative would be to reset the project at every test which is something undergoing some research, given that it simplifies testing at runtime, but makes the tests more complex and consequently prone to issues.

Regarding the CSV story, what do you think could be changed to make it clearer?

Thanks!

cpfergus1 commented 5 months ago

Good morning!

Thanks for the update, 💯 that typo got me. Those tiny details....

I will create create summarized list for you all, especially for all the help you have given me, I will try to point out where I feel there could certainly be more context!

I was curious on search results story where we return empty search results when the search comes back. what did you expect the return to be? image

cpfergus1 commented 5 months ago

I fixed the typo! I think that should fix most of those errors

cpfergus1 commented 5 months ago

Thank you again for all your time in allowing me to run my program multiple times through your system!

There were certainly a lot of issues on my end which prevented a successful run with your tests, and I also feel that some of the tests could have utilized a little more explanation. I see that you have already updated the story to point out some of attention to detail issues, which are all wonderful additions.

After going through each of these stories, I feel the following could be updated.

-Import Users via CSV Endpoint - 400 Bad Request on Error - Once of your tests expected that the entire transaction is rolled back on a single error with any of the users such as duplicates. This is not clear in the story and should be specified IMO.

Thoughts on the assignment:

Implementation of a couple of these could help grow adoption by candidates as a learning and code sampling tool, as well as employers looking for better ways to gauge candidates during the hiring process.

Lastly, it looks like the runs we did this weekend saturated your landing page examples and may need to be purged 😄

While I realize it might be asking too much, but I am glutton for punishment and would love to see one more run if that is in the realm of possibilities. Again, thank you and your team!

FeatureSpitter commented 5 months ago

Hey @cpfergus1 oh wow, that's quite extensive feedback!

Indeed, this challenge is relatively new, the instructions are lacking, we reviewing them in order to add more detail (as you already noticed as well :smile:). The challenge here is finding that thin balance between having a complete specification, but without telling the developer what needs to be done step by step (for eg. we don't mention anywhere that passwords needs to be hashed, but we penalize developers who don't do it -- because we try to find if the developer has the natural habit of hashing passwords). But in the cases you mentioned the instructions are clearly lacking some clarity indeed.

The Content-Type in the CSV endpoints was buggy, at this moment it should be returning the correct type, I hope.

Consider providing X number of tries for an example, each with their own score. Your involvement and logs were tremendously helpful to understand what was going on making corrections based on how the the program was being processed. Additionally this tool became a wonderful learning session that helped brush off a lot of rust in areas of RoR I have not touched in my professional experience in a while.

We were actually discussing that internally today! We need to determine X basically, because given enough attempts, anyone can score 100%. But we also do understand that 0 attempts is unfairly punitive, specially for ZYLYTY newcomers who are trying the engine for the first time.

Consider limiting the number of Gotchas. It is incredibly important to have attention to detail and I completely understand the use non-RESTFUL patterns to see if the candidate catches these differences in the story. IMO breaking from this pattern so often leads me to be less inclined to show this particular code sample recruiters due to how far it deviates from certain common patterns. It also seems more important to recruiters that the developer can produce performant and highly scalable solutions while maintaining readability.

That's a very good point. Maybe we should try to break from the pattern just in a few localized test cases (for eg. simulating an integration with one single legacy API endpoint), and not within the entire solution (for eg. the case with the Authorization token, were we decided to go with our custom Token header which probably contributes to less appealing code).

Clarity on scoring criteria would be helpful as I still do not fully understand how the assignments scores relate to the results, i.e. I counted 59 tests of which 32 passed for 54% over all, but functionality is @ 24 on the dash, which test counts for which category? Same for performance, am I being scored against all other languages or against other ruby environments? Is there a static goal to achieve? This would help the candidate learn from the the assignment and try to find out how to make their programs more performant for the next exercise.

We are still elaborating the explanation of each evaluated dimension, hopefully it will be ready before the next 3 months :stuck_out_tongue: . Also, recently we even added a new one that evaluates the quality of the data model in the relational database.

All the scores you see in the radar chart go from 0 to 100 and they are absolute, i.e. they are calculated with your submission alone, and not against the performance of other challenges. So multiple users can have 100% if they meet all goals in the challenge.

On the other hand, your position in the environment impact, because it is unbounded (0 to infinity) is calculated in comparison with everyone else, within the same language.

Regarding your question on the functional tests, the discrepancy you see in the number of tests passed and the score is due to the relative weight of each test. Each test has a weight based on how important recruiters find them. We can eventually publish these weights in the detail logs so that you know in a retry where you should focus your time to increase your score effectively.

Along with the scoring criteria, generalized automated feedback could be a wonderful addition too. i.e. based on some performance, responses could be generated to notify the user where they could concentrate their efforts to improve, such as performance, "We are seeing an avg response time from your program to be X when we really are expecting it to be closer to Y"

That is also a feature we are working, in some places deterministically, in other places we are testing the usage of a trained LLM to provide a more high-level feedback regarding the project's design.

Together with this we are also designing new challenges, and working in isolating the tests to avoid those nasty cascade assertion failures.

Lastly, it looks like the runs we did this weekend saturated your landing page examples and may need to be purged 😄

Yeah, the platform is not used to multiple retries, but it is an easy fix, we just have it grab the latest submission per candidate.

While I realize it might be asking too much, but I am glutton for punishment and would love to see one more run if that is in the realm of possibilities. Again, thank you and your team!

Yeah, just let us know when you want us to trigger the evaluation again for this challenge.

Thank you for this very detailed feedback! :smiley:

cpfergus1 commented 5 months ago

Yeah, just let us know when you want us to trigger the evaluation again for this challenge.

Please do! I have corrected what I believe to be the issues we found on Saturday. I also updated the import to look only for text/plain coming in which I hope resolves the issue of finding the CSV data. Only issues I think might pop up now are things I could not have seen in the cascading failures 😄 Thank you so much!

ZYLYTY commented 5 months ago

@cpfergus1 You should be able to see the new results now!

cpfergus1 commented 5 months ago

@cpfergus1 You should be able to see the new results now!

Thank you, I am still curious as to how my csv endpoint is still rejecting your request and how these might have failed: image My response is a 201 but it still had an unexpected error. image I returned the empty search object discussed in https://github.com/cpfergus1/zylytyPractice/issues/1#issuecomment-2028486639 but still failed 🤔

Oh well, I have taken enough of your time and I will look forward to the next challenge!

ZYLYTY commented 5 months ago

Very curious, could be a bug on our side.

Meanwhile I notice you are failing on of the tests because of this:

image Counts 101

image Counts 1

But now we should count 102, but you are returning 101: image

You are considering just the last category of the filter: image

cpfergus1 commented 5 months ago

You are considering just the last category of the filter: ![image](https://private-user-images.githubusercontent.com/

I must not be receiving your list as expected, perhaps another misunderstanding of how I should expect the request to send the parameters. I am currently splitting a string list, i.e. "test8, Default".split(",") > ["test8", "Deafault"] if I understand listOf() correctly, I might be receiving "['test8','Default']" which would end up being ["[test8", "Default]"].

ZYLYTY commented 5 months ago

Your framework doesn't parse the JSON payload automatically for you?

The order of the list shouldn't matter, since you would end up returning the threads of both categories present in that list anyway.

ZYLYTY commented 5 months ago
    Request #2:
        POST /thread
        Cookie: session=eyJhbGciOiJIUzI1NiJ9.eyJ1c2VyX2lkIjoxLCJleHAiOiIxNzEyMjMxOTk0In0.Ru3iTadFJ0yE-13U0ABjZTB7creimkhLuJKcURXZ600
        Content-Type: application/json
        Content-Length: 2772

        {"category":"shared category","title":"My first thread","openingPost":{"text":"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Duis sed ultrices metus. Aliquam sed ligula elementum, pulvinar nisi et, aliquet nisi. Nullam aliquam massa et tempus cursus. Curabitur nisl ligula, venenatis et eg...

    Response:
        HTTP Code: 404
        X-Frame-Options: SAMEORIGIN
        X-XSS-Protection: 0
        X-Content-Type-Options: nosniff
        X-Download-Options: noopen
        X-Permitted-Cross-Domain-Policies: none
        Referrer-Policy: strict-origin-when-cross-origin
        Content-Type: application/json
        Cache-Control: no-cache
        X-Request-Id: 816e4c16-3570-425b-a73b-04d3faeb30bc
        X-Runtime: 0.002337
        vary: Origin
        Transfer-Encoding: chunked
        Content-Length: -1

I don't understand if this is an issue on our engine with your project, or if you are in fact returning 404 when trying to create a thread.

cpfergus1 commented 5 months ago

Your framework doesn't parse the JSON payload automatically for you?

The order of the list shouldn't matter, since you would end up returning the threads of both categories present in that list anyway.

Good morning!

Yes, the order doesn't matter, however if the uri looks like:

image

OR

image

Then the format coming into rails looks like:

image

and

image

In Rails, parameters are always passed as strings (even boolean), so if I was expecting a string array, I could first strip the brackets and then split/process the elements.

I'll be honest though, I am not quite sure how they are passed based on your screen in https://github.com/cpfergus1/zylytyPractice/issues/1#issuecomment-2033222221.

As for the directly preceding comment -> I would guess just by looking at the above, that the category "shared category" does not exist because you are we are returning a 404 there.

Here is my endpoint:

image

The method find_by! will raise an error ActiveRecord::NotFound if the category is not found, which will trigger the 404 response. This is also a case sensitive search, so if the category "Shared category" was created before, then that would be the issue there. The payload structure looks fine to me and even works on my end if I mock something similar. Here is how I process it:

image

current_user is found by processing a valid JWT which has the user_id of the user making the request it (from the session cookie). If that user is not found, you would end up getting a 401. Error on creation of the model would have result in a 400

By the way, if you would like to have a call on this to get it sorted out, please let me know! I have time and would love to get to the bottom of this as well 😄

Here is a similar call working on my end: image image

cpfergus1 commented 5 months ago

I updated the index action to consider the incoming parameter to be either a single word string or a parse-able JSON string, i.e. "[\"cum\",\"ipsum\",\"libero\"]" or "ipsum"

I think this is why it is a valid concern to have an example in the story with how multiple categories will be passed as there could be a specific way to correctly handle the incoming parameter 😄

ZYLYTY commented 5 months ago

Your framework doesn't parse the JSON payload automatically for you? The order of the list shouldn't matter, since you would end up returning the threads of both categories present in that list anyway.

Good morning!

Hello @cpfergus1 How are you?

Yes, the order doesn't matter, however if the uri looks like:

image

OR

image

Then the format coming into rails looks like:

image

and

image

In Rails, parameters are always passed as strings (even boolean), so if I was expecting a string array, I could first strip the brackets and then split/process the elements.

I'll be honest though, I am not quite sure how they are passed based on your screen in #1 (comment).

Indeed, the original instructions you got weren't clear about this, and to my admiration, no single framework supports the 3 more standard formats of passing arrays in the GET query, which is crazy to still be the case in 2020s:

https://stackoverflow.com/questions/6243051/how-to-pass-an-array-within-a-query-string

The way we are passing the array:

GET /thread?categories=Default&categories=Funny&newest_first=true&page=0&page_size=10
Cookie: session=<session_cookie>

This would equate to "categories"=>[Default,Funny].

Jeez, which one of the 3 non-standard ways to pick...

cpfergus1 commented 5 months ago

So when passing

GET /thread?categories=Default&categories=Funny&newest_first=true&page=0&page_size=10
Cookie: session=<session_cookie>

Rails parses that URI as: image

It just overwrites the preceding category list 🤯 I would have to modify the middleware to try and parse that param correctly or maybe I am missing something...

The more you know! 🌈 ⭐

ZYLYTY commented 5 months ago

That explains the issue. I am really baffled that there is no standard way of passing an array. This causes a problem because it is nowhere a goal of this test to verify if your framework can handle arrays in GET queries 😆

cpfergus1 commented 5 months ago

I just looked it up for Rails, to pass an array in the URI, it must be formatted like so: Array /thread?categories[]=Default&categories[]=Funny&newest_first=true&page=0&page_size=10 Hash /thread?categories[someKey]=Default&categories[anotherKey]=Funny&newest_first=true&page=0&page_size=10

That certainly makes testing on your side more difficult. Unless you have candidates specifically modify their frameworks to accept URI parameters in specific way, you might have to identify the framework and pass formatted parameters accordingly

ZYLYTY commented 5 months ago

As for the directly preceding comment -> I would guess just by looking at the above, that the category "shared category" does not exist because you are we are returning a 404 there.

Here is my endpoint:

image

The method find_by! will raise an error ActiveRecord::NotFound if the category is not found, which will trigger the 404 response. This is also a case sensitive search, so if the category "Shared category" was created before, then that would be the issue there. The payload structure looks fine to me and even works on my end if I mock something similar. Here is how I process it:

image

current_user is found by processing a valid JWT which has the user_id of the user making the request it (from the session cookie). If that user is not found, you would end up getting a 401. Error on creation of the model would have result in a 400

By the way, if you would like to have a call on this to get it sorted out, please let me know! I have time and would love to get to the bottom of this as well 😄

Here is a similar call working on my end: image image

Yes you're right, for some reason the categories were not getting created (this does not affect your score tho, as this is being tested in the new test version that isolates tests per suit.

ZYLYTY commented 5 months ago

I just looked it up for Rails, to pass an array in the URI, it must be formatted like so: Array /thread?categories[]=Default&categories[]=Funny&newest_first=true&page=0&page_size=10 Hash /thread?categories[someKey]=Default&categories[anotherKey]=Funny&newest_first=true&page=0&page_size=10

That certainly makes testing on your side more difficult. Unless you have candidates specifically modify their frameworks to accept URI parameters in specific way, you might have to identify the framework and pass formatted parameters accordingly

We will just try the 3 ways, the test passes if at least one is ok.

ZYLYTY commented 5 months ago
Expected status code 404, got 200 instead
Request history:

    Request #1:
        POST /user/login
        Content-Type: application/json
        Content-Length: 46

        {"username":"test","password":"testpwd111!!!"}

    Response:
        HTTP Code: 200
        X-Frame-Options: SAMEORIGIN
        X-XSS-Protection: 0
        X-Content-Type-Options: nosniff
        X-Download-Options: noopen
        X-Permitted-Cross-Domain-Policies: none
        Referrer-Policy: strict-origin-when-cross-origin
        Set-Cookie: session=eyJhbGciOiJIUzI1NiJ9.eyJ1c2VyX2lkIjoxLCJleHAiOiIxNzEyMjU5MTk3In0.jdHh3Dc3C2nxWDerKK7P_CqOBp0-M5gTEuAQmjS9A3Q
        Content-Type: application/json; charset=utf-8
        ETag: W/"a889223570485b11f843669216ca3e13"
        Cache-Control: max-age=0, private, must-revalidate
        X-Request-Id: 59736cbc-f4ea-4214-aa4d-2f005afc2c11
        X-Runtime: 0.203384
        vary: Origin
        Transfer-Encoding: chunked
        Content-Length: -1

        {"username":"test","email":"test@zylyty.com"}

    Faulty Request:
        GET /thread?categories=---------------&newest_first=true&page=0&page_size=10
        Cookie: session=eyJhbGciOiJIUzI1NiJ9.eyJ1c2VyX2lkIjoxLCJleHAiOiIxNzEyMjU5MTk3In0.jdHh3Dc3C2nxWDerKK7P_CqOBp0-M5gTEuAQmjS9A3Q
        Content-Type: application/json

    Response:
        HTTP Code: 200
        X-Frame-Options: SAMEORIGIN
        X-XSS-Protection: 0
        X-Content-Type-Options: nosniff
        X-Download-Options: noopen
        X-Permitted-Cross-Domain-Policies: none
        Referrer-Policy: strict-origin-when-cross-origin
        Content-Type: application/json; charset=utf-8
        ETag: W/"fb07c9eca1ff0193819d488f8afca383"
        Cache-Control: max-age=0, private, must-revalidate
        X-Request-Id: 512a7361-95ff-48a9-9fdc-3d3f07a15abe
        X-Runtime: 0.003035
        vary: Origin
        Transfer-Encoding: chunked
        Content-Length: -1

        {"threads":[]}

This one is simple to fix :)

cpfergus1 commented 5 months ago

This one is simple to fix :)

Hah, after 15+ retries I figured I should get some wrong 😄