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

Stick incident #2065

Merged
merged 1 commit into from
Sep 6, 2016
Merged

Stick incident #2065

merged 1 commit into from
Sep 6, 2016

Conversation

sapk
Copy link
Contributor

@sapk sapk commented Aug 17, 2016

Implement #2023

work in branch : stick-incident

@sapk sapk force-pushed the stick-incident branch 4 times, most recently from 5adfd9e to 8e48f60 Compare August 17, 2016 02:29
*
* @var bool
*/
public $sticked;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe stickied is better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to use real words. "stuck"

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Not according to the Oxford English dictionary.

Copy link
Contributor

Choose a reason for hiding this comment

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

The savageness is real.

Though I agree with @GrahamCampbell, stuck also reads better.

Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

Copy link
Contributor Author

@sapk sapk Aug 17, 2016

Choose a reason for hiding this comment

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

Maybe a more explicite name like show(ed)_on_top or display(ed)_on_top or simply on_top ?

@ConnorVG
Copy link
Contributor

Should stickied really be a bool? It'd be cool if you can, say, stick until... No? I might be over-engineering this in my head.

@sapk
Copy link
Contributor Author

sapk commented Aug 17, 2016

If you know exacly the date of end it maybe more a maintenance ?
Like planning for a maintenance that consist as the end of monitoring maybe.

I was more thinking of being able to show a long running problem (or task) so don't know when it will be solved and that is far away in history.

@jbrooksuk
Copy link
Member

Should stickied really be a bool? It'd be cool if you can, say, stick until... No? I might be over-engineering this in my head.

Yeah, I'm with @sapk, that's more maintenance.

@ConnorVG
Copy link
Contributor

Yeah, I clearly didn't take that into account. Apologies.

You're 100% correctamundo.

@sapk
Copy link
Contributor Author

sapk commented Aug 17, 2016

Ok It start to be good. Just need some information now.
So "stickied" ? And for translation I init all to the english one ?

@sapk
Copy link
Contributor Author

sapk commented Aug 17, 2016

I don't really know why the tests failed on PUT. I think it's maybe a init of the base incident before the update that don't have sticked member but i have not find anything on that. :-(

@sapk sapk changed the title [WIP] Stick incident Stick incident Aug 19, 2016
@ConnorVG
Copy link
Contributor

Could you please make the tests pass and possibly make the definitions more consistent?

IE: https://github.com/CachetHQ/Cachet/pull/2065/files#diff-18a3c8f98774995896a403aa092a5638R115, https://github.com/CachetHQ/Cachet/pull/2065/files#diff-42a1e178504e45c7b3c2eb1207ed13e7R253, https://github.com/CachetHQ/Cachet/pull/2065/files#diff-47adb6518a63b5179a1b8476682aa9ccR78, https://github.com/CachetHQ/Cachet/pull/2065/files#diff-4053f3352543b0ef0025ff2aa1c87ce0R67

Plus, I'm unsure if stickied should really be a required parameter? Seems wise to default it to false (like in your migration). This will most likely also be the reason why the tests are failing.

@sapk
Copy link
Contributor Author

sapk commented Aug 30, 2016

For the consistent of definition do you mean beacause sometimes it's bool and other it's int ? I use the notify argument as a example and was thiniking that it's store as a int in db but it is a logical bool.

@ConnorVG
Copy link
Contributor

Utilise the data as it's expected, not as it's stored. If it's logically interpreted as a bool, manage it that way 😄

@sapk
Copy link
Contributor Author

sapk commented Aug 30, 2016

Sorry for the long commit history. I am using github web ui. I know it's bad ... i will squash them tonight

@jbrooksuk
Copy link
Member

Could you squash into one commit and rebase this please? I'll have a look then.

@jbrooksuk jbrooksuk added this to the V2.4.0 milestone Sep 4, 2016
@jbrooksuk jbrooksuk self-assigned this Sep 4, 2016
@sapk sapk force-pushed the stick-incident branch 5 times, most recently from 20121b8 to 4090053 Compare September 5, 2016 00:13
@sapk
Copy link
Contributor Author

sapk commented Sep 5, 2016

Should be ok.

@jbrooksuk
Copy link
Member

Needs rebasing again, sorry!

@sapk
Copy link
Contributor Author

sapk commented Sep 6, 2016

Little merge conflict with lang change. Should be ok.

@jbrooksuk jbrooksuk merged commit 21b785a into cachethq:2.4 Sep 6, 2016
@jbrooksuk
Copy link
Member

🎉

Thanks @sapk!

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

Successfully merging this pull request may close these issues.

4 participants