Skip to content

Comments

feat: let users configure their first day of week#46592

Merged
st3iny merged 1 commit intomasterfrom
feat/first-day-of-week-user-setting
Jul 23, 2024
Merged

feat: let users configure their first day of week#46592
st3iny merged 1 commit intomasterfrom
feat/first-day-of-week-user-setting

Conversation

@st3iny
Copy link
Member

@st3iny st3iny commented Jul 17, 2024 •
edited
Loading

Summary

The property will be propagated to the whole ecosystem via window.firstDay and our l10n library respectively.

Automatic

Bildschirmfoto_20240718_082159

Overwritten

Bildschirmfoto_20240718_082121

Popover open

Bildschirmfoto_20240718_081938

TODO

  • Add screenshots
  • Fix tests etc.

Checklist

@st3iny st3iny added this to the Nextcloud 30 milestone Jul 17, 2024
@st3iny st3iny self-assigned this Jul 17, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 17, 2024
View reviewed changes
apps/settings/lib/Settings/Personal/PersonalInfo.php Outdated
'headline' => $this->getProperty($account, IAccountManager::PROPERTY_HEADLINE),
'biography' => $this->getProperty($account, IAccountManager::PROPERTY_BIOGRAPHY),
'birthdate' => $this->getProperty($account, IAccountManager::PROPERTY_BIRTHDATE),
'firstDayOfWeek' => $this->config->getUserValue($uid, 'core', 'first_day_of_week')

Check notice

Code scanning / Psalm

PossiblyFalseArgument

Argument 1 of OCP\IConfig::getUserValue cannot be false, possibly null|string value expected
@st3iny st3iny added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 18, 2024
@st3iny st3iny force-pushed the feat/first-day-of-week-user-setting branch from 1cabe94 to 7d4d200 Compare July 18, 2024 06:25
@st3iny st3iny marked this pull request as ready for review July 18, 2024 06:25
@st3iny st3iny requested a review from Pytal July 18, 2024 06:26
@st3iny st3iny force-pushed the feat/first-day-of-week-user-setting branch from 7d4d200 to 4674699 Compare July 18, 2024 06:35
ChristophWurst
ChristophWurst reviewed Jul 18, 2024
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Smooth 😎

I think we only need Sunday+Monday as options. Are the other days used in any culture?

apps/provisioning_api/lib/Controller/UsersController.php
case self::USER_FIELD_FIRST_DAY_OF_WEEK:
$intValue = (int)$value;
if ($intValue < -1 || $intValue > 6) {
throw new OCSException($this->l10n->t('Invalid first day of week'), 102);
Copy link
Member

@ChristophWurst ChristophWurst Jul 18, 2024

Choose a reason for hiding this comment

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

magic number 102?

Copy link
Member Author

@st3iny st3iny Jul 18, 2024

Choose a reason for hiding this comment

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

Just like the other properties. For some reason they all return 102 in case of invalid values 🤷‍♂️

artonge
artonge reviewed Jul 18, 2024
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Should we keep the "Week starts on ..." below the "Locale" setting?

@artonge
Copy link
Contributor

artonge commented Jul 18, 2024

I think we only need Sunday+Monday as options. Are the other days used in any culture?

I imagine this is also useful to people that do not have usual weekend days, so I would keep the full list of days.

@ChristophWurst
Copy link
Member

ChristophWurst commented Jul 18, 2024 •
edited
Loading

The start day is also used in the date picker. I'm not sure it can even cope with Wednesday as start day. Something to test?

https://github.com/nextcloud-libraries/nextcloud-vue/blob/30506f203f5cbb505e6b58ecde98456ecb027881/src/components/NcDateTimePicker/NcDateTimePicker.vue#L311-L312

@st3iny
Copy link
Member Author

st3iny commented Jul 18, 2024

The start day is also used in the date picker. I'm not sure it can even cope with Wednesday as start day. Something to test?

I tested it in Calendar and it copes fine.

grafik

github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 18, 2024
View reviewed changes
apps/settings/lib/Settings/Personal/PersonalInfo.php
'headline' => $this->getProperty($account, IAccountManager::PROPERTY_HEADLINE),
'biography' => $this->getProperty($account, IAccountManager::PROPERTY_BIOGRAPHY),
'birthdate' => $this->getProperty($account, IAccountManager::PROPERTY_BIRTHDATE),
'firstDayOfWeek' => $this->config->getUserValue($uid, 'core', AUserData::USER_FIELD_FIRST_DAY_OF_WEEK),

Check notice

Code scanning / Psalm

PossiblyFalseArgument

Argument 1 of OCP\IConfig::getUserValue cannot be false, possibly null|string value expected
ChristophWurst
ChristophWurst approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good

@st3iny st3iny requested a review from artonge July 22, 2024 13:43
@st3iny st3iny mentioned this pull request Jul 22, 2024
Merged
@st3iny st3iny force-pushed the feat/first-day-of-week-user-setting branch from 76ff158 to b37fb43 Compare July 22, 2024 20:37
@st3iny st3iny added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 22, 2024
@st3iny st3iny enabled auto-merge July 22, 2024 20:37
@st3iny st3iny merged commit ee6f857 into master Jul 23, 2024
@st3iny st3iny deleted the feat/first-day-of-week-user-setting branch July 23, 2024 07:03
@tcitworld
Copy link
Member

tcitworld commented Jul 23, 2024

I think we only need Sunday+Monday as options. Are the other days used in any culture?

There's also Friday and Saturday : https://en.wikipedia.org/wiki/Week

@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
@st3iny st3iny mentioned this pull request Sep 9, 2024
Merged
@joshtrichards joshtrichards mentioned this pull request Feb 2, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@artonge artonge artonge approved these changes

@come-nc come-nc Awaiting requested review from come-nc

@Pytal Pytal Awaiting requested review from Pytal

Assignees

@st3iny st3iny

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement feature: language/translations (l10n/i18n) Localization and translation matters feature: settings

Projects

None yet

Milestone

Nextcloud 30

Development

Successfully merging this pull request may close these issues.

Starting day of the week (Sunday vs Monday)

4 participants

@st3iny @artonge @ChristophWurst @tcitworld