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

HTTP/422 When Attempting to Assign Existing Tag to Stage #250

Closed
prohrlac opened this Issue Sep 23, 2017 · 26 comments

Comments

Projects
None yet
5 participants
@prohrlac
Member

prohrlac commented Sep 23, 2017

image

image

image

@thongteav

This comment has been minimized.

Contributor

thongteav commented Sep 23, 2017

Hi @rohrlach, is there a certain way to replicate this bug?

This is what I have when doing it:
image

@thongteav

This comment has been minimized.

Contributor

thongteav commented Sep 23, 2017

Oh I got it, the first time you add a tag 'test test', it's automatically converted to 'test-test' and it works. But the second time when you try to add 'test test', it won't work and will give you that bug. Also, if you do 'test-test', it should be fine too.

@prohrlac

This comment has been minimized.

Member

prohrlac commented Sep 23, 2017

Okay. This isn't the expected behavior. The system should be able to handle that.

@prohrlac prohrlac modified the milestone: Sprint 11 Sep 23, 2017

@charlie0605 charlie0605 self-assigned this Sep 25, 2017

@charlie0605

This comment has been minimized.

Contributor

charlie0605 commented Oct 3, 2017

Hi @rohrlach, it seems like when trying to find new tag the error arises. In the controller create method. @tag = Tag.find_by_name(params[:tag][:name]), this line, the params is not formatted before it asks if the tag is existed in the database. I've added .strip.downcase.gsub(/[^a-z0-9\s-]/, '').gsub(/\s/, '-') at the end and it allowed adding existing tag with space to perspective but when there is empty name it still give HTTP 422 error, so i check before it tries to find it from the database if the param is empty. Do you have any suggestions on how I could go about doing this?

@prohrlac

This comment has been minimized.

Member

prohrlac commented Oct 3, 2017

@charlie0605 I believe your diagnosis to be correct, having looked at the code again.

You'll notice that the Tag model has a single validation instruction that makes sure the name conforms and is unique:

validates :name, :presence => true, :uniqueness => true, :format => /[a-z0-9\-]/, :length => { maximum: 25 }

You'll also notice that the name is "standardized" before validations are run:

before_validation :standardize_name

What I suggest, is that you re-implement the create action such that it utilizes this workflow. Consider the following:

  • Create a new object with the given parameters
  • Check if it's valid
  • If yes
    • This must be a new tag
    • Create the tag
  • If no, we can assume that it's because the tag exists
    • Fetch the existing tag
    • Use that instead
@prohrlac

This comment has been minimized.

Member

prohrlac commented Oct 3, 2017

Also worth noting: it'd be good to write some tests for this once you've fixed the issue

@thongteav

This comment has been minimized.

Contributor

thongteav commented Oct 6, 2017

This should address bug #172 upon completion.

@charlie0605

This comment has been minimized.

Contributor

charlie0605 commented Oct 8, 2017

@rohrlach How would I go about testing when the tag name is empty. I have got some test up. Now I'm trying to test when the tag name is empty on stage. How would I simulate when the player is on the stage page and clicked on add tag and it renders the form, then they enter an empty name. I'm trying to test for the presence of flash[:danger]

@vickis

This comment has been minimized.

vickis commented Oct 9, 2017

@prohrlac

This comment has been minimized.

Member

prohrlac commented Oct 10, 2017

@charlie0605 can you provide the spec you have thus far?

@charlie0605

This comment has been minimized.

Contributor

charlie0605 commented Oct 10, 2017

@rohrlach I have committed the code in issue#250 branch

@prohrlac

This comment has been minimized.

Member

prohrlac commented Oct 10, 2017

Does what you're currently doing not work?

@charlie0605

This comment has been minimized.

Contributor

charlie0605 commented Oct 10, 2017

No, the flash[:danger] is nil
image

@prohrlac

This comment has been minimized.

Member

prohrlac commented Oct 10, 2017

