Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

CM_FormField_Location add setValueByRequest #676

Merged
merged 20 commits into from
Jul 12, 2013
Merged

CM_FormField_Location add setValueByRequest #676

merged 20 commits into from
Jul 12, 2013

Conversation

sweleck
Copy link
Contributor

@sweleck sweleck commented Jul 5, 2013

@ghost ghost assigned sweleck Jul 4, 2013
@sweleck
Copy link
Contributor Author

sweleck commented Jul 5, 2013

@alexispeter please review. I don't see a chance how I can test it... do you have any suggestion?

@alexispeter
Copy link
Contributor

You should be able to instantiate a new Request with the corresponsing headers to set the ip.

@njam
Copy link
Member

njam commented Jul 5, 2013

2 small notes:

  • Please pass the request, using global state is bad, and we want to do it as far on the "outside" as possible therefore
  • getIp can return null
  • If the level from getByIp is too "high" you can try getting a lower level from the location object

@sweleck
Copy link
Contributor Author

sweleck commented Jul 8, 2013

@alexispeter please review and merge if ok.

@alexispeter
Copy link
Contributor

retest this please

@alexispeter
Copy link
Contributor

  • setValueByRequest() should also try to get the right level when it's smaller than levelMin
  • fix tests

@sweleck
Copy link
Contributor Author

sweleck commented Jul 9, 2013

@alexispeter please review and merge if ok

@njam
Copy link
Member

njam commented Jul 9, 2013

  • Docu
  • Request::getIp can return null (add test?)
  • level is compared with > and <= - shouldn't it either both times include or not include the current level? (not sure)
  • Tests: Instead of writing all this data to the database, would it be possible to use mocks? This would make it more maintainable.

@alexispeter
Copy link
Contributor

  • if ($requestLocation->getLevel() <= $this->_options['levelMin']) { should probably be if ($requestLocation->getLevel() < $this->_options['levelMin']) {
  • please also test that it works correctly when the location level is not high enough (you only test the other case)
  • test what happens if it couldn't find a location from the ip
  • test what happens if it couldn't get the desired level of location

@sweleck
Copy link
Contributor Author

sweleck commented Jul 10, 2013

@alexispeter @njam

please review

@alexispeter
Copy link
Contributor

  • tests/library/CM/FormField/LocationTest.php use $this->assertNull($foo); instead of $this->assertSame(null, $foo);. Plus, when using assertSame($foo, $bar); the first arg should be the expected value and the second arg the actual value.
  • Put commits that belong together together. E.g. 452b718 and 3a4304c

@sweleck
Copy link
Contributor Author

sweleck commented Jul 11, 2013

@alexispeter please review.

I don't see where I violated this:

assertSame($foo, $bar); the first arg should be the expected value and the second arg the actual value.

@alexispeter
Copy link
Contributor

In e4fe53d, but it was changed in d45e683.

@sweleck
Copy link
Contributor Author

sweleck commented Jul 11, 2013

Sorry for mixing up the commits. Everything else ok?

@alexispeter
Copy link
Contributor

All good for me.

@sweleck
Copy link
Contributor Author

sweleck commented Jul 12, 2013

@fauvel please merge and create a tag.

fauvel added a commit that referenced this pull request Jul 12, 2013
CM_FormField_Location add setValueByRequest
@fauvel fauvel merged commit 8f2b2f0 into cargomedia:master Jul 12, 2013
@fauvel
Copy link
Contributor

fauvel commented Jul 12, 2013

Tagged 1.1.151

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

Successfully merging this pull request may close these issues.

4 participants