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

Temporary passwords fixes 31952 #31981

Merged
merged 1 commit into from
Jun 8, 2022
Merged

Temporary passwords fixes 31952 #31981

merged 1 commit into from
Jun 8, 2022

Conversation

StCyr
Copy link
Contributor

@StCyr StCyr commented Apr 14, 2022

fixes to #31952 31952

@StCyr StCyr force-pushed the temporary-passwords-fixes-31952 branch 3 times, most recently from 619d1fa to 11c28f4 Compare April 15, 2022 21:07
@StCyr StCyr requested review from PVince81 and CarlSchwan April 15, 2022 21:08
@StCyr StCyr force-pushed the temporary-passwords-fixes-31952 branch 2 times, most recently from 6727a72 to e40fe7e Compare April 16, 2022 12:06
@blizzz blizzz added this to the Nextcloud 25 milestone Apr 21, 2022
@PVince81 PVince81 added bug 3. to review Waiting for reviews labels Apr 25, 2022
@PVince81 PVince81 requested review from skjnldsv and Pytal May 10, 2022 08:46
@PVince81
Copy link
Member

/compile amend /

PVince81
PVince81 reviewed May 12, 2022
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot!

There's only one minor issue I found when the password already expired in the dropdown, see comment in the diff

@nextcloud-command nextcloud-command force-pushed the temporary-passwords-fixes-31952 branch from e40fe7e to b1f961b Compare May 12, 2022 07:41
@PVince81
Copy link
Member

PVince81 commented May 12, 2022 •
edited by StCyr
Loading

  • BUG: when sharing by link (no email), the message "the password expires a few seconds ago" appears in the dropdown. Please add a condition check to not display this message at all for regular link shares

@PVince81 PVince81 mentioned this pull request May 12, 2022
Closed
@StCyr
Copy link
Contributor Author

StCyr commented May 13, 2022

There are conflicting files in the dist folder. Is it important @PVince81 ?

@StCyr
Copy link
Contributor Author

StCyr commented May 13, 2022

fixed the wrongly shown "the password expires a few seconds ago" message in 4ac717b

@PVince81
Copy link
Member

@StCyr the dist folder contains compiled/minified JS assets, and probably someone else changed something there on master.

I suggest to rebase your branch and ignore the "dist" files at first.
Then run "make build-js-production" locally (slow) and then force push.

If you forget the latter, we can also summon the bot with "/compile amend /" which will compile those assets for us.

@PVince81
Copy link
Member

@StCyr please rebase onto master, this branch is still based on beta3.

also, did you address #31981 (comment) ? (I haven't tested yet)

thanks!

@StCyr StCyr force-pushed the temporary-passwords-fixes-31952 branch from 31fa4a8 to a411d9e Compare May 18, 2022 09:30
@PVince81
Copy link
Member

PVince81 commented May 18, 2022 •
edited
Loading

@StCyr you probably forgot to fetch or had an outdated master, this is why icons are missing (there was some work on master to fix it). Please try fetching again + rebasing.

Or if you like I can take over

@StCyr StCyr force-pushed the temporary-passwords-fixes-31952 branch from a411d9e to 7409316 Compare May 18, 2022 18:42
@StCyr
Copy link
Contributor Author

StCyr commented May 18, 2022

also, did you address #31981 (comment) ? (I haven't tested yet)

Just did (in 9707bf4)

@Pytal
Copy link
Member

Pytal commented May 19, 2022

dist/icons.css seems to be removed

@Pytal
Copy link
Member

Pytal commented May 19, 2022

/rebase

@StCyr StCyr force-pushed the temporary-passwords-fixes-31952 branch from f49c80d to 5eac1db Compare May 20, 2022 12:54
@StCyr
Copy link
Contributor Author

StCyr commented May 20, 2022

Seems good to go @PVince81

@StCyr StCyr force-pushed the temporary-passwords-fixes-31952 branch from 5eac1db to 67aafcf Compare May 29, 2022 17:36
@StCyr
Copy link
Contributor Author

StCyr commented May 29, 2022

No idea why it fails

@PVince81
Copy link
Member

acceptance test failure likely unrelated, some of them fail sometimes

the node issue is known to fail on master: #32591

PVince81
PVince81 approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@StCyr can you squash ?

@PVince81 PVince81 requested a review from artonge May 30, 2022 11:18
@StCyr StCyr force-pushed the temporary-passwords-fixes-31952 branch from 67aafcf to fa3aaa1 Compare May 31, 2022 14:38
artonge
artonge requested changes May 31, 2022
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good overall. Remaining work:

  • Rebase
  • Drop package-lock.json changes
  • Drop icons.css deletion
  • Squash

@StCyr StCyr force-pushed the temporary-passwords-fixes-31952 branch from fa3aaa1 to e49ef59 Compare June 8, 2022 09:05
@StCyr
Copy link
Contributor Author

StCyr commented Jun 8, 2022

Everything is green @CarlSchwan @artonge

@artonge
Copy link
Contributor

artonge commented Jun 8, 2022

/compile amend /

@StCyr @nextcloud-command
Various improvements related to the recent implementation of temporar…
50badb3 
…y passwords

for mail shares:

1- Changes style of "forgot password?" and "Back" button
2- Adds information about share password's expiration time in the emails sent.
3- Shows password expiration time in the Share menu
4- Fixes an issue when the message "Password expires..." would be shown for non email share types (which don't have temporary passswords)
5- At share's creation, password should only be sent when it's a permanent one

See also #31952

Signed-off-by: Cyrille Bollu <cyrpub@bollu.be>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@nextcloud-command nextcloud-command force-pushed the temporary-passwords-fixes-31952 branch from e49ef59 to 50badb3 Compare June 8, 2022 11:29
@artonge artonge merged commit de6b0da into master Jun 8, 2022
@artonge artonge deleted the temporary-passwords-fixes-31952 branch June 8, 2022 12:59
@szaimen
Copy link
Contributor

szaimen commented Jun 8, 2022

/backport to stable24

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jun 8, 2022
Merged
@artonge artonge mentioned this pull request Jun 20, 2022
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

@artonge artonge artonge approved these changes

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@Pytal Pytal Awaiting requested review from Pytal

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug
Projects
None yet
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.
6 participants
@StCyr @PVince81 @Pytal @artonge @szaimen @blizzz