Ahh - It's probably because whilst you are using a spec file for the controller, you are only actually dealing with models.

You need to be testing TagsController and StagesController not Stage and Tag

prohrlac pushed a commit that referenced this issue Oct 15, 2017

prohrlac pushed a commit that referenced this issue Oct 16, 2017

prohrlac pushed a commit that referenced this issue Oct 16, 2017

rohrlach
Address #250
- Tags can now ONLY be created if they are also going to be assigned to something
- Format is enforced in before_validation hook, don't need to validate it

prohrlac pushed a commit that referenced this issue Oct 16, 2017

Paul Rohrlach
Issue #250 (#277)
* Address #250

- Tags can now ONLY be created if they are also going to be assigned to something
- Format is enforced in before_validation hook, don't need to validate it

* Fix removal issue

* Fix contextually aware 'Add' button

* added unit test

* changed format

* adding test for empty tag name

* Rspec test for Tag controller complete

* remove unused code in spec_helper

* changed description

* changed description

* updated tag tests

* update tag tess

* Added Tags to Navbar

@prohrlac prohrlac added this to the Sprint 12 milestone Oct 16, 2017

@prohrlac prohrlac assigned helenvarley and vickis and unassigned charlie0605 Oct 16, 2017

@prohrlac prohrlac added qa and removed stages labels Oct 16, 2017

@prohrlac

This comment has been minimized.

Member

prohrlac commented Oct 16, 2017

@thongteav I've also added you as someone who can test this in QA

@thongteav

This comment has been minimized.

Contributor

thongteav commented Oct 18, 2017

Hi @rohrlach, do I just log in to https://limelight-qa.upstage.org.nz/login with same login as limelight upstage?

@helenvarley

This comment has been minimized.

helenvarley commented Oct 18, 2017

not quite sure what i'm meant to test here, but i did the following:

  • went to the edit interface for the hentest stage & added the tag "test"
  • went to media, created a tag "helen" on an avatar, then went to the edit interface for the hentest stage & added the tag "helen" to this stage. i typed h in the input field & "helen" appeared as a dropdown so i selected it.

i see that there is now a "tags" link in the navigation, & when i click on this i get a list of tags, but clicking on a tag in this list does not then present me with a list of media items & stages that have this tag - which is what i expected to happen. (new issue?)

@charlie0605

This comment has been minimized.

Contributor

charlie0605 commented Oct 18, 2017

@helenvarley are you referring to #210 ?

@helenvarley

This comment has been minimized.

helenvarley commented Oct 18, 2017

yes, thank you! :)

@charlie0605

This comment has been minimized.

Contributor

charlie0605 commented Oct 18, 2017

This issue is mainly to do with when you add duplicate tags with space in the tag name. The second time you add it the console should not get an HTTP 422 error. It should redirect you back to the page you were on ie stage if you were trying to add tag to the stage

@helenvarley

This comment has been minimized.

helenvarley commented Oct 18, 2017

ok - i'm not really sure how to test that. i didn't get any errors.

@prohrlac

This comment has been minimized.

Member

prohrlac commented Oct 21, 2017

@thongteav No, you'll need to create another account - The envs are isolated.

@thongteav

This comment has been minimized.

Contributor

thongteav commented Oct 22, 2017

Ok, I've created the account. Waiting for it to get activated.

@prohrlac

This comment has been minimized.

Member

prohrlac commented Oct 22, 2017

Done

@thongteav

This comment has been minimized.

Contributor

thongteav commented Oct 22, 2017

Testing procedure:

  • creating new tag for 'test 2'
  • creating another tag with 'test 2'
  • creating another one with 'test-2'
  • unassigned the tag

Result: No http/422 error found.
Tested on: Google chrome, Microsoft edge, Firefox on windows.
image

image

image

@thongteav

This comment has been minimized.

Contributor

thongteav commented Oct 23, 2017

@rohrlach So is this fine?

@prohrlac prohrlac closed this Feb 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment