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

[ticket/10601] Make inbox default (instead of compose) in PM module #545

Merged
merged 10 commits into from Dec 1, 2012

Conversation

@brunoais
Copy link
Contributor

brunoais commented Jan 30, 2012

This alters how names for the "i" get variable are created.
With this, if an element has a name, the name will always be used in favor of the id. With that in mind, the inbox is shown when the "Private messages" tab is created it already has the link to the pm system with defaults to the inbox tab instead of the compose tab (the default for the i=176 is compose, the default for i=ucp_pm is the inbox)

This includes the change to the DB so that the ucp_pm is written in the db for the pm.

PHPBB3-10601

@imkingdavid

This comment has been minimized.

Copy link
Contributor

imkingdavid commented Jan 30, 2012

  • If the private messaging has a new module_id this patch will fall apart
    and it will interfere with the "new" module with the id 176.

That isn't an acceptable solution then. We can't rely on module ids because they can differ between installations and versions.

@brunoais

This comment has been minimized.

Copy link
Contributor Author

brunoais commented Jan 30, 2012

In that case... I don't know how to do this patch. After I know you read this I'll revoke this pull request.

@brunoais brunoais closed this Feb 2, 2012
@brunoais brunoais reopened this Feb 2, 2012
@brunoais
brunoais reviewed Feb 2, 2012
View changes
phpBB/includes/functions_module.php Outdated
@@ -438,6 +438,8 @@ function set_active($id = false, $mode = false)
* Loads currently active module
*
* This method loads a given module, passing it the relevant id and mode.
*
* @param string $mode mode, as passed through to the module

This comment has been minimized.

Copy link
@brunoais

brunoais Feb 2, 2012

Author Contributor

Hum... I wonder why this appears here but not on my client...

This comment has been minimized.

Copy link
@imkingdavid

imkingdavid Feb 3, 2012

Contributor

These changes in this file are from this commit: 6ad87d3

I'm not exactly sure why it did that instead of realizing that the changes have already been made by another commit, but can you checkout the latest commit and re-do your branch based off of it?

@brunoais
brunoais reviewed Feb 2, 2012
View changes
phpBB/includes/functions_module.php Outdated
* @param string $name module name (class name of the module, or its basename
* phpbb_ext_foo_acp_bar_module, ucp_zebra or zebra)
* @param string $mode mode, as passed through to the module
*

This comment has been minimized.

Copy link
@brunoais

brunoais Feb 2, 2012

Author Contributor

Hum... I wonder why this appears here but not on my client...

@brunoais
brunoais reviewed Feb 2, 2012
View changes
phpBB/includes/functions_module.php Outdated
{
$name = $class . '_' . $name;
}

This comment has been minimized.

Copy link
@brunoais

brunoais Feb 2, 2012

Author Contributor

Hum... I wonder why this appears here but not on my client...

@imkingdavid

This comment has been minimized.

Copy link
Contributor

imkingdavid commented Feb 3, 2012

Alright, here's a thought.

The RFC asked to move the folders to the top because that is what was assumed was needed to make folders show by default. However, that did not turn out to be the case to acheive the actual reason behind the RFC: to display the inbox by default. As such, folders do not need to be moved to the top, necessarily.

The reason I bring this up is for the following reasons:

  1. For instance, when using GMail, Compose link is above the folders. It is what I'm used to and expect when I use my email/messaging service
  2. In phpBB, compose is currently the first link listed, and most users are used to it like that.

Because the real request was to have the folders display by default instead of compose, I think that we can leave the folders where they are and simply do the other change to make them show up by default.

Thoughts?

@michaelcullum

This comment has been minimized.

Copy link
Member

michaelcullum commented Feb 3, 2012

+1 Makes more sense

@imkingdavid
imkingdavid reviewed Feb 4, 2012
View changes
phpBB/includes/functions_module.php Outdated
$u_title .= '&icat=' . $current_id;
}
$u_title .= '&mode=' . $item_ary['mode'];
}

This comment has been minimized.

Copy link
@imkingdavid

imkingdavid Feb 4, 2012

Contributor

According to phpBB coding guidelines, each curly bracket, { and }, should have it's own line.

Also, there should be a space after "if" in the if statements. Instead of if( it should be if (

Same thing in the changes for the other file.

This comment has been minimized.

Copy link
@brunoais

brunoais Feb 4, 2012

Author Contributor

You're right... My bad. Correction under do

@imkingdavid
imkingdavid reviewed Feb 4, 2012
View changes
phpBB/install/install_install.php Outdated
@@ -1478,8 +1478,12 @@ function add_modules($mode, $sub)

foreach ($this->module_categories[$module_class] as $cat_name => $subs)
{
$basename = '';
if(isset($module_categories_basenames[$cat_name])){

This comment has been minimized.

Copy link
@imkingdavid

imkingdavid Feb 4, 2012

Contributor

bracket needs its own line

@imkingdavid
imkingdavid reviewed Feb 4, 2012
View changes
phpBB/install/install_install.php Outdated
@@ -1507,8 +1511,12 @@ function add_modules($mode, $sub)
{
foreach ($subs as $level2_name)
{
$basename = '';
if(isset($module_categories_basenames[$level2_name])){

This comment has been minimized.

Copy link
@imkingdavid

imkingdavid Feb 4, 2012

Contributor

bracket needs its own line

@imkingdavid

This comment has been minimized.

Copy link
Contributor

imkingdavid commented Feb 5, 2012

That almost works. The only problem is that the class property $module_categories_basenames is not being properly accessed, so it doesn't put the basename in properly (it needs to be called as a class property rather than a normal variable, i.e. using $this->module_categories_basenames).

imkingdavid@6528893
That is a commit that fixes this. Please run the following:

git remote add imkingdavid git://github.com/imkingdavid/phpbb3.git
git fetch imkingdavid
#make sure you're on your ticket/10601 branch
git cherry-pick 6528893c13288075125ecab6e3c022fd26dbff03
git push origin ticket/10601

If I am recalling correctly, that is how to properly cherry pick my commit and put it into this pull request. You do not then need to squash the commits. I have tested it with this commit and it works as intended.

@imkingdavid

This comment has been minimized.

Copy link
Contributor

imkingdavid commented Mar 9, 2012

I renamed the PR title to make it more accurate as to what this does. If I can get a second opinion on this (and it's favorable) then I'll merge this. I tested it when I added my commit and it works properly.

@p

This comment has been minimized.

Copy link
Contributor

p commented Mar 18, 2012

On my test board there were no changes with the patch. Default open view continued to be compose.

@imkingdavid

This comment has been minimized.

Copy link
Contributor

imkingdavid commented Mar 18, 2012

Let me test it again. It worked for me but I'll double check and post back here with the results and steps I took.

@brunoais

This comment has been minimized.

Copy link
Contributor Author

brunoais commented Mar 18, 2012

You need to make a change in the DB.
You need to alter the _modules table.
Go to there the id is 176 or you may use module_langname to search.
Or search for UCP_PM.
Alter the module_basename = ucp_pm
then you will have it in this order in the table:
176 1 1 ucp_pm ucp 0 31 42 UCP_PM
Instead of:
176 1 1 ucp 0 31 42 UCP_PM

This system does not actually solve the situation :) it cheats.
Instead of working when you try to access the module using the module id, it solves by changing the the i to have ucp_pm instead of 176. :D

@p

This comment has been minimized.

Copy link
Contributor

p commented Apr 1, 2012

Whatever changes are necessary should be made by the code in this PR. The following cases should work correctly:

  1. Installing a new 3.1 board
  2. Installing a 3.0 board and using the database updater to update to 3.1.
@brunoais

This comment has been minimized.

Copy link
Contributor Author

brunoais commented Apr 2, 2012

Installing a new 3.1:
Tested in my system once and worked.
This was tested with the current version some weeks ago, after imkingdavid's correction

Using a 3.0 to 3.1 updater:
I cannot test. I don't have the updater.

@dhruvgoel92

This comment has been minimized.

Copy link
Contributor

dhruvgoel92 commented Apr 5, 2012

dhruvgoel92@149e73e

Using 3.0 to 3.1 updater
This commit should complete this patch.

@brunoais

This comment has been minimized.

Copy link
Contributor Author

brunoais commented Apr 6, 2012

After the correction:
Is this complete?

@@ -2363,6 +2363,12 @@ function change_database_data(&$no_updates, $version)
set_config('teampage_memberships', '1');
}
// Add module_basename ucp_pm for UCP_PM
$sql = "UPDATE " . MODULES_TABLE . "
SET module_basename = 'ucp_pm'

This comment has been minimized.

Copy link
@p-scratch

p-scratch Apr 14, 2012

What was module_basename previously? Why is only ucp_pm changed?

This comment has been minimized.

Copy link
@brunoais

brunoais Apr 14, 2012

Author Contributor

What was module_basename previously?

The empty string

Why is only ucp_pm changed?

With this change, to make the inbox appear at the top, no other changes to the DB are required.

@@ -1483,8 +1483,13 @@ function add_modules($mode, $sub)
foreach ($this->module_categories[$module_class] as $cat_name => $subs)
{
$basename = '';
if (isset($this->module_categories_basenames[$cat_name]))

This comment has been minimized.

Copy link
@p-scratch

p-scratch Apr 14, 2012

Please add a comment explaining what is going on here.

This comment has been minimized.

Copy link
@brunoais

brunoais Apr 14, 2012

Author Contributor

Done

This comment has been minimized.

Copy link
@imkingdavid

imkingdavid Aug 1, 2012

Contributor

I don't see a comment...

@@ -1512,8 +1517,13 @@ function add_modules($mode, $sub)
{
foreach ($subs as $level2_name)
{
$basename = '';
if (isset($this->module_categories_basenames[$level2_name]))

This comment has been minimized.

Copy link
@p-scratch

p-scratch Apr 14, 2012

A comment here also.

This comment has been minimized.

Copy link
@brunoais

brunoais Apr 14, 2012

Author Contributor

Done

This comment has been minimized.

Copy link
@imkingdavid

imkingdavid Aug 1, 2012

Contributor

I don't see a comment. can you double check that you pushed it?

This comment has been minimized.

Copy link
@brunoais

brunoais Aug 1, 2012

Author Contributor

It's not there... I went and check my backups... It's in the backup... Something happened...
Anyway, new commit with those comments.

@@ -2110,6 +2120,9 @@ function get_submitted_data()
'UCP_ZEBRA' => null,
),
);
var $module_categories_basenames = array(
'UCP_PM' => 'ucp_pm',

This comment has been minimized.

Copy link
@p-scratch

p-scratch Apr 14, 2012

What happens to the other modules?

This comment has been minimized.

Copy link
@imkingdavid

imkingdavid Apr 14, 2012

Contributor

Other modules are loaded as they currently are (via module ID rather than basename). The issue was that when the module was loaded by ID, it loaded the first submodule that was not set to hidden. The folders are controlled by the (hidden) viewfolders module (or something to that effect) and so even though it is first in the database, it is skipped when the modules are loaded by ID because it is hidden. This simply makes it load by basename so that it won't skip the hidden folders module, so folders will be displayed first instead of compose message.

This comment has been minimized.

Copy link
@p

p Nov 6, 2012

Contributor
  1. you are missing database updater changes for this
  2. this data should be in xcp info files, not in installer

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 7, 2012

Author Contributor

About 2.
Why? All other data about the categories themselves (AFAIK) are in install_install.php. Why would this be in the info files, then?

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Nov 10, 2012

Contributor

So you can restore the modules with the files. All information that might be different in the modules (like auth, language and their name), is in the info files. just "module enabled" and module_display arent?

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 28, 2012

Author Contributor

In that case, can you help me doing that there? I can't really find a way of properly doing it if I do it using that method.

This comment has been minimized.

Copy link
@p

p Dec 1, 2012

Contributor

Apparently there is other module data in installer. I don't know if that is what we want but I will accept putting more module data into installer.

@p-scratch

This comment has been minimized.

Copy link

p-scratch commented Apr 14, 2012

After reading the diff I have been unable to understand where the default is set/how it is determined. Please add comments in the code to this effect.

New 3.1 installation produces inbox as default view. Updating to 3.1 is broken presumably by styles changes, could not test.

$u_title .= '&icat=' . $current_id;
}
$u_title .= '&mode=' . $item_ary['mode'];
}

This comment has been minimized.

Copy link
@brunoais

brunoais Apr 14, 2012

Author Contributor

@p-scratch The default module used is set in this block of code.

@p-scratch

This comment has been minimized.

Copy link

p-scratch commented Apr 19, 2012

(07:06:01) nickvergessen: i dont really see the need for the pm thing
(07:06:20) nickvergessen: you can just click on the "no new pms" link to get to your inbox
(07:06:27) an-: great
(07:07:10) an-: well post in rfc i guess
(07:07:16) an-: that's a good point about header link

(07:09:35) an-: a full page of +1s in rfc topic
(07:09:39) an-: http://area51.phpbb.com/phpBB/viewtopic.php?f=108&t=42403

@imkingdavid

This comment has been minimized.

Copy link
Contributor

imkingdavid commented Apr 23, 2012

The issue is that two links that should go to the same place do not go to the same page. Both links should go to the inbox. If I click a link called "Private Messages", I expect to see private messages, not a form to compose a new one. If I wan to compose one, I will click the Compose link.

@erikfrerejean

This comment has been minimized.

Copy link

erikfrerejean commented Nov 7, 2012

@brunoais, yes the files are correct now.

@p
p reviewed Nov 7, 2012
View changes
phpBB/install/database_update.php Outdated

// ticket/10601: Make inbox default. Add basename to ucp's pm category
// Check if this was already applied
$sql = 'SELECT module_id, module_basename, parent_id, left_id, right_id

This comment has been minimized.

Copy link
@p

p Nov 7, 2012

Contributor

Indentation is off here.

@p
p reviewed Nov 7, 2012
View changes
phpBB/install/database_update.php Outdated
if ($row = $db->sql_fetchrow($result))
{
// Checking if this is not a category
if ($row['left_id'] === $row['right_id'] - 1)

This comment has been minimized.

Copy link
@p

p Nov 7, 2012

Contributor

This should be part of the query if it actually does anything.

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 8, 2012

Author Contributor

This needs to be off the query unless you mean I should do the comparison in the SELECT clause.
I need to know if the 1st one I get is not a category. If that happens, then this update is applied, else, it isn't.

Talk to me in IRC if you need more details.

@p

This comment has been minimized.

Copy link
Contributor

p commented Nov 8, 2012

You need to explain what you are doing in a way that is understandable.

I should be able to easily see what you did and why it works from comments in the code.

Right now I have to read the entire code you wrote, reverse engineer what it is trying to do and figure out why you thought it would work.

In this most recent example, you are assuming that categories have ids below their children, and that left/right ids are set up correctly. These are two assumptions that do not need to be made and that can lead to difficult to track down issues in the future.

From what I see in my database, category has langname=UCP_PM and modules have langname<>UCP_PM. You can use either basename=ucp_pm and langname=UCP_PM for locating the category or basename=ucp_pm and langname<>UCP_PM for locating a module.

@brunoais

This comment has been minimized.

Copy link
Contributor Author

brunoais commented Nov 8, 2012

I may not use the the basename and langname as you stated for the search.
I may use the langname=UCP_PM and basename<>ucp_pm, though. I just don't know if the langname is something that is prone to be changed.
If you want me to change the update code to be like that, then you just need to tell me so.

Hum... about the comments... I don really know how many improvements are really needed... Got any advices to help me improve 'em?

@p

This comment has been minimized.

Copy link
Contributor

p commented Nov 8, 2012

I may use the langname=UCP_PM and basename<>ucp_pm

What other basenames can there be for that langname?

I may not use the the basename and langname as you stated for the search.

Then don't. Find a module and walk up to the parent via parent_id.

Got any advices to help me improve 'em?

Write more.

You need to explain what you are doing to someone who is entirely unfamiliar with it. A single line comment for something that takes a page of discussion to figure out is not going to work.

@brunoais

This comment has been minimized.

Copy link
Contributor Author

brunoais commented Nov 8, 2012

"What other basenames can there be for that langname?"
None... I think... but if it's not found then the update is already applied.

"Then don't. Find a module and walk up to the parent via parent_id."
That's what I did.

"You need to explain what you are doing to someone who is entirely unfamiliar with it..."

I'll create and comments more verbose for all places where I made changes.

@bantu
bantu reviewed Nov 8, 2012
View changes
phpBB/install/database_update.php Outdated
$sql = 'SELECT module_id, module_basename, parent_id, left_id, right_id
FROM ' . MODULES_TABLE . '
WHERE
module_basename = \'ucp_pm\'

This comment has been minimized.

Copy link
@bantu

bantu Nov 8, 2012

Member

Move this onto the WHERE line

This comment has been minimized.

Copy link
@bantu

bantu Nov 8, 2012

Member

I'd prefer double quotes instead of escaping.

@bantu
bantu reviewed Nov 8, 2012
View changes
phpBB/install/database_update.php Outdated
FROM ' . MODULES_TABLE . '
WHERE
module_basename = \'ucp_pm\'
ORDER BY module_id';

This comment has been minimized.

Copy link
@bantu

bantu Nov 8, 2012

Member

is the order required?

This comment has been minimized.

Copy link
@p

p Nov 9, 2012

Contributor

There is an unstated assumption that categories have lower id than modules. Therefore, prior to the update this query retrieves a module with the lowest id, and on repeated database updater runs this query retrieves the category for said module. The subsequent left/right comparison is supposed to determine which of these happened based on left=right-1 for a module (leaf).

@bantu
bantu reviewed Nov 8, 2012
View changes
phpBB/install/database_update.php Outdated
@@ -2705,6 +2705,33 @@ function change_database_data(&$no_updates, $version)
$config->set('site_home_url', '');
$config->set('site_home_text', '');
}


This comment has been minimized.

Copy link
@bantu

bantu Nov 8, 2012

Member

Only one empty line please.

@bantu
bantu reviewed Nov 8, 2012
View changes
phpBB/install/database_update.php Outdated
WHERE
module_basename = \'ucp_pm\'
ORDER BY module_id';
$result = $db->sql_query_limit($sql, 1);

This comment has been minimized.

Copy link
@bantu

bantu Nov 8, 2012

Member

Use sql_query and a while loop below? There can be more than one row because you're not using a primary key for the query.

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 9, 2012

Author Contributor

I only care about the 1st result. If it is not what I'm after (the view module, the 1st one with basename=ucp_pm), then there's nothing to be done here.

@nickvergessen
nickvergessen reviewed Nov 10, 2012
View changes
phpBB/install/database_update.php Outdated
// Check if this was already applied
$sql = 'SELECT module_id, module_basename, parent_id, left_id, right_id
FROM ' . MODULES_TABLE . '
WHERE module_basename = "ucp_pm"

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Nov 10, 2012

Contributor

please use double quotes in the outer part and normal quotes on the inner part

@nickvergessen
nickvergessen reviewed Nov 10, 2012
View changes
phpBB/install/database_update.php Outdated
@@ -2706,6 +2706,31 @@ function change_database_data(&$no_updates, $version)
$config->set('site_home_text', '');
}

// ticket/10601: Make inbox default. Add basename to ucp's pm category

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Nov 10, 2012

Contributor

should propably be PHPBB3-10601 as that is what our tracker creates...

@nickvergessen
nickvergessen reviewed Nov 10, 2012
View changes
phpBB/install/database_update.php Outdated
// This update is still not applied. Applying it

$sql = 'UPDATE ' . MODULES_TABLE . '
SET module_basename = \'ucp_pm\'

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Nov 10, 2012

Contributor

please use double quotes in the outer part and normal quotes on the inner part to avoid '

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 28, 2012

Author Contributor

K'ay, done.

@nickvergessen
nickvergessen reviewed Nov 10, 2012
View changes
phpBB/install/database_update.php Outdated

}
}
$db->sql_freeresult($result);

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Nov 10, 2012

Contributor

this whole block makes no sense? you get 1 block that has basename = 'ucp_pm' and and update it's basename to 'ucp_pm' ?!!

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 28, 2012

Author Contributor

No. I get one with the basename = 'ucp_pm' and then I update it's parent's basename to 'ucp_pm', if the one I got is not a category.

@p

This comment has been minimized.

Copy link
Contributor

p commented Nov 17, 2012

Here we are waiting for a more robust/easier to follow implementation of database updating code.

Conceptually what the pr tries to do seems fine.

@brunoais

This comment has been minimized.

Copy link
Contributor Author

brunoais commented Nov 18, 2012

Here we are waiting for a more robust/easier to follow implementation of database updating code.

Got any advice?
It's been a hell to me... I don't have time to do anything I really like to do... I only have time to do my obligations with stress all the time... It's horrible! :(

I still need to find that thin time to think properly how to place the needed comments...

brunoais and others added 8 commits Feb 4, 2012
Explain in the code where
 if (isset($this->module_categories_basenames[$cat_name]))
and
if (isset($this->module_categories_basenames[$level2_name]))
exists, what does it do.

PHPBB3-10601
This is what is needed to update the database to comply with these code changes

PHPBB3-10601
Added the space after the (int) as requested

PHPBB3-10601
- Removed the double line before the addition
- Moved the condition of the WHERE so that both are in the same line
- Removed TABs from the black lines
- Used double quotes instead of escaped single quotes.

PHPBB3-10601
- Renamed the comment to PHPBB3-10601
- Removed backslashes
- Traded double quotes into single quotes inside.

PHPBB3-10601
@brunoais

This comment has been minimized.

Copy link
Contributor Author

brunoais commented Nov 28, 2012

Still waiting for help...

brunoais added 2 commits Dec 1, 2012
 - New approach in the database update algorithm

PHPBB3-10601
PHPBB3-10601
p added a commit to p/phpbb3 that referenced this pull request Dec 1, 2012
* brunoais/ticket/10601:
  [ticket/10601] The ORDER BY is only taking space there
  [ticket/10601] New approach in the update algorithm
  [ticket/10601] Comment to help understanding the code
  [ticket/10601] Requested code changes
  [ticket/10601] Cosmetic code changes
  [ticket/10601] Database updating code v2
  [ticket/10601] Database updating code
  [ticket/10601] Comment explaning the basename applied to categories
  [ticket/10601] Correctly access class property
  [ticket/10601]Move Inbox the default in private messages module
@p p merged commit 1ce0671 into phpbb:develop Dec 1, 2012
1 check passed
1 check passed
default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.