Forum OpenACS Development: Re: Changes to OpenACS 5.10 from ]project-open[?

Collapse
2: Re: Changes to OpenACS 5.10 from ]project-open[? (response to 1)
Posted by Gustaf Neumann on 05/21/20 02:49 PM
Dear Frank,

The "report" is just 262 lines, because it is mostly undocumented and is using a stone-age diff
format (why are not using the unified diff, which makes it much easier to read and provides
a context). Applying undocumented and unclear changes is not a good idea, especially
when it might break other people's code or installations.

I took the effort and worked through the list of 14 changes and added some comments
to it. For some of these  changes, i could find a possible explanation, for some not.

Please provide more details where necessary.

PS: why are you not using the OpenACS bug-tracker?
This allows detailed discussion and tracking for the issues on a per-package basis. Actually, it is not a good practice to bypass change request guidelines.

=======================================================
1) server-restarts
Restart doesn't work on Windows. Talk to Maurizio.
=======================================================

Based on a request from PO, the parameter "NsShutdownWithNonZeroExitCode"
was introduced in OpenACS 5.9.1 to handles the windows case
(PO should set this parameter).  You have confirmed that
the command "ns_shutdown -restart" works.

It is not clear, why now the command should be run in
a scheduled thread, as the patch suggests.

[1] https://openacs.org/forums/message-view?message_id=5367113

=======================================================
2) authentication-procs.tcl
Why not accept email instead of username? It's so easy.
=======================================================
if someone want to use email addresses for login, the
parameter "UseEmailForLoginP" should be used. Why is
this surgery necessary?

=======================================================
3) acs-bootstrap-installer
Tip from Brian. Cuts server startup time to half.
=======================================================
Generally deactivating automated testing does not make
sense and will break existing instances. However, in general
this is a sensible feature request to avoid loading the test files for
"production" sites .... i will look into this suggestion.

=======================================================
4) Content-Repository
Upgrade failed because of "dangling" entries.
=======================================================
This is a change in the update script from 5.9.0d1-5.9.0d2!
When this change would be applied, you will delete some
user content. This should not go into a general release,
since it can damage data in installations.

=======================================================
5) DS
Wrong URL
=======================================================
This change is hard to read. If one activates the
developer support, and calls "ds_link" in the ds/shell,
all URLs are ok, at least in OpenACS 5.10.

=======================================================
6) acs-lang
Bad HTML...
=======================================================
What is the problem? The HTML code is according to HTML
validators correct (as checked by firefox). ... Are you saying,
we should insert deprecated tags?

=======================================================
7) mail-lite
$fixed_sender not respected.
=======================================================
Not sure, what the problem is, that you are trying to fix here.
When the package parameters "FixedSenderEmail" and
"OriginatorEmail" are set, then the fields are supplied with the
correct $fixed_sender value. If there is a problem, please explain.

        # Decide which sender to use
        set fixed_sender [parameter::get \
                              -parameter "FixedSenderEmail" \
                              -package_id $mail_package_id]

        if { $fixed_sender ne "" && !$use_sender_p} {
            set from_addr $fixed_sender
        }

        # Set originator header
        set originator_email [parameter::get \
                                  -parameter "OriginatorEmail" \
                                  -package_id $mail_package_id]
        ...
        switch -exact -- $originator_email {
            fixed_sender {
                if { $fixed_sender ne "" } {
                    set originator $fixed_sender
                } elseif { $from_addr ne "" } {
                    set originator $from_addr
                } else {
                    set originator $message_id
                }
            }

=======================================================
8) calendar JS
Fixed JavaScript error in case of empty date field.
=======================================================
How can this error be reproduced? Who says that the
user wants the current date, when no date is specified?

=======================================================
9) acs-permissions
Empty party_id causes a hard error in permission_p.
=======================================================
No. The actual code does not cause a hard error, but
uses the current user_id in such cases.

=======================================================
10) http-client-procs.tcl: Fix curl issue with insecure connections
@OpenACS: Please add accept_insecure_p or similar to util::http::curl::request.
=======================================================
The Use -k1 causes to accept blindly invalid certificates.
This should not be activated by default. However, this is
a sensible feature request to add an extra flag.

=======================================================
11) date-procs
Fix issue with ISO date.
=======================================================
What is the issue with the ISO date? How can the problem be
reproduced?

=======================================================
12) calendar/view-week-display
]po[ needs a base_url to point to a different package.
=======================================================
The diff is hard to read. can you provide a unified diff
(or context diff). The current code seems different and
seems to provide the base_url already.

=======================================================
13) file-storage
Installation breaks. Ignoring is OK.
=======================================================
Blindly ignoring errors is never the best solution.
Please provide a unified diff and explain the reason
behind the desired change.

=======================================================
14) file-storage
Unregister can throw a hard error in case of non-existing data.
This breaks installation.
=======================================================
Blindly ignoring errors is never the best solution.
Please provide a unified diff and explain the reason
behind the desired change.