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.

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

Bug/noid/fix missing quickaccess favorite folder on add #10888

Merged

Conversation

newhinton
Copy link
Contributor

When you mark a folder as favorite, it should be added to the quickaccessbar. This is not the case, because the route for checking if the newly marked element is a folder or a directory is missing. This PR readds the missing route.

@newhinton newhinton added bug 3. to review Waiting for reviews labels Aug 27, 2018
@ChristophWurst
Copy link
Member

Hey, @newhinton,

thanks a lot for this fix. Could you please provide some reproduction steps for this? I don't fully understand your description, so I couldn't reproduce the problem.

Why do you mean by 'quickaccessbar'? The left sidebar in the files app?

@ChristophWurst
Copy link
Member

Oh, and commit 8d38f80 seems to be unrelated to this fix, correct?

@newhinton
Copy link
Contributor Author

yeah, 8d38f80 belongs to #10887 (actually, it IS #10887)
it must have sneeked in here

The quickaccessbar is the dropdown which gets added to the favorites-navigation-element in the filesapp navigation bar.

There are currently two ways do add an item to the bar:
Mark a folder as favorite with a right-klick;
Open details of said folder and klick the star in the details-bar;

Both should immediately add the folder to the quickaccess, which does not happen, because there is a route missing.

To make them show up, you need to reload the page

@skjnldsv
Copy link
Member

skjnldsv commented Oct 2, 2018

Rebased

@MorrisJobke
Copy link
Member

@skjnldsv Conflicting

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 2, 2018
@skjnldsv skjnldsv force-pushed the bug/noid/fix-missing-quickaccess-favorite-folder-on-add branch from 649c271 to 5b94fc2 Compare October 5, 2018 15:20
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 5, 2018
@skjnldsv skjnldsv force-pushed the bug/noid/fix-missing-quickaccess-favorite-folder-on-add branch from 5b94fc2 to c8af5fb Compare October 5, 2018 15:21
@juliushaertl
Copy link
Member

@newhinton Can you rebase the changes and remove the unrelated 5d2ca94 commit? Otherwise it looks good from my side.

@newhinton newhinton force-pushed the bug/noid/fix-missing-quickaccess-favorite-folder-on-add branch 2 times, most recently from 5d8af24 to 52a66a3 Compare October 10, 2018 11:13
…added to favorites

Signed-off-by: fnuesse <felix.nuesse@t-online.de>
@newhinton newhinton force-pushed the bug/noid/fix-missing-quickaccess-favorite-folder-on-add branch from 52a66a3 to 8dc4499 Compare October 10, 2018 11:34
@newhinton
Copy link
Contributor Author

@juliushaertl should be fine now :)

@rullzer
Copy link
Member

rullzer commented Oct 24, 2018

mmm are we missing commits here?

@newhinton
Copy link
Contributor Author

@rullzer What do you mean? Is this fix not working?

@juliushaertl
Copy link
Member

juliushaertl commented Oct 24, 2018

@rullzer Doesn't seem so 😉

Tested and works as expected 👍

CI failure seems unrelated:

  Scenario: change email                                               # /drone/src/github.com/nextcloud/server/tests/acceptance/features/users.feature:136
    Given I act as Jane                                                # ActorContext::iActAs()
    And I am logged in as the admin                                    # LoginPageContext::iAmLoggedInAsTheAdmin()
    And I open the User settings                                       # SettingsMenuContext::iOpenTheUserSettings()
    And I see that the list of users contains the user user0           # UsersSettingsContext::iSeeThatTheListOfUsersContainsTheUser()
    And I see that the mailAddress of user0 is ""                      # UsersSettingsContext::iSeeThatTheFieldOfUserIs()
    When I set the mailAddress for user0 to "test@nextcloud.com"       # UsersSettingsContext::iSetTheFieldForUserTo()
    And I see that the mailAddress cell for user user0 is done loading # UsersSettingsContext::iSeeThatTheCellForUserIsDoneLoading()
    Then I see that the mailAddress of user0 is "test@nextcloud.com"   # UsersSettingsContext::iSeeThatTheFieldOfUserIs()
      Failed asserting that two strings are equal.
      --- Expected
      +++ Actual
      @@ @@
      -''
      +'test@nextcloud.com'

@rullzer rullzer merged commit b3a0d87 into master Oct 24, 2018
@rullzer rullzer deleted the bug/noid/fix-missing-quickaccess-favorite-folder-on-add branch October 24, 2018 11:45
@MorrisJobke
Copy link
Member

@newhinton Mind to backport this to stable14?

@newhinton
Copy link
Contributor Author

@MorrisJobke I will try to do it on the weekend, i just create a pr with the fixes to stable14 right?

@juliushaertl
Copy link
Member

@newhinton Yes, we do backports by cherry picking the changes. The following git commands should do the job for this PR:

# checkout stable14 branch
git checkout stable14
# update branch to latest state
git pull
# create a new branch for the backport
git checkout stable14-10888
# cherry pick the commit from this pr into the backport branch
git cherry-pick 8dc449990dcd5a62e4280c1af9769582cf0a719a
# push your branch
git push

@MorrisJobke
Copy link
Member

@newhinton Do you need any further help for the backport? I plan to do the RC for the maintenance releases on Thursday so it should be back ported by then.

@newhinton
Copy link
Contributor Author

@MorrisJobke No, i just didn't found the time, but i will do it today, so there should be a PR ready tomorrow morning

@MorrisJobke
Copy link
Member

@MorrisJobke No, i just didn't found the time, but i will do it today, so there should be a PR ready tomorrow morning

Perfect. If you need help then we are here to do so :)

@newhinton
Copy link
Contributor Author

@MorrisJobke Done! :D

@MorrisJobke
Copy link
Member

@MorrisJobke Done! :D

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants