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.

Sign up for GitHub

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

Jump to bottom

Invalid default scenarios #528

Merged
merged 3 commits into from
Jan 22, 2019
Merged

Invalid default scenarios #528

merged 3 commits into from
Jan 22, 2019

Conversation

ikedas
Copy link
Member

@ikedas ikedas commented Jan 15, 2019

Fixed bugs:

  • WWSympa: On list config page, default settings of list scenarios are ignored. And if user updated configuration, the first option in select box is saved.
    Fixed by assigning defaults in Sympa::ListDef.
  • archive.web_access list parameter did not have default value.

Change:

  • The feature making a symbolic link "*.default" to assign default values of scenarios is deprecated.
    It was hard to determine original option value linked to default.

ikedas added 2 commits January 15, 2019 16:52
@ikedas
[bug] WWSympa: On list config page, default settings of list scenario…
c0ea8d0 
…s are ignored. And if user updated configuration, the first option in select box is saved.

Fixed by assigning defaults in Sympa::ListDef.
[change] The feature making symbolic link "*.default" to assign default values are deprecated.
@ikedas
[bug] archive.web_access list parameter did not have default value.
a693fa9
@ikedas ikedas added bug design ready A PR is waiting to be merged. Close to be solved labels Jan 15, 2019
@ikedas ikedas added this to the 6.2.40 milestone Jan 15, 2019
@dverdin
Copy link
Contributor

dverdin commented Jan 15, 2019

cool!
I was about to argue about *.default but you're right: we don't care about it. It was never actually used.
So, yes, godspeed!

@ikedas ikedas force-pushed the invalid_default_scenarios branch from 83bf1cb to 7fbb263 Compare January 18, 2019 03:02
@ikedas ikedas merged commit e949099 into sympa-community:sympa-6.2 Jan 22, 2019
@ikedas ikedas removed the ready A PR is waiting to be merged. Close to be solved label Jan 22, 2019
@dverdin
Copy link
Contributor

dverdin commented Jan 25, 2019

Hey, I'm stupid.
I can't believe I did not see it: how do you define the default scenario now?
Except by modifying ListDef, of course; I mean: how do you define the default scenario when you're an administrator? Most of these parameters don't have a sympa.conf default value.
An people having defined custom default scenarios will have their defaults ignored when upgrading.
Soji, we fucked up. This PR is a disaster. Please roll back and let's think about it again.

@dverdin dverdin mentioned this pull request Jan 25, 2019
Closed
@ikedas
Copy link
Member Author

ikedas commented Jan 25, 2019

Except by modifying ListDef? None. As I summed up, the feature defining defaults by symblic link haven't worked correctly. In other words, there have never been such ways.

@dverdin
Copy link
Contributor

dverdin commented Jan 25, 2019

Wait... So this feature NEVER worked?

@dverdin
Copy link
Contributor

dverdin commented Jan 25, 2019

Then, how does Sympa determines the default scenario for now?

@ikedas ikedas mentioned this pull request Jan 28, 2019
Merged
@ikedas
Copy link
Member Author

ikedas commented Jan 28, 2019

@dverdin, are you feeling calm now?

I added robot-/site- level defaults to list scenario parameters, as the other list parameters have. See #540.
If you found something wrong, please let us know.

I'll merge it in these weekdays.

@dverdin
Copy link
Contributor

dverdin commented Jan 29, 2019

@ikedas : Yes I do. Sorry for losing my calm. I should know better and trust your judgement since the time...
Setting the defaults in the server / virtual host configuration is perfect to me.
So you got rid of a confusing bug and even improved Sympa.
As far as I'm concerned, it's OK to close these issues.
And in the future, I'll take deep breathes before commenting.
Regards,
David

ikedas added a commit that referenced this pull request Jan 30, 2019
@ikedas
Merge pull request #540 from ikedas/invalid_default_scenarios-additio…
9d8a9da 
…n by ikedas

Invalid default scenarios: Follow up to #528
@ikedas ikedas deleted the invalid_default_scenarios branch February 1, 2019 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug design
Projects
None yet
Milestone
6.2.42
Development

Successfully merging this pull request may close these issues.
2 participants
@ikedas @dverdin