About patch for issue 1932 and 1951

[Jesús García Crespo]

Hi Wu,

I think the last patch you sent needs yet some more work. I'm going to talk you only about function module, ok? Then you can apply the same ideas everywhere:

Regarding to apps/qubit/modules/function/actions/browseAction.class.php:

1) Why did you create an empty if block in lines 35-38? I think this is not necessary.

    if (!QubitAcl::check($this->object, 'update'))

    {

    }

2) You should take care of code indentation also in actions code. For example:

This is not correct:

if (!QubitAcl::check($this->object, 'update'))

{

switch ($request->sort)

{

...

}

}

You should write:

if (!QubitAcl::check($this->object, 'update'))

{

  switch ($request->sort)

  {

    ...

  }

}

3) I think you can make the code simpler and avoid to repeat the switch block twice. I'd prefer this: http://pastebin.com/ZAbfg6bK, what do you think?

Regarding to apps/qubit/modules/function/templates/browseSuccess.php:

1) You decided to show the pager above the results at the same time it is shown at the bottom. I don't think you should include this modification now, please keep your patch as simple as possible and only focus on one problem. However, that idea is not a bad idea at all and it is something that we can discuss later by planning to apply the same change everywhere, if the team agree.

2) I found a bug in your code, please try to reproduce it following these steps:

    1. Open ICA-AtoM

    2. Check that you are not logged (click on log out button)

    3. Open function browser

    4. Alphabetic view is opened by default but the arrows are shown in the "Updated" cell instead of "Name".

That's all! Thank you for the excellent work you are doing. I hope you can send us a new revision soon, please let me know! :-)

Regards,

--
Jesús García Crespo,
Software Engineer, Artefactual Systems Inc.
http://www.artefactual.com | +1.604.527.2056

[wu....@usask.ca]

hi Jesús

i revised my patch and fixed the bug you found. Would you please
review it again

THanks

wu

On Mar 30, 10:52 am, Jesús García Crespo <je...@artefactual.com>
wrote:

> Hi Wu,
>
> I think the last

> patch<http://code.google.com/p/qubit-toolkit/issues/detail?id=1951#c10>you

[Jesús García Crespo]

Hi again Wu!

I have submitted your patch for issues 1951 and 1932 to qubit-trunk! However, I have realized some inconsistencies and filed new issue for it, issue 1976.

Furthermore, I send you other notes about your code that I'd like you to take into account for next patches:

1) Sorry for reiteration of the same thing, but please next time don't mix two fixes/enhancements in the same patch (issues 1932 and 1951) because it makes more difficult to analyze them.

2) Indent typos. We do:

  switch (condiction)

  {

    ...

  }

Instead of:

  switch (condition)

    {

      ...

    }

  

Or we do:

    ...

  <?php else: ?>

    <ul>

      <?php foreach ($item->getCreators(array('inherit' => true)) as $creator): ?>

        <li><?php echo link_to(render_title($creator), array($creator, 'module' => 'actor')) ?></li>

      <?php endforeach; ?>

    </ul>

  <?php endif; ?>

Instead of:

    <?php else: ?>

  <ul>

    <?php foreach ($item->getCreators(array('inherit' => true)) as $creator): ?>

      <li><?php echo link_to(render_title($creator), array($creator, 'module' => 'actor')) ?></li>

    <?php endforeach; ?>

  </ul>

    <?php endif; ?>

3) Enable debug mode when you are writing new code so you can check errors and common pitfalls. For example, after applying your patch I get three three errors like this in the information object browsew screen:

  Notice: Undefined variable: resource in [...]/apps/qubit/modules/informationobject/templates/browseSuccess.php on line 34 Repository

4) If you want to check if the user is authenticated you should do:

• In sf templates: $sf_user->isAuthenticated()
• In sf actions: $this->getUser()->isAuthenticated()

You used:

• QubitAcl::check($this->object, 'update') or
• QubitAcl::check($resource, 'update),

... but it won't work because of two reasons:

• $this->object and $this->resource were not assigned before they are accessed and there is no reason to check only for an object when the browser is showing multiple records.
• QubitAcl::check method checks if a record can be updated by the current user, but based in what the issue description says we only want to check if the user is authenticated into the application.

5) Don't do this:

  <?php echo __(sfConfig::get('app_ui_label_repository')) ?>

... because this label is already translatable, this is better:

  <?php echo sfConfig::get('app_ui_label_repository') ?>

6) I think that this piece of code

  <?php if (sfConfig::get('app_multi_repository')): ?>

    <?php echo __('Level') ?>

  <?php else: ?>

    <?php echo __('Level') ?>

  <?php endif; ?>

... could be simplified to:

  <?php echo __('Level') ?>

  

7) Same thing here:

  <?php if (sfConfig::get('app_multi_repository')): ?>

    <?php echo $item->levelOfDescription ?>

  <?php else: ?>

    <?php echo $item->levelOfDescription ?>

  <?php endif; ?>

8) When we render the repository name we check if it exists:

  <?php foreach ($item->getCreators(array('inherit' => true)) as $creator): ?>

    <li><?php echo link_to(render_title($creator), array($creator, 'module' => 'actor')) ?></li>

  <?php endforeach; ?>

... in some cases you forgot to add the condition and you directly wrote:

  <li><?php echo link_to(render_title($creator), array($creator, 'module' => 'actor')) ?></li>

9) Please, don't use tabs, instead use two spaces. If you're using Vim, you can set it up to show whitespaces and tabs, putting these directivos into your vim configuration file ($HOME/.vimrc):

  set list

  set listchars=tab:»·,trail:·,nbsp:%,extends:>,precedes:<