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

Make shipped apps updatable via appstore #8808

Merged
merged 25 commits into from
Jun 16, 2014

Conversation

georgehrke
Copy link
Contributor

Make shipped apps updatable via the appstore. Apps need an ocsid assigned to them. See example file for how to do so. (will add to doc asap).

You can test by:

  • setting up a new ownCloud
  • adding the videos app linked below
  • test different scenarios
    • default app dir writable
    • default app dir not writable

will remove https://github.com/owncloud/core/pull/8808/files#diff-1225d18e3966cf8859db1978ce38fcaeR508 as soon as there are 2 👍s

please review @DeepDiver1975 @PVince81 @schiesbn

@georgehrke
Copy link
Contributor Author

for testing purposes: videos.zip

@georgehrke georgehrke changed the title WIP Make shipped apps updatable via appstore May 31, 2014
@georgehrke georgehrke added this to the ownCloud 7 milestone May 31, 2014
@DeepDiver1975
Copy link
Member

@georgehrke please add unit test - THX

@georgehrke
Copy link
Contributor Author

will add one on monday

@karlitschek
Copy link
Contributor

looks good to me 👍

}
}
return false;

if (count($possibleApps) === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (empty($possibleApps)) {

@PVince81
Copy link
Contributor

The update works now.
When the app store is disabled, the update button is not there.

However, there are the following additional issues:

  1. After the update, the button just disappears. I need to refresh the page to see that the version has updated.
  2. I had my "apps" folder writable, but not "apps/videos". The button still appeared. When clicked, permission denied errors appeared in the log. Then the button disappeared. No error displayed in the UI. Refresh the page: app not updated.

I believe that these issues are probably not directly related to this PR here so I suggest to address these separately.

And not to forget the issue mentionned by @MorrisJobke:
3) Verify certificate

So I'm ok to merge this to move this forward, but the certificate check should be handled before the final release.

@georgehrke
Copy link
Contributor Author

I found one tiny issue, fixing it right now

@georgehrke
Copy link
Contributor Author

any opinion on skipping codeCheck for shipped apps? wasn't sure about it

those apps will have to use the public api
@scrutinizer-notifier
Copy link

The inspection completed: 9 new issues, 18 updated code elements

@ghost
Copy link

ghost commented Jun 13, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5600/

@PVince81
Copy link
Contributor

I think it's best to keep the code check even for shipped apps, unless there is something unfixable.
But I think even shipped apps should use public API.
Not sure whether they are all fully migrated ?

CC @icewind1991

@icewind1991
Copy link
Contributor

I don't think the shipped apps only use the public api, for now I would say disable the check for shipped apps untill we've fully moved everything to the public api

@scrutinizer-notifier
Copy link

A new inspection was created.

@karlitschek
Copy link
Contributor

Yes. Unfortunately they are not fully migrated to the public api yet. So we have to disable the check for now. Let´s try again for ownCloud 8.

@georgehrke
Copy link
Contributor Author

This means we will have to be super careful when reviewing apps for the app store, as 3rdparty apps could set the shipped flag once installed

@ghost
Copy link

ghost commented Jun 16, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5624/

@georgehrke
Copy link
Contributor Author

ok,
so I disabled the code check for shipped apps,
tests are passing and no rebase is necessary.

Are there any objections left or can this be merged?

@PVince81
Copy link
Contributor

👍 you can merge this but make sure to raise a ticket for handling the HTTPS cert validation part next

georgehrke added a commit that referenced this pull request Jun 16, 2014
@georgehrke georgehrke merged commit 15c215c into master Jun 16, 2014
@georgehrke georgehrke deleted the update_shipped_apps_from_appstore branch June 16, 2014 13:54
@PVince81
Copy link
Contributor

Follow up ticket is here: #9054

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

Successfully merging this pull request may close these issues.