diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 44fc619..c40f76c 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -15,8 +15,6 @@ class ApplicationController < ActionController::Base def get_current_users_screenshots if user_signed_in? @current_users_screenshots = current_user.screenshots - else - @current_users_screenshots = Screenshot.uploaded_by(session[:token]) end end diff --git a/app/controllers/packages_controller.rb b/app/controllers/packages_controller.rb index 4787312..0b4bd38 100644 --- a/app/controllers/packages_controller.rb +++ b/app/controllers/packages_controller.rb @@ -29,14 +29,16 @@ class PackagesController < ApplicationController # This action saves the screenshots already if they are valid. The user is # then given the chance to comment on and delete the screenshots again. def upload_receive - # Send a cookie to remember the user by the cookie session. - create_user_token unless user_signed_in? + # Create a pseudo user account for the user. + # The user won't know that an account is created. + # But this makes it easier to track who screenshots belong to. + create_pseudo_user unless user_signed_in? @package = Package.find_by!(name: params[:name]) @valid_images = [] @invalid_images = [] - params[:file].each do |img| + params[:file].each do |img| new_screenshot = @package.screenshots.new(image: img) # Check if the image was valid @@ -44,14 +46,22 @@ class PackagesController < ApplicationController new_screenshot.uploaderhash = session[:token] new_screenshot.uploaderip = session[:ip] new_screenshot.version = @package.version + + # TODO: must it be moderated first? + new_screenshot.save - Log.log "Screenshot #{new_screenshot.id} uploaded successfully from #{session[:ip]}. User has token #{session[:token]}" + Log.log "Screenshot #{new_screenshot.id} uploaded successfully from #{session[:ip]}. User has token #{session[:token]} or is #{current_user}" @valid_images.push new_screenshot else + Log.log "Screenshot #{new_screenshot.image_file_name} invalid (#{new_screenshot.errors[:image]})." @invalid_images.push new_screenshot end end + # Show a list of invalid uploads by default. Or redirect to the review page + # if all uploads were okay. + redirect_to upload_review_path unless @invalid_images + # TODO # if @invalid_images… # ' #{image.image_file_name} (#{image.errors[:image].join(' and ')}) @@ -92,7 +102,10 @@ class PackagesController < ApplicationController # Is the user allowed to delete the screenshot? @screenshot = Screenshot.find(params[:id]) - if user_can_alter_screenshot? + # Check if the user is allowed to change this screenshot + # - Is this the user's own screenshot? (anonymous) + if self.user = current_user or current_user.is_admin? + logger.debug "User #{current_user} deletes screenshot #{@screenshot}" @screenshot.destroy flash['notice'] = "Screenshot deleted." redirect_to :back @@ -203,6 +216,17 @@ class PackagesController < ApplicationController private + # Seamlessly create a user account for the current client. + # It helps track uploads. + def create_pseudo_user + generated_password = Devise.friendly_token.first(8) + new_user = User.create( + name: 'Anonymous', + password: generated_password) + Log.log "New pseudo user for anonymous upload created: #{new_user}" + sign_in(new_user) + end + # Send a dummy thumbnail reading "No screenshot available. Sorry." def thumbnail404 send_file Rails.root.join('public/images/dummy/thumbnail404.png'), @@ -240,13 +264,6 @@ class PackagesController < ApplicationController return packages end - # Check if the user is allowed to do changed to a screenshot - def user_can_alter_screenshot? - # - Is this the user's own screenshot? - # - Is the user an admin (=logged in)? - @screenshot.uploaderhash == session[:token] or user_signed_in? - end - # Store a random identifier and the client's IP address in the session # for later identification. def create_user_token @@ -254,7 +271,6 @@ class PackagesController < ApplicationController session[:ip] ||= request.remote_ip end - # Get reviews of this package from the Ubuntu API def get_ubuntu_reviews(packagename) # Use the URL defined in the configuration to get a JSON string diff --git a/app/models/package.rb b/app/models/package.rb index 2da1678..358bfc4 100644 --- a/app/models/package.rb +++ b/app/models/package.rb @@ -48,7 +48,7 @@ class Package < ApplicationRecord # Return a query of all approved/public screenshots of this package def screenshots_approved - self.screenshots.find_by(approved: true) + self.screenshots.where(approved: true) end # Return a query of all approved/public screenshots of this package diff --git a/app/models/screenshot.rb b/app/models/screenshot.rb index 95bedab..4040bb8 100644 --- a/app/models/screenshot.rb +++ b/app/models/screenshot.rb @@ -90,33 +90,8 @@ class Screenshot < ApplicationRecord end end - # Check if the user is allowed to change this screenshot - def user_can_alter? - # - Is this the user's own screenshot? (anonymous) - if self.uploaderhash == session[:token] - logger.debug "Screenshot belongs to current anonymous user" - return true - end - - # - Is this the user's own screenshot? (anonymous) - if self.user = current_user - logger.debug "Screenshot belongs to user who is currently logged in" - return true - end - - # - Is the user an admin (=logged in)? - if current_user.can_admin? - logger.debug "User is an administrator and has super powers" - return true - end - - return false - end - # Publish a screenshot from the moderation queue def approve_screenshot! - - self.delete_reason = nil self.markedfordelete = false self.approved = true @@ -131,11 +106,6 @@ class Screenshot < ApplicationRecord self.find_by(approved: true) end - # Query for screenshots being uploaded by a certain user (by their token) - def self.uploaded_by(token) - self.where(approved: false, uploaderhash: token) - end - # Check whether the user has administrative permissions def can_admin? self.admin == 1 diff --git a/app/views/packages/_grid_thumbnail.slim b/app/views/packages/_grid_thumbnail.slim index c07fa5d..500f782 100644 --- a/app/views/packages/_grid_thumbnail.slim +++ b/app/views/packages/_grid_thumbnail.slim @@ -1,7 +1,7 @@ // TODO: Handle packages with multiple screenshots a.black href=package_path(name: pkg.name) div.grid-thumbnail - - if pkg.screenshots.any? + - if pkg.screenshots_approved.any? // TODO: smarter selection of the most useful screenshot instead of taking the first one - screenshot = pkg.screenshots.first = image_tag(screenshot.image.url(:thumb, timestamp: false), alt: screenshot.caption, class: 'thumbnail') diff --git a/app/views/packages/list.slim b/app/views/packages/list.slim index 99ae8f1..a65b7bd 100644 --- a/app/views/packages/list.slim +++ b/app/views/packages/list.slim @@ -19,7 +19,7 @@ .pkgname =pkg.name ' > - - if pkg.screenshots.any? + - if pkg.screenshots_approved.any? // TODO: smarter search for the best screenshot instead of taking the first one - screenshot = pkg.screenshots.first a.black title=screenshot.caption href=package_path(name: pkg.name) diff --git a/doc/TODO b/doc/TODO new file mode 100644 index 0000000..ae8ce2a --- /dev/null +++ b/doc/TODO @@ -0,0 +1,46 @@ +Next tasks + +> Before next deployment. + +Why does weboob-qt show an uploaded image in grid mode if no screenshot is approved? + +Fix upload process. + +Fix my/my uploads. + +Make sure that the moderation workflow works. + +Any upload will trigger the creation of an ad-hoc account. +Screenshots will always be assigned to a user. + +After an upload ask the user to add descriptions to the +screenshots. And allow them to delete the screenshots. + +Check all code with "current_user" and "user_signed_in?" to +make sure it matches the new concept. + +Create a user account called "Anonymous" - via a migration. +All existing screenshots will become uploaded by that user. + +Turn moderation into a seperate page showing just the one +screenshot and aks the user for a deletion reason. + +Make sure that users can delete their own screenshots but +no other. + +Fix formatting of http://localhost:3000/packages/list?search=weboob&show= +(Enumerations etc.) + +> After next deployment + +Make sure that ad-hoc users are recognized when they visit again. +Devise should recognize them by their session cookie. + +Offer anonymous users to become real users to track their uploads. +The screenshots will have to be moved to the new account +and the ad-hoc account can be deleted. + +Move ad-hoc/anonymous uploads to the main anonymous user after +a week. + +Add tests.