Skip to content

[ticket/15137] Global Announcements shouldn't always be never ending#4752

Closed
nomind60s wants to merge 2 commits intophpbb:3.1.xfrom
nomind60s:ticket/15137
Closed

[ticket/15137] Global Announcements shouldn't always be never ending#4752
nomind60s wants to merge 2 commits intophpbb:3.1.xfrom
nomind60s:ticket/15137

Conversation

@nomind60s
Copy link
Copy Markdown
Contributor

@nomind60s nomind60s commented Mar 21, 2017

To be consistent with Sticky and Announcements topic types, Global announcements can
now stick for a non-zero number of days, i.e. they are no longer never ending.

Before this change, if the topic type was Global and Stick topic for: was set to a non-zero
number of days, the number was silently ignored and a Global announcement was always
never ending.

PHPBB3-15137

Checklist:

  • Correct branch: master for new features; 3.2.x, 3.1.x for fixes
  • Tests pass
  • Code follows coding guidelines: master / 3.2.x, 3.1.x
  • Commit follows commit message format

Tracker ticket (set the ticket ID to your ticket ID):

https://tracker.phpbb.com/browse/PHPBB3-15137

…f days

To be consistent with Sticky and Announcements topic types, Global
announcements can now stick for a non-zero number of days, i.e. they are
no longer never ending.

Before this change, if the topic type was Global and a non-zero number of
days where set to stick topic, the number was silently ignored and a Global
announcement was always never ending.

PHPBB3-15137
@Nicofuma Nicofuma added this to the 3.2.1 milestone Mar 21, 2017
@Nicofuma Nicofuma changed the base branch from 3.1.x to 3.2.x March 21, 2017 10:41
@Nicofuma Nicofuma changed the base branch from 3.2.x to 3.1.x March 21, 2017 10:41
'STICKY_ANNOUNCE_TIME_LIMIT'=> 'Sticky/Announcement time limit',
'STICK_TOPIC_FOR' => 'Stick topic for',
'STICK_TOPIC_FOR_EXPLAIN' => 'Enter 0 for a never ending Sticky/Announcement. Please note that this number is relative to the date of the post.',
'STICK_TOPIC_FOR_EXPLAIN' => 'Enter 0 for a never ending Sticky/Announcement/Global. Please note that this number is relative to the date of the post.',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is necessary. There are only 2 types of posts, not three.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I waffled on it, but there are four choices given, Normal, Sticky, Announcement and Global. I've been around phpBB long enough to know that Global is a special case of Announcement, but I couldn't assume that every board admin would know that, so thought adding it would make it complete and clear.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You would have to be consistent and also change STICKY_ANNOUNCE_TIME_LIMIT. I'm not sure this change is needed at all however. I'll let the person that would merge this make that decision.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are correct I missed that one. I'm still on the fence on whether the change is necessary as well and appreciate the feedback. I tend to err on the side of caution and over-explaining. I can live with having whomever merges make the decision also. They've probably got more experience with phpBB code development than I.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't really mind if it is a bit too explicit. Especially because it will makes it clear for existing users used to the current behavior.

Change STICKY_ANNOUNCE_TIME_LIMIT for consistency

PHPBB3-15137
@Nicofuma Nicofuma closed this in c9aab0b Mar 21, 2017
@Nicofuma
Copy link
Copy Markdown
Member

Thanks for contributing (merged via c9aab0b)

@nomind60s
Copy link
Copy Markdown
Contributor Author

I'm a bit confused as to why the changes to includes/functions_posting.php were not merged. They are part of the fix.

@Nicofuma
Copy link
Copy Markdown
Member

Nicofuma commented Mar 23, 2017

certainly a mistake in the merge conflict resolution

@nomind60s
Copy link
Copy Markdown
Contributor Author

I know what the problem is, a variable name used in the changed line is different between 3.1.x and 3.2.x. Who takes care of the merge conflict resolution, the fixer or the merger?

@Nicofuma
Copy link
Copy Markdown
Member

it depends, but in this case I did and I missed it. But it's not a big deal, just send a new PR against 3.2 fixing the issue (if you don't I will do it later). You can reuse the same ticket number.

@nomind60s
Copy link
Copy Markdown
Contributor Author

I understand the changes to make, but I'm still learning the github/pull request stuff so I'll leave it to you. From what I can see, none of the changes are merged to 3.1.x and they are partially merged to 3.2.x. Too many parts that I'm not comfortable that I understand at this point.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants