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

Model Alternative Key #431

Closed
wants to merge 10 commits into from
Closed

Model Alternative Key #431

wants to merge 10 commits into from

Conversation

SmolSoftBoi
Copy link
Contributor

@SmolSoftBoi SmolSoftBoi commented Mar 7, 2017

To be able to specify an alternative key. For example; a country or currency alpha code, or a URL slug, etc…

Added $altKey property, findByKey($key), findByIdKey($idKey), saveByKey($key, $data), saveByIdKey($idKey, $data), updateByKey($key, $data), updateByIdKey($idKey, $data), deleteByKey($key) and deleteByIdKey($idKey) public methods, and findBy($byKeys, $value), saveBy($byKeys, $data), updateBy($byKeys, $key, $data) and deleteBy($byKeys, $key, bool $purge = false) protected methods.

Issue: #428

Signed-off-by: Kristian Matthews kristian.matthews@me.com

To be able to specify an alternative key. For example; a country or currency alpha code, or a URL slug, etc…

Added `$altKey` property, `findByKey($key)`, `findByIdKey($idKey)`, `saveByKey($key, $data)`, `saveByIdKey($idKey, $data)`, `updateByKey($key, $data)`, `updateByIdKey($idKey, $data)`, `deleteByKey($key)` and `deleteByIdKey($idKey)` public methods, and `findBy($byKeys, $value)`, `saveBy($byKeys, $data)`, `updateBy($byKeys, $key, $data)` and `deleteBy($byKeys, $key, bool $purge = false)` protected methods.

Signed-off-by: Kristian Matthews <kristian.matthews@me.com>
@SmolSoftBoi
Copy link
Contributor Author

Are there any Model tests that I'm unaware of? I'm not sure how to run database tests locally either.

This was the implementation I was thinking of, let me know your thoughts, and when agreed I'll add the documentation.

@kenjis
Copy link
Member

kenjis commented Mar 7, 2017

Are there any Model tests that I'm unaware of?

See https://github.com/bcit-ci/CodeIgniter4/blob/develop/tests/system/Database/Live/ModelTest.php.

I'm not sure how to run database tests locally either.

See https://github.com/bcit-ci/CodeIgniter4/tree/develop/tests#setup.

Missed passing the key.

Signed-off-by: Kristian Matthews <kristian.matthews@me.com>
@SmolSoftBoi
Copy link
Contributor Author

@kenjis Thanks for pointing me to the model tests, I think we need some documentation on running database tests.

@lonnieezell
Copy link
Member

@EpicKris Here's what we have currently: https://bcit-ci.github.io/CodeIgniter4/general/testing.html#testing-your-database

Does that help?

@lonnieezell
Copy link
Member

lonnieezell commented Mar 8, 2017

I'm not 100% on the implementation, honestly. A number of the items, like findByKey() and the like can already be handled just fine without adding a bunch of new methods, by doing $model->findWhere(key, x). The way I thought you might implement it was to add a withKey method, or something, that would temporarily override the primary key and would be used for save/update type methods. Similar to the soft delete functionality.

It's pretty late and I'm getting ready to crash, so I'll take a better look at this in the morning.

@SmolSoftBoi
Copy link
Contributor Author

@lonnieezell I agree with your solution, this was done quickly and obviously wasn't a good solution, I'll re-work this when I've got some time.

Let me know if you have any other thoughts after taking another look in the morning.

Added `withKey(string $key = null)` and `orWithKey(string $key = null)` methods.

Signed-off-by: Kristian Matthews <kristian.matthews@me.com>
@SmolSoftBoi
Copy link
Contributor Author

@lonnieezell Is this more in-line with what you had in mind?

@lonnieezell
Copy link
Member

@EpicKris Conceptually, yes. I'd like to see tests on this, of course. Among those tests, please ensure you can have a primary key or alt key that is an array, since a number of database platforms support that.

Oh yeah, and docs. :)

Thanks.

@SmolSoftBoi
Copy link
Contributor Author

@lonnieezell Sorry for being quiet the last few months, things have been very busy, I'd like to get these tests and docs done and committed in the new few weeks though!

@lonnieezell
Copy link
Member

@EpicKris Glad to hear you're still with us! :) Hope the project went well, and I'd be interested to hear your experiences with CI4 since it sounded like you were actually building something on it? Shoot me an email with any good points/pain points. I'd love to hear them.

And glad you'll be able to wrap this up. I was wondering which PRs were abandoned and which I would need to step in and finish.

Signed-off-by: Kristian Matthews <kristian.matthews@me.com>
Updated job model, migrations, and seeds to include job `key`.

Signed-off-by: Kristian Matthews <kristian.matthews@me.com>
Added `withAlt()` and `orWithAlt()` tests.

Signed-off-by: Kristian Matthews <kristian.matthews@me.com>
Added `key` data.

Signed-off-by: Kristian Matthews <kristian.matthews@me.com>
@SmolSoftBoi
Copy link
Contributor Author

@lonnieezell The project went fairly well and is ongoing. I'll get an email across to you with my thoughts so far!

Looks like Postgres tests are failing, I'm not able to test these myself, would someone be able to take a look for me?

@jim-parry
Copy link
Contributor

Is this PR still alive?
not to be too picky, but we expect commits to be GPG-signed :)

@SmolSoftBoi
Copy link
Contributor Author

@jim-parry This PR is still alive, although it looks like the Postgres tests are failing and I'm not able to test these myself and would need someone to be able to take a look for me.

It also seems like I need to resolve some conflicts which I'll get on to as soon as possible.

@jim-parry jim-parry added the abandoned? Activity on a pull request is almost none since the last interaction with the author label Oct 29, 2018
@atishhamte
Copy link
Contributor

Is this still alive?

@lonnieezell
Copy link
Member

I don't find this one a super-high priority item. Seeing as how it hasn't been touched 2 years, I'm going to say no one else does either. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned? Activity on a pull request is almost none since the last interaction with the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